Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing bugs on inheritance system and add free primary key capability #98

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mmeloni
Copy link
Contributor

@mmeloni mmeloni commented Nov 20, 2015

Some bug fixes

@lphuberdeau
Copy link
Owner

Perhaps a test could be added?

Fix looks good enough.

…me every name. It need a getter method properly implemented.

Added a test FreePrimaryKey.
Added a inheritance test to show that previous fix is working
@mmeloni
Copy link
Contributor Author

mmeloni commented Mar 2, 2016

Added test you have request.
It's enough?

I have added another feature(I hope you like it).
Now is possible to use every value for primary key. Not only "id". I have appended also some tests to show how it work.

@lphuberdeau
Copy link
Owner

Seems like some other changes slipped in from the original request.

@mmeloni
Copy link
Contributor Author

mmeloni commented Mar 2, 2016

Do you prefer that I split all in 2 pull requests?

@lphuberdeau
Copy link
Owner

Not necessarily, but please update the PR to include some details about the purpose of the change.

Also, I know it's been lingering for a while, but could you look into that authentication failure issue?

@mmeloni
Copy link
Contributor Author

mmeloni commented Mar 3, 2016

In later versions of Neo4j it was introduced an authentication system . There are two solutions to our problem , disable the authentication in config or send an authentication header every request.
Strongly suggest the first .

@mmeloni mmeloni changed the title Fixing bug on removePreviousRelations method Fixing bugs on inheritance system and add free primary key capability Mar 3, 2016
@lphuberdeau
Copy link
Owner

lphuberdeau commented Mar 3, 2016 via email

@mmeloni
Copy link
Contributor Author

mmeloni commented Mar 3, 2016

I have no idea how to do it in travis...it's up to you right?

@lphuberdeau
Copy link
Owner

That was harder than expected. I just merged a few commits that fix the build process. Please update your branch.

@mmeloni
Copy link
Contributor Author

mmeloni commented Mar 7, 2016

Ok, all is green. What's your opinion?

{
$meta = $this->getMeta($entity);
$pk = $meta->getPrimaryKey();
$pkGetter = 'get'.ucfirst($pk->getName());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reflected property should allow to read the value directly from the property rather than relying on the naming convention. There used to be a lot of this, but it has since been removed.

I fail to understand the purpose of the change. Is it not possible to use @auto on an arbitrary property?

@mmeloni
Copy link
Contributor Author

mmeloni commented May 12, 2016

Any news? It's our intention to remain in sync with main repo.

@mmeloni mmeloni closed this May 12, 2016
@mmeloni mmeloni reopened this May 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants