Adding unit tests showing #3037 #7407

sandvige · 2018-09-24T21:38:29Z

sandvige · 2018-09-24T21:52:27Z

All tests are passing, except the one related to code coverage (it appears to be stuck). It is possible this dirty fix is implying a few quirks on performances (It's keep ALL entities, ever tracked), so it is possible this array is pretty big and the coverage is not very happy.

Ocramius · 2018-09-25T11:18:13Z

tests/Doctrine/Tests/ORM/Functional/IdentityMapTest.php

+ $this->_em->flush();
+
+ $articles = [];
+ for($i=0;$i<100;$i++) {


Can we reproduce this with just two?

The problem is the underlying GC implementation. With PHP 5.6, it is triggering the assert at the 85th entity. With PHP7, it's at the 100th. I think the problem is based on how the internal implementation of PHP is choosing among the available memory slots. When I try with 2, nothing happen.

PHP 5.6 is generally not relevant - what's important is to verify PHP 7. The fact that it acts on the very last iteration, yet we can't reduce the iteration count to just 2 means that we don't fully understand the problem yet.

Well, we're talking about how PHP is recycling memory. What if PHP is storing available memory slots into an hashmap? It would result into something more or less random, this is why having a lot of slots having the same size being available (due to recycling) is making this error to happens more frequently. This unit tests is just showing the problem, but there's too much random factor in this bug. Keep in mind that this is happening, and why: the documentation about spl_object_hash is clear: ids are recycled, thus not unique in time. My fix is pretty simple, it just ensure the UOW is still holding a reference on managed entities to avoid this pitfall. To be honest, I don't understand why we need a unit tests for this case, this is part of the way PHP is handling memory. And we DO have a unit test that is showing the issue, what a chance :). I think we do understand what is happening here, this is why the unit test is "statically working". I think this unit test should NOT be merged into the core as PHP might change the way it handles memory in the future. This fix is important as long as the UOW is using spl_object_hash (and spl_object_id for PHP7).

I know that object hashes are being recycled, but writing a test that relies on random optimisations is incorrect.

A better test would then be to check all array keys of all UnitOfWork properties to make sure the object hash is gone for good from anywhere, because this particular test is otherwise too fragile.

sandvige · 2018-09-25T16:16:52Z

tests/Doctrine/Tests/ORM/Functional/IdentityMapTest.php

+ $this->_em->flush();
+
+ $articles = [];
+ for($i=0;$i<100;$i++) {


The problem is the underlying GC implementation. With PHP 5.6, it is triggering the assert at the 85th entity. With PHP7, it's at the 100th. I think the problem is based on how the internal implementation of PHP is choosing among the available memory slots. When I try with 2, nothing happen.

sandvige · 2018-09-25T16:17:43Z

lib/Doctrine/ORM/UnitOfWork.php

@@ -1622,9 +1633,10 @@ public function removeFromIdentityMap($entity)

 $className = $classMetadata->rootEntityName;

+ unset($this->oidMap[$oid]);
+ unset($this->readOnlyObjects[$oid]);


There's exactly the same issue with the read only objects: they can points at dead entities. We have to clear the oid even if the entity is not found into the identityMap.

sandvige · 2018-09-25T16:20:34Z

I don't understand why the code coverage is stuck with my implementation. Is there something asserting (when using coverage only) something on the object layout of the UOW?

Fix config template for PHPUnit >= 7.2

Update homepage

Update chat link from Gitter to Slack.

[2.7] CI: Test against PHP 7.4snapshot instead of nightly (8.0)

sandvige · 2019-02-11T16:39:28Z

@Ocramius a build failed on travis because of this:

$ phpenv global 7.2 2>/dev/null
7.2 is not pre-installed; installing
Downloading archive: https://s3.amazonaws.com/travis-php-archives/binaries/ubuntu/14.04/x86_64/php-7.2.tar.bz2
404.01s$ curl -s -o archive.tar.bz2 $archive_url && tar xjf archive.tar.bz2 --directory /
curl: (56) SSL read: error:00000000:lib(0):func(0):reason(0), errno 104
0.01s0.03s$ phpenv global 7.2
rbenv: version `7.2' not installed
The command "phpenv global 7.2" failed and exited with 1 during .
Your build has been stopped.

Do you want to restart the build, because as a project maintainer, I'm pretty sure you have this button, or should I bump the PR with a fake commit?

lcobucci · 2019-02-11T17:01:41Z

@sandvige I've restarted the build. Could you please rebase this branch on top of 2.7 instead of merging things in here? It would be slightly better for reviews if we don't have all these unnecessary merge commits.

sandvige · 2019-02-12T14:50:58Z

@lcobucci Done.

bigfoot90 · 2021-08-27T15:51:08Z

Are there any news on this?
I'm also facing this issue, tried to apply this changes on my project and it's working very well for me, I'm using doctrine/orm:2.9.5 on php7.4.
Should I create a new MR from branch 2.9.x?
Please tell me if I ca do something to help.
Thanks

greg0ire · 2021-08-27T16:58:16Z

Should I create a new MR from branch 2.9.x?

Yes, that would be helpful I think! I need to read the comments on this PR to see what's missing, but let's try this 👍

bigfoot90 · 2021-09-09T15:33:02Z

@greg0ire Created new PR of this changes from 2.9.x in #8994

mpdude · 2023-06-23T18:44:28Z

@sandvige maybe you want to join #10791

Ocramius suggested changes Sep 25, 2018

View reviewed changes

sandvige commented Sep 25, 2018

View reviewed changes

Ocramius mentioned this pull request Oct 15, 2018

DDC-2332: [UnitOfWork::doPersist()] The spl_object_hash() generate not unique hash! #3037

Closed

Majkl578 force-pushed the 2.7 branch from 01cff97 to 62fb493 Compare November 11, 2018 22:32

Majkl578 and others added 8 commits November 21, 2018 01:39

Bump version to 2.7-dev

06de1fd

Set copy-method as deprecated

cd52549

Add deprecation of EntityManagerInterface::copy() to upgrade information

56ba9ac

Fix config template for PHPUnit >= 7.2

1935af7

Create 2.7 upgrade headline for deprecation changes

aa0a344

Update homepage

a49df31

Merge pull request doctrine#7340 from guilliamxavier/patch-1

4d42049

Fix config template for PHPUnit >= 7.2

Merge pull request doctrine#7382 from Majkl578/homepage-2.7

c036c0b

Update homepage

lcobucci force-pushed the 2.7 branch from 62fb493 to c036c0b Compare November 21, 2018 00:41

jwage and others added 3 commits December 12, 2018 20:10

Merge pull request doctrine#7521 from doctrine/update-chat-link

266f0d7

Update chat link from Gitter to Slack.

CI: Test against PHP 7.4snapshot instead of nightly (8.0)

eba5f20

Merge pull request doctrine#7600 from Majkl578/travis-php7.4-2.7

30c5a00

[2.7] CI: Test against PHP 7.4snapshot instead of nightly (8.0)

DDC-2332: Ensure managed entities are always tracked by UOW

1db1e88

sandvige force-pushed the 3037 branch from b37b2f5 to 1db1e88 Compare February 12, 2019 14:50

lcobucci force-pushed the 2.7 branch from 9e73672 to 40f2406 Compare September 23, 2019 09:29

lcobucci force-pushed the 2.7 branch from 40f2406 to d9c30e3 Compare October 23, 2019 13:59

bigfoot90 mentioned this pull request Sep 9, 2021

DDC-2332: Ensure managed entities are always tracked by UOW #8994

Closed

greg0ire force-pushed the 2.7 branch from 498604f to 657e013 Compare December 1, 2021 20:36

greg0ire force-pushed the 2.7 branch 2 times, most recently from e531738 to 66c95a6 Compare December 1, 2021 20:54

greg0ire changed the base branch from 2.7 to 2.10.x December 28, 2021 22:49

mpdude mentioned this pull request Jun 23, 2023

Avoid creating unmanaged proxy instances for referred-to entities during merge() #10791

Merged

greg0ire closed this in #10791 Jun 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding unit tests showing #3037 #7407

Adding unit tests showing #3037 #7407

sandvige commented Sep 24, 2018 •

edited

Loading

sandvige commented Sep 24, 2018

Ocramius Sep 25, 2018

sandvige Sep 25, 2018 •

edited

Loading

Ocramius Sep 26, 2018

sandvige Sep 26, 2018

Ocramius Sep 26, 2018

sandvige Sep 25, 2018 •

edited

Loading

sandvige Sep 25, 2018

sandvige commented Sep 25, 2018

sandvige commented Feb 11, 2019

lcobucci commented Feb 11, 2019

sandvige commented Feb 12, 2019

bigfoot90 commented Aug 27, 2021 •

edited

Loading

greg0ire commented Aug 27, 2021

bigfoot90 commented Sep 9, 2021

mpdude commented Jun 23, 2023 •

edited

Loading

Adding unit tests showing #3037 #7407

Adding unit tests showing #3037 #7407

Conversation

sandvige commented Sep 24, 2018 • edited Loading

sandvige commented Sep 24, 2018

Ocramius Sep 25, 2018

Choose a reason for hiding this comment

sandvige Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Ocramius Sep 26, 2018

Choose a reason for hiding this comment

sandvige Sep 26, 2018

Choose a reason for hiding this comment

Ocramius Sep 26, 2018

Choose a reason for hiding this comment

sandvige Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

sandvige Sep 25, 2018

Choose a reason for hiding this comment

sandvige commented Sep 25, 2018

sandvige commented Feb 11, 2019

lcobucci commented Feb 11, 2019

sandvige commented Feb 12, 2019

bigfoot90 commented Aug 27, 2021 • edited Loading

greg0ire commented Aug 27, 2021

bigfoot90 commented Sep 9, 2021

mpdude commented Jun 23, 2023 • edited Loading

sandvige commented Sep 24, 2018 •

edited

Loading

sandvige Sep 25, 2018 •

edited

Loading

sandvige Sep 25, 2018 •

edited

Loading

bigfoot90 commented Aug 27, 2021 •

edited

Loading

mpdude commented Jun 23, 2023 •

edited

Loading