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
Don't compute changeset for entities that are going to be deleted #845
Conversation
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DDC-2790 We use Jira to track the state of pull requests and the versions they got |
@flack good catch, but this is a minor bc break, especially for people that may rely on an event listener that applies operations on changed items regardless of their state. |
@Ocramius When you say "minor BC break", does that mean the change can't be made in 2.4.x or in 2.x? It's funnny that you mention event listeners, because they are what triggers errors both in my code and in DDC-2762. In my code, I resorted to only deleting references (i.e. @stof's comment here #829 (comment) seems to indicate he considers UPDATE before DELETE a bug as well. Anyways, if the current behaviour is considered part of the API, it would be nice good to have a test for it, so that people like me would realize it before sending Pull Requests :-) |
After looking at the code a little longer, I think the use case @Ocramius describes does not really work right now, at least not for https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/UnitOfWork.php#L996 which will in turn throw an exception here: https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/UnitOfWork.php#L882 because the entity state is |
@flack this would probably land in 2.5. The breakage behind listeners is when you have a public function onFlush(...) {
foreach (... as $changed) {
if (/* is changed */ && /* is deleted */) {
$em->persist($changed); // disallow removal
}
}
} Are you saying this wouldn't work? If the example that I just made works and is broken by this PR, then we could need an entry in I agree that UPDATE + DELETE is buggy though. |
Also: needs tests! |
@Ocramius I got your example to work now: https://gist.github.com/flack/7437117 So it is actually possible to cancel deletions from onFlush handlers, but AFAICT only with implicit change tracking (it's line 35 in the gist that makes it work). But he price for this is superfluous changeset calculations & update statements for people not using So it's your call to make: If you want to keep the current https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/UnitOfWork.php#L739 which is (as everything the preceding lines) exactly the same as https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/UnitOfWork.php#L451 I just noticed this duplication and think it should probably be removed (in favor of making |
P.S.: The superfluous UPDATE statement could be avoided by adding this $this->entityUpdates = array_diff_key($this->entityUpdates, $this->entityDeletions); here: https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/UnitOfWork.php#L325 That wouldn't change the semantics like the PR does, but I'm not sure if avoiding one UPDATE statement is really worth it (especially since the |
@flack I'm fine with the breakage as long as it's documented. I'm not questioning the validity of the PR. Let me rephrase:
|
@Ocramius Alright, I'll try to finish this up in the next few days then. Do you have any preference as far as the unit test is concerned? I was thinking of just testing whether or not |
@Ocramius I added a basic test, some documentation and changed a second occurence of the same logic in UoW now, so I think this should be ready for another round of review. |
Looks great, merged! |
Don't compute changeset for entities that are going to be deleted
This is somewhat related to
http://www.doctrine-project.org/jira/browse/DDC-2761
I'm not sure if it fixes that particular problem, but it certainly helps with some unrelated issues I'm seeing in my code. My theory is that doctrine doesn't even need to compute changesets when an entity is scheduled to be removed, because all that would accomplish is to insert an UPDATE statement immediately before the DELETE statement for the entity.
The test suite passes, but I have not added a test specifically for this change (mostly because I wouldn't know exactly what to test). I could try to create one if necessary, though