Fix incorrect return of null values in L2C #7750

AlexSmerw · 2019-06-20T14:30:49Z

Bug fix for issue 7735 for doctrine 2.6

lcobucci

@AlexSmerw thanks for your contribution!

This does look sane from a very quick look but could you please add a functional that reproduces the issue and proves that the fix solves it?

AlexSmerw · 2019-06-21T06:28:33Z

@AlexSmerw thanks for your contribution!

This does look sane from a very quick look but could you please add a functional that reproduces the issue and proves that the fix solves it?

This is not enough - description with link on code ?

lcobucci · 2019-06-21T09:34:00Z

Not really, we need a phpunit test that gets integrated to the codebase, which also ensures that this problem will never happen again.

You can find examples on https://github.com/doctrine/doctrine2/tree/388afb46d0cb3ed0c51332e8df0de9e942c2690b/tests/Doctrine/Tests/ORM/Functional/Ticket

AlexSmerw · 2019-06-25T11:26:59Z

Not really, we need a phpunit test that gets integrated to the codebase, which also ensures that this problem will never happen again.

You can find examples on https://github.com/doctrine/doctrine2/tree/388afb46d0cb3ed0c51332e8df0de9e942c2690b/tests/Doctrine/Tests/ORM/Functional/Ticket

I have added unitTest in my branch. If you use this test Issue7735Test without my edits, you will get Exception TypeError: Return value of Doctrine\Tests\Models\Issue7735\Engine::getModel() must be of the type string, null returned. Look, I set 1000000 iterations for getting this bug. I think this count depends on you PC

lcobucci

Thank you very much for adding the test, I've got a tiny adjustment request: can you please put the models in the same file as the test and remove all attributes which might be irrelevant to reproducible the issue?

lcobucci · 2019-06-25T21:02:26Z

tests/Doctrine/Tests/OrmFunctionalTestCase.php

-            $factory        = new DefaultCacheFactory($cacheConfig->getRegionsConfiguration(), $cache);
+
+            $regionConfig = $cacheConfig->getRegionsConfiguration();
+            $regionConfig->setDefaultLifetime(static::DEFAULT_CACHE_REGION_LIFE_TIME);


Is this change really required for the test?

For reproducing this problem, I set second level cache time limit = 1 second. This code need for this.

Sorry, what I meant to say was: can't we keep this contained to the functional test only? This is now changing the default for every test in the suite, which might create undesired side-effects 😁

Sorry, what I meant to say was: can't we keep this contained to the functional test only? This is now changing the default for every test in the suite, which might create undesired side-effects grin

Done

AlexSmerw · 2019-07-08T08:45:25Z

Thank you very much for adding the test, I've got a tiny adjustment request: can you please put the models in the same file as the test and remove all attributes which might be irrelevant to reproducible the issue?
Hi! I have done this.

lcobucci · 2019-07-08T08:56:28Z

@AlexSmerw it seems like we still have separate files for each class... Check this example: https://github.com/doctrine/orm/blob/388afb46d0cb3ed0c51332e8df0de9e942c2690b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6362Test.php

AlexSmerw · 2019-07-08T15:02:38Z

@AlexSmerw it seems like we still have separate files for each class... Check this example: https://github.com/doctrine/orm/blob/388afb46d0cb3ed0c51332e8df0de9e942c2690b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6362Test.php

Oh! It's Evil!

AlexSmerw · 2019-07-09T11:26:46Z

@AlexSmerw it seems like we still have separate files for each class... Check this example: https://github.com/doctrine/orm/blob/388afb46d0cb3ed0c51332e8df0de9e942c2690b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6362Test.php

Done. Check it, please.

lcobucci · 2019-07-09T14:44:49Z

@AlexSmerw I'll take a look at it ASAP, thanks for applying the changes!

AlexSmerw · 2019-07-24T07:24:52Z

@AlexSmerw I'll take a look at it ASAP, thanks for applying the changes!
Hi!
My request still with label Missing Tests. Is it ok?
I worry about its fate :)

greg0ire · 2019-07-26T06:26:09Z

Please associate your commit email address with your Github account, or change the email in your commits to an address already associated with it. Also, you should probably squash these 9 commits.

More importantly, please improve your commit message, it's unclear what was broken, and why your changes fix anything.

lib/Doctrine/ORM/Cache/Persister/Entity/AbstractEntityPersister.php

lcobucci

@AlexSmerw I've managed to clean up your test and remove the need for the cache TTL configuration and the for loop.

Thank you very much for fixing this bug!

The same variable name is used below, and that causes a bug etc. Fixes doctrine#7735

@mlocati

v2.6.4 [![Build Status](https://travis-ci.org/doctrine/orm.svg?branch=v2.6.4)](https://travis-ci.org/doctrine/orm) In this release we've fixes many bugs (including a performance regression) and made the v2.x series compatible with PHP 7.4. -------------------------------------------- - Total issues resolved: **11** - Total pull requests resolved: **32** - Total contributors: **30** Improvement ----------- - [7785: Fix "access array offset on value of type null" PHP 7.4 notices](doctrine#7785) thanks to @mlocati - [7142: Rename this repository to doctrine/orm](doctrine#7142) thanks to @greg0ire Bug ------------------ - [7821: Bug: doctrine#7820 paginator ignores dbal type conversions in identifiers](doctrine#7821) thanks to @Ocramius - [7778: Guard L2C regions against corrupted data](doctrine#7778) thanks to @umpirsky - [7767: PersistentCollection::matching() does not respect the collections native sorting](doctrine#7767) thanks to @stephanschuler - [7766: Respect collection orderBy meta when matching()](doctrine#7766) thanks to @stephanschuler - [7761: Do not modify UOW on PersistentCollection::clear() when owner has DEFFERED&doctrine#95;EXPLICIT change tracking policy](doctrine#7761) thanks to @paxal - [7750: Fix incorrect return of null values in L2C](doctrine#7750) thanks to @AlexSmerw - [7737: Fix MEMBER&doctrine#95;OF comparison when using criteria in query builder](doctrine#7737) thanks to @Smartel1 - [7735: Null in fields value in Cached Entity several times on day on high-load project.](doctrine#7735) thanks to @AlexSmerw - [7630: Fix doctrine#7629 - `scheduledForSynchronization` leaks memory when using `@ORM\ChangeTrackingPolicy("DEFERRED&doctrine#95;EXPLICIT")`](doctrine#7630) thanks to @yethee - [7528: Prevent `UnitOfWork` lookup for DBAL types specified in `Doctrine\ORM\Query#setParameter()`](doctrine#7528) thanks to @Ocramius - [7322: JoinedSubclassPersister pass identifier types on delete](doctrine#7322) thanks to @dennisenderink and @fred-jan - [7266: Call to a member function resolveAssociationEntries() on boolean {"detail":"&doctrine#91;object&doctrine#93; (Symfony\\Component\\Debug\\Exception\\FatalThrowableError(code: 0): Call to a member function resolveAssociationEntries() on boolean at /www/vendor/doctrine/orm/lib/Doctrine/ORM/Cache/DefaultQueryCache.php:140)"}](doctrine#7266) thanks to @mingmingxianseng - [4632: DDC-3789: Paginator does not convert entity ids if they are value objects](doctrine#4632) thanks to @doctrinebot Documentation ------------- - [7818: Add note into docs about not using SimpleAnnotationReader](doctrine#7818) thanks to @SenseException - [7791: Fix preFlush event documentation stating incorrectly that flush can be called safely](doctrine#7791) thanks to @Steveb-p - [7753: Add ORM annotations in getting-started docs](doctrine#7753) thanks to @SenseException and @wajdijurry - [7744: Fixed a typo-error](doctrine#7744) thanks to @noobshow - [7732: &doctrine#91;Documentation&doctrine#93; Missing comma fix](doctrine#7732) thanks to @lchrusciel - [7729: Update DATE&doctrine#95;ADD and DATE&doctrine#95;SUB docs](doctrine#7729) thanks to @JoppeDC - [7672: Added cross-links to relevant documentation](doctrine#7672) thanks to @jschaedl - [7612: Update ordered-associations.rst](doctrine#7612) thanks to @spirlici - [7610: Change APC to OPcache in improving-performance.rst ](doctrine#7610) thanks to @smtchahal - [7596: Correct method names and broken link in docs](doctrine#7596) thanks to @mbessolov - [7577: Fix of single link to dbal docs in advanced-configuration.rst](doctrine#7577) thanks to @SenseException - [7572: Remove codeigniter Framework example](doctrine#7572) thanks to @SenseException - [7571: Fix typo in inheritance mappings docs](doctrine#7571) thanks to @batwolf - [7557: Change Stackoverflow tag to doctrine-orm](doctrine#7557) thanks to @malarzm - [7551: &doctrine#91;2.6&doctrine#93; Migrate repository name doctrine/doctrine2 -> doctrine/orm](doctrine#7551) thanks to @Majkl578 - [7530: Documentation error typo fix: s/Used-defined/User-Defined](doctrine#7530) thanks to @vladyslavstartsev - [7519: doctrine#7518 Fixed type mismatch between `EntityRepository#&doctrine#95;&doctrine#95;construct()` and its documented constructor arguments](doctrine#7519) thanks to @koftikes - [7518: `EntityRepository::&doctrine#95;&doctrine#95;construct()` expects `Doctrine\ORM\EntityManager` instead of actual required `EntityManagerInterface`](doctrine#7518) thanks to @koftikes - [7490: Fix broken link](doctrine#7490) thanks to @vladyslavstartsev - [7483: Fixed a minor syntax issue](doctrine#7483) thanks to @javiereguiluz CI ----------------- - [7794: Fix test compatibility with DBAL 2.10.x-dev](doctrine#7794) thanks to @lcobucci - [7731: Replace custom install script with add-on](doctrine#7731) thanks to @greg0ire - [7473: Incremental CS checks in 2.x branches](doctrine#7473) thanks to @Majkl578

AlexSmerw mentioned this pull request Jun 20, 2019

null values in cached entity fix #7749

Closed

lcobucci reviewed Jun 20, 2019

View reviewed changes

lcobucci assigned AlexSmerw Jun 21, 2019

lcobucci added Bug Missing Tests labels Jun 21, 2019

lcobucci suggested changes Jun 25, 2019

View reviewed changes

lcobucci self-requested a review July 9, 2019 14:44

greg0ire removed the Missing Tests label Jul 26, 2019

greg0ire reviewed Jul 26, 2019

View reviewed changes

lib/Doctrine/ORM/Cache/Persister/Entity/AbstractEntityPersister.php Show resolved Hide resolved

lcobucci force-pushed the issue_7735_null_values_in_entities_cache_for_2.6 branch from 8dc80f9 to bf3c9b7 Compare August 11, 2019 22:55

lcobucci added this to the 2.6.4 milestone Aug 11, 2019

lcobucci force-pushed the issue_7735_null_values_in_entities_cache_for_2.6 branch 2 times, most recently from 20b7faa to 4cd44d0 Compare August 11, 2019 22:57

lcobucci previously approved these changes Aug 11, 2019

View reviewed changes

lcobucci added the Pick to master label Aug 11, 2019

lcobucci changed the title ~~null values in cached entity fix for 2.6~~ Fix incorrect return of null values in L2C Aug 11, 2019

lcobucci dismissed their stale review via 5234ca2 August 11, 2019 23:13

lcobucci force-pushed the issue_7735_null_values_in_entities_cache_for_2.6 branch 2 times, most recently from 5234ca2 to a4c567f Compare August 11, 2019 23:15

Avoid reusing variable name

e8f9143

The same variable name is used below, and that causes a bug etc. Fixes doctrine#7735

lcobucci force-pushed the issue_7735_null_values_in_entities_cache_for_2.6 branch from a4c567f to e8f9143 Compare August 11, 2019 23:18

lcobucci approved these changes Aug 11, 2019

View reviewed changes

lcobucci merged commit 6e56bcd into doctrine:2.6 Aug 11, 2019

lcobucci mentioned this pull request Aug 11, 2019

Null in fields value in Cached Entity several times on day on high-load project. #7735

Closed

lcobucci assigned lcobucci and unassigned AlexSmerw Aug 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect return of null values in L2C #7750

Fix incorrect return of null values in L2C #7750

AlexSmerw commented Jun 20, 2019

lcobucci left a comment

AlexSmerw commented Jun 21, 2019

lcobucci commented Jun 21, 2019

AlexSmerw commented Jun 25, 2019

lcobucci left a comment

lcobucci Jun 25, 2019

AlexSmerw Jun 26, 2019

lcobucci Jun 26, 2019

AlexSmerw Jul 8, 2019

AlexSmerw commented Jul 8, 2019

lcobucci commented Jul 8, 2019

AlexSmerw commented Jul 8, 2019

AlexSmerw commented Jul 9, 2019

lcobucci commented Jul 9, 2019

AlexSmerw commented Jul 24, 2019

greg0ire commented Jul 26, 2019 •

edited

Loading

lcobucci left a comment •

edited

Loading

Fix incorrect return of null values in L2C #7750

Fix incorrect return of null values in L2C #7750

Conversation

AlexSmerw commented Jun 20, 2019

lcobucci left a comment

Choose a reason for hiding this comment

AlexSmerw commented Jun 21, 2019

lcobucci commented Jun 21, 2019

AlexSmerw commented Jun 25, 2019

lcobucci left a comment

Choose a reason for hiding this comment

lcobucci Jun 25, 2019

Choose a reason for hiding this comment

AlexSmerw Jun 26, 2019

Choose a reason for hiding this comment

lcobucci Jun 26, 2019

Choose a reason for hiding this comment

AlexSmerw Jul 8, 2019

Choose a reason for hiding this comment

AlexSmerw commented Jul 8, 2019

lcobucci commented Jul 8, 2019

AlexSmerw commented Jul 8, 2019

AlexSmerw commented Jul 9, 2019

lcobucci commented Jul 9, 2019

AlexSmerw commented Jul 24, 2019

greg0ire commented Jul 26, 2019 • edited Loading

lcobucci left a comment • edited Loading

Choose a reason for hiding this comment

greg0ire commented Jul 26, 2019 •

edited

Loading

lcobucci left a comment •

edited

Loading