#6217 when hydrating an entity with a composite primary key that is both an `EAGER` and a `LAZY` association and also cached, the `DefaultQueryCache` tries to pass L2 cache implementation detail objects to the `UnitOfWork` #6284

gadelkareem · 2017-02-10T16:19:25Z

I recreated the branch for #6217 which forced closing to PR so I am submitting this as a new PR.

DESC: Handle AssociationCacheEntry identifier

Fixes Notice: Undefined property: Doctrine\ORM\Cache\AssociationCacheEntry::$id when retrieving an associated entity from a Composite primary key

Ocramius · 2017-02-10T17:38:58Z

@lcobucci if you merge this one, can you please check if the test can be moved to a smaller SUT test?

lcobucci

@gadelkareem thanks for the patch!

@Ocramius I've managed to reduce the SUT and saw that the problem ONLY happens when the fetch mode is EAGER.

I've rebased it, removed some unrelated CS fixes and applied the same logic here.

@Ocramius although it LGTM would be nice to have your eyes too

Ocramius

Ouch, forgot to send in this review :-\

Ocramius · 2017-02-20T12:53:41Z

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

@@ -738,6 +739,15 @@ public function getIdentifierValues($entity)
        return [$id => $value];
    }

+    private function getIndentifierValue($entity, $id)


Docblock needed. id probably needs a type-hint too, if we target 2.6 only

The fact that $entity can both be an entity and a cache entry is quite bothersome... Why was this choice made?

Ocramius · 2017-02-20T12:58:14Z

tests/Doctrine/Tests/ORM/Functional/Ticket/GH6217Test.php

+ * @Entity
+ * @Cache(usage="NONSTRICT_READ_WRITE")
+ */
+class GH6217UserProfile


Could you rename this to something like GH6217EntityWith3IdentityFieldsThroughAssociation or such? I think that expresses intent much better. (yes, if you have a better name, please use that :-))

Same applies to the other entities.

Ocramius · 2017-02-20T12:58:32Z

tests/Doctrine/Tests/ORM/Functional/Ticket/GH6217Test.php

+     * @Id
+     * @Cache("NONSTRICT_READ_WRITE")
+     * @ManyToOne(targetEntity="GH6217Profile", fetch="EAGER")
+     *


No join column here?

Ocramius · 2017-02-20T12:58:41Z

tests/Doctrine/Tests/ORM/Functional/Ticket/GH6217Test.php

+     * @Id
+     * @Cache("NONSTRICT_READ_WRITE")
+     * @ManyToOne(targetEntity="GH6217Category", fetch="EAGER")
+     *


No join column here?

Ocramius · 2017-02-20T12:59:09Z

tests/Doctrine/Tests/ORM/Functional/Ticket/GH6217Test.php

@@ -0,0 +1,162 @@
+<?php
+namespace Doctrine\Tests\Functional\Ticket;


Can you add a strict types declaration, since you are being quite strict about API usage in the test?

Ocramius · 2017-08-21T07:03:08Z

Re-iterating this: I don't think that classmetadata should ever interact with an AssociationCacheEntry object. This patch fixes a symptom, rather than the cause of the issue, which is the fact that an AssociationCacheEntry object is given to the metadata in first place.

Ocramius · 2017-08-22T18:40:26Z

I checked this patch locally, and the bug is fixed on the wrong end here: the DefaultQueryCache is passing incorrect data to UnitOfWork#createEntity(). I'm currently fixing that 👍

…ry should never ever reach metadata

…id field

…e issue

…ching issue. We are not hydrating some of the cached association data into entities due to keys missing in the cache association definition. Since this is an extreme edge case that is just a mismatch between db and cache, a detailed explanation was provided in the fix snippet as well

…id fix that was actually fixing the symptom

Ocramius · 2017-08-22T22:32:57Z

@gadelkareem please check #6640 and verify it against your codebase.

gadelkareem · 2017-08-23T10:10:01Z

@Ocramius thanks for the fix and sorry for having no time to help
All tests are passing including GH6217Test so I think this is fixed.

…l2-cache-information-internals-to-the-uow #6217 #6284 when hydrating an entity with a composite primary key that is both an `EAGER` and a `LAZY` association and also cached, the `DefaultQueryCache` tries to pass L2 cache implementation detail objects to the `UnitOfWork`

Ocramius added the Bug label Feb 10, 2017

Ocramius assigned lcobucci Feb 10, 2017

Ocramius requested a review from lcobucci February 10, 2017 17:38

Ocramius added the Requires merge/review label Feb 10, 2017

lcobucci force-pushed the patch-1 branch 3 times, most recently from 6731689 to 7509c16 Compare February 19, 2017 10:48

Add tests for doctrine#6217

17d67f5

lcobucci force-pushed the patch-1 branch from 7509c16 to 17d67f5 Compare February 19, 2017 10:50

lcobucci added this to the 2.6.0 milestone Feb 19, 2017

lcobucci approved these changes Feb 19, 2017

View reviewed changes

lcobucci assigned Ocramius and unassigned lcobucci Feb 20, 2017

lcobucci requested a review from Ocramius February 20, 2017 12:45

Ocramius suggested changes Jun 21, 2017

View reviewed changes

Ocramius changed the title ~~Add tests for #6217~~ #6217 when hydrating an entity with a composite primary key that is both an EAGER and a LAZY association and also cached, the DefaultQueryCache tries to pass L2 cache implementation detail objectsto the UnitOfWork Aug 22, 2017

Ocramius added a commit that referenced this pull request Aug 22, 2017

#6284 removing the "WTF" part of the logic - an association cache ent…

a8453dd

…ry should never ever reach metadata

Ocramius added a commit that referenced this pull request Aug 22, 2017

#6284 simplifying tests to a minimum, removing unused auto-generated …

c7281f6

…id field

Ocramius added a commit that referenced this pull request Aug 22, 2017

#6284 simplifying tests to a minimum, removing unused auto-generated …

3842ad8

…id field

Ocramius added a commit that referenced this pull request Aug 22, 2017

#6284 reducing annotation mapping clutter

805ba04

Ocramius added a commit that referenced this pull request Aug 22, 2017

#6284 eager fetching is strictly required to verify the issue

4e0b76c

Ocramius added a commit that referenced this pull request Aug 22, 2017

#6284 reducing annotation mapping clutter

ca39abc

Ocramius added a commit that referenced this pull request Aug 22, 2017

#6284 removing profile entity, since it is not needed to reproduce th…

dac1ce4

…e issue

Ocramius added a commit that referenced this pull request Aug 22, 2017

#6284 adding assertions about equality of the loaded classes

6f6e88c

Ocramius added a commit that referenced this pull request Aug 22, 2017

#6284 renaming entities to match the scope of this test

220dc79

Ocramius added a commit that referenced this pull request Aug 22, 2017

#6284 correcting alignment, removing stuff that isn't strictly needed

3f09e20

Ocramius added a commit that referenced this pull request Aug 22, 2017

#6284 renaming variables, classes and properties to fit tested behavior

a2f4053

Ocramius added a commit that referenced this pull request Aug 22, 2017

#6284 removing unused separate class

c29a1e9

Ocramius added a commit that referenced this pull request Aug 22, 2017

#6284 removing unused lazy entity

c9d1f85

Ocramius added a commit that referenced this pull request Aug 22, 2017

#6284 #6217 removing hacks around the ClassMetadata details - inval…

caa008b

…id fix that was actually fixing the symptom

Ocramius mentioned this pull request Aug 22, 2017

#6217 #6284 when hydrating an entity with a composite primary key that is both an EAGER and a LAZY association and also cached, the DefaultQueryCache tries to pass L2 cache implementation detail objects to the UnitOfWork #6640

Merged

Ocramius added a commit that referenced this pull request Aug 25, 2017

#6284 #6217 s/$this->assert/self::assert as per @lcobucci's review

660f164

Ocramius closed this in #6640 Aug 25, 2017

Ocramius removed the Requires merge/review label Aug 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#6217 when hydrating an entity with a composite primary key that is both an `EAGER` and a `LAZY` association and also cached, the `DefaultQueryCache` tries to pass L2 cache implementation detail objects to the `UnitOfWork` #6284

#6217 when hydrating an entity with a composite primary key that is both an `EAGER` and a `LAZY` association and also cached, the `DefaultQueryCache` tries to pass L2 cache implementation detail objects to the `UnitOfWork` #6284

gadelkareem commented Feb 10, 2017

Ocramius commented Feb 10, 2017

lcobucci left a comment •

edited

Loading

Ocramius left a comment

Ocramius Feb 20, 2017

Ocramius Feb 20, 2017

Ocramius Feb 20, 2017

Ocramius Feb 20, 2017

Ocramius Feb 20, 2017

Ocramius Feb 20, 2017

Ocramius commented Aug 21, 2017

Ocramius commented Aug 22, 2017

Ocramius commented Aug 22, 2017

gadelkareem commented Aug 23, 2017

		@@ -0,0 +1,162 @@
		<?php
		namespace Doctrine\Tests\Functional\Ticket;

#6217 when hydrating an entity with a composite primary key that is both an EAGER and a LAZY association and also cached, the DefaultQueryCache tries to pass L2 cache implementation detail objects to the UnitOfWork #6284

#6217 when hydrating an entity with a composite primary key that is both an EAGER and a LAZY association and also cached, the DefaultQueryCache tries to pass L2 cache implementation detail objects to the UnitOfWork #6284

Conversation

gadelkareem commented Feb 10, 2017

Ocramius commented Feb 10, 2017

lcobucci left a comment • edited Loading

Choose a reason for hiding this comment

Ocramius left a comment

Choose a reason for hiding this comment

Ocramius Feb 20, 2017

Choose a reason for hiding this comment

Ocramius Feb 20, 2017

Choose a reason for hiding this comment

Ocramius Feb 20, 2017

Choose a reason for hiding this comment

Ocramius Feb 20, 2017

Choose a reason for hiding this comment

Ocramius Feb 20, 2017

Choose a reason for hiding this comment

Ocramius Feb 20, 2017

Choose a reason for hiding this comment

Ocramius commented Aug 21, 2017

Ocramius commented Aug 22, 2017

Ocramius commented Aug 22, 2017

gadelkareem commented Aug 23, 2017

#6217 when hydrating an entity with a composite primary key that is both an `EAGER` and a `LAZY` association and also cached, the `DefaultQueryCache` tries to pass L2 cache implementation detail objects to the `UnitOfWork` #6284

#6217 when hydrating an entity with a composite primary key that is both an `EAGER` and a `LAZY` association and also cached, the `DefaultQueryCache` tries to pass L2 cache implementation detail objects to the `UnitOfWork` #6284

lcobucci left a comment •

edited

Loading