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

Unset nodeIdentifier on Delete #158

Merged

Conversation

wjzijderveld
Copy link

I am not completely sure this is an actual bug and if so, if this is the correct fix. But it works for me ;-)

The bug I ran into can be reproduced like this:

  1. Use an existing workspace
  2. Delete a node from it (that has a jcr:uuid)
  3. $session->save()
  4. Recreate the node with a new reference
  5. $session->save()
  6. Use that node as reference for another (new) node

You will now get an PHPCR\ReferentialIntegrityException, stating that the reference you gave the does not exist.
That failed because Client.php holds an internal list of UUID's and found the UUID for the deleted Node there.

@dbu
Copy link
Member

dbu commented Dec 19, 2013

seems sane. could you maybe add a test to phpcr-api-tests that demonstrates the bug? by "recreate" you mean create it at the same path?

@wjzijderveld
Copy link
Author

Yeah, I meant recreating at the same path. In this case I was recreating some example nodes, so deleted them before creating them again.

Busy with the test right now.

@wjzijderveld
Copy link
Author

It seems I forgot to add the referenceable mixin...
Probably still a bug but with the wrong Exception?
I'll look into it later..

@dbu
Copy link
Member

dbu commented Dec 19, 2013

yeah trying to create a reference on a node without that mixin is not allowed and should trigger an error immediately. surprised it does not already.

if you have the mixin, all works as expected?

@wjzijderveld
Copy link
Author

Yes, then it works like expected. I'll try to look at the error this
evening (as well as at the still open #156)

@dbu
Copy link
Member

dbu commented Dec 28, 2013

@wjzijderveld any chance you manage to fix this in the next couple of days so we can have it in 1.1.0?

@wjzijderveld
Copy link
Author

I'll try, I had a vacation the last 10 days so I'm spending most of my
time with my wife and less time behind my computer.
Hoping to pick those up again in the next few days, but not sure if I
can create enough time to fix them.

@wjzijderveld
Copy link
Author

Added the test to phpcr-api-tests and rebased on master.

@wjzijderveld
Copy link
Author

IMO this PR can be merged as soon as phpcr/phpcr-api-tests#122 is merged.
I could replicate the issue even with the mixin, not sure what I did wrong then.

@wjzijderveld wjzijderveld mentioned this pull request Jan 4, 2014
@dbu
Copy link
Member

dbu commented Jan 4, 2014

i merged the test and retriggered the build, lets see.

@dbu dbu merged commit 366f9fb into jackalope:master Jan 4, 2014
@dbu
Copy link
Member

dbu commented Jan 4, 2014

thanks!

@wjzijderveld wjzijderveld deleted the remove-cached-identifier-on-delete branch January 9, 2014 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants