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

Already on GitHub? Sign in to your account

Bug in UnitOfWork::executeDeletions when at least two entities have to b... #519

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants

maryo commented Nov 21, 2012

Bug in UnitOfWork::executeDeletions.

There is a foreach on line 1012 and dispatchEvent on line 1038.
When at least two entities have to be deleted and a listener listening to postRemove event is attached to them and there is also EntityManager::flush in that listener.

  • EntityManager::remove
  • EntityManager::remove
  • EntityManager::flush
    • UnitOfWork::commit
      • UnitOfWork::executeDeletions
        • BasicEntityPersister::delete // FOR THE FIRST REMOVED ENTITY
          ...
        • EventManager::dispatchEvent
          • PostRemoveListener::postRemove
            • EntityManager::flush
              • UnitOfWork::commit
                • UnitOfWork::executeDeletions
                  • BasicEntityPersister::delete // FOR THE SECOND REMOVED ENTITY
        • BasicEntityPersister::delete // FOR THE SECOND REMOVED ENTITY AGAIN because that foreach operates with array "copies" and that unset on line 1020 in UnitOfWork doesnt affact to the array copy
          • UnitOfWork::getEntityIdentifier // line 2735 - NOTICE, because the entity is already deleted from the array

Tests passed (those tests which are not skipped in my environment).

PHPUnit 3.7.9 by Sebastian Bergmann.

Configuration read from D:\Www\doctrine2\phpunit.xml.dist

.....................SSSSSS.................................. 61 / 1783 ( 3%)
............................................................. 122 / 1783 ( 6%)
......S.....SSSS............................................. 183 / 1783 ( 10%)
......................................................SS..... 244 / 1783 ( 13%)
............................................................. 305 / 1783 ( 17%)
............................................................. 366 / 1783 ( 20%)
......S...................................................... 427 / 1783 ( 23%)
............................................................. 488 / 1783 ( 27%)
....................SSSSSSSSSSSSS...........S................ 549 / 1783 ( 30%)
...........S...S...........S............................S.... 610 / 1783 ( 34%)
............................................................. 671 / 1783 ( 37%)
..............................................SS............. 732 / 1783 ( 41%)
....................S........................................ 793 / 1783 ( 44%)
............................................................. 854 / 1783 ( 47%)
............................................................. 915 / 1783 ( 51%)
............................................................. 976 / 1783 ( 54%)
...........................................................S. 1037 / 1783 ( 58%)
........................................S.................... 1098 / 1783 ( 61%)
............................................................. 1159 / 1783 ( 65%)
............................................................. 1220 / 1783 ( 68%)
............................................................. 1281 / 1783 ( 71%)
............................................................. 1342 / 1783 ( 75%)
............................................................. 1403 / 1783 ( 78%)
............................................................. 1464 / 1783 ( 82%)
............................................................. 1525 / 1783 ( 85%)
............................................................. 1586 / 1783 ( 88%)
............................................................. 1647 / 1783 ( 92%)
................S.............S.............................. 1708 / 1783 ( 95%)
.....................................................

Time: 14 seconds, Memory: 97.75Mb

OK, but incomplete or skipped tests!
Tests: 1761, Assertions: 5932, Skipped: 39.

Bug in UnitOfWork::executeDeletions when at least two entities have t…
…o be deleted and a listener listening to postRemove event is attached to them and there is also EntityManager::flush in that listener.

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2159

Oh btw, I just (automatically) realized that you are not creating this pull request against the master branch.

Unless there are good reasons for this, i would suggest to close and rebase the Pull Request against master and then create it again. Sorry!

maryo commented Nov 21, 2012

In project i've encounter this issue it uses 2.3 branch. So i've commited to 2.3 branch... Maybe it would be better in master... But it's just adding of one character :)...

Or it can be changed to while and each if you think it's clearer of course.

Owner

Ocramius commented Nov 21, 2012

@maryo objects are always passed by reference in PHP 5. Why are you adding that one? Can you add a failing test case for this issue?

Also: yes, master please!

Member

stof commented Nov 21, 2012

@maryo it should not be a single char change. Yo are telling that all test cases are passing, but they are also passing without your change. You need to add a test for this case.

And btw, I don't think calling flush in a listener is a supported case (it means you are calling flush inside another flush)

maryo commented Nov 21, 2012

OK. When i'll have more time I'll write test for it.

Ocramius: They are. But this is not only a reference to an object but a reference to the variable.

stof: But... Is there any other option than calling flush inside another flush? You can't just recalculate changeset bcause it's too late for postRemove i think.

Owner

Ocramius commented Nov 21, 2012

@maryo ah, got it... so that variable is replaced at some time.

You should never call another flush within a flush, that's dangerous because you will be messing with a very particular state of the UnitOfWork. I think you can eventually trigger it during postFlush, but again, it's not supposed to be done, and as @stof said, it's probably an unsupported use case.

maryo commented Nov 21, 2012

Ocramius: The key of that array is deleted on line 1019.

$array =[1,2,3];

foreach ($array as $i => $value) {
unset($array[2])
echo $value;
}

// outputs 123

foreach ($array as $i => &$value) {
unset($array[2])
echo $value;
}

// outputs 12

Owner

beberlei commented Nov 21, 2012

Calling Flush inside a flush event is not allowed.

@beberlei beberlei closed this Nov 21, 2012

Contributor

fprochazka commented Nov 21, 2012

There may be an Exception for that situation? To let the developer know, he's doing something wrong.

maryo commented Nov 21, 2012

HosipLan: Yeah. It would save me a lot of time.

Beberlei, Ocramius, Stof: I called some service called NotificationManager from that listener... It uses something like SELECT COUNT from repository... But because u say calling flush from "flush" events is not allowed, i had to move it to onFlush and i can't use that SELECT COUNT because the queries are not executed yet... So I have to calculate that count myself... Which is awful. The old way was much simpler.

Owner

Ocramius commented Nov 21, 2012

@hosiplan keeping track of the fact that the UoW is flushing or not can be useless for end users, but I just see a lot of complication when it comes to error handling. Not worth it IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment