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

[maryo]

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.

[doctrinebot]

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

[doctrinebot]

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]

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.

[Ocramius]

@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!

[stof]

@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]

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.

[Ocramius]

@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]

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

[beberlei]

Calling Flush inside a flush event is not allowed.

[fprochazka]

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

[maryo]

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.

[Ocramius]

@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.