Put into cache using root entity name #8009
Conversation
| foreach ($result as $index => $entity) { | ||
| $identifier = $this->uow->getEntityIdentifier($entity); | ||
| $entityKey = new EntityCacheKey($entityName, $identifier); | ||
| $entityKey = new EntityCacheKey($cm->getMetadataValue('rootEntityName'), $identifier); |
There was a problem hiding this comment.
Why do you call $cm->getMetadataValue('rootEntityName') inside the loop?
Can you please also put the = in line? Probably removing spaces of $identifier
There was a problem hiding this comment.
At first I called the property of the metadata directly and then changed to the method call in situ, hence remaining inside the loop. I see your point though, no need to call each time. Updated now.
There was a problem hiding this comment.
Fixed the alignment too.
|
|
||
| class DDC7969Test extends SecondLevelCacheAbstractTest | ||
| { | ||
| public function testChildEntityRetrievedFromCache() |
There was a problem hiding this comment.
| public function testChildEntityRetrievedFromCache() | |
| public function testChildEntityRetrievedFromCache() : void |
|
Is there anything I can do to trigger the travis build again without updating the PR? I don't think the failure is related to anything I added. |
No problem, do I need to add more reviewers? |
| $identifier = $this->uow->getEntityIdentifier($entity); | ||
| $entityKey = new EntityCacheKey($entityName, $identifier); | ||
| $identifier = $this->uow->getEntityIdentifier($entity); | ||
| $entityKey = new EntityCacheKey($rootEntityName, $identifier); |
There was a problem hiding this comment.
| $entityKey = new EntityCacheKey($rootEntityName, $identifier); | |
| $entityKey = new EntityCacheKey($cm->rootEntityName, $identifier); |
| @@ -279,9 +279,12 @@ public function put(QueryCacheKey $key, ResultSetMapping $rsm, $result, array $h | |||
|
|
|||
| $region = $persister->getCacheRegion(); | |||
|
|
|||
| $cm = $this->em->getClassMetadata($entityName); | |||
| $rootEntityName = $cm->getMetadataValue('rootEntityName'); | |||
There was a problem hiding this comment.
| $rootEntityName = $cm->getMetadataValue('rootEntityName'); |
You should directly use $cm->rootEntityName in line 287, getMetadataValue has a different use-case.
There was a problem hiding this comment.
Hi @beberlei , I originally did that but then the codesniffer job failed due to accessing a property when presented only with an interface - should we suppress or ignore that?
Actually looking at the report again, I could add the instance of check but that seems a bit clunky still:
IssueId: 42927595 Message: Accessing
rootEntityNameon the interfaceDoctrine\Common\Persistence\Mapping\ClassMetadatasuggest that you code against a concrete implementation. How about adding aninstanceofcheck?
There was a problem hiding this comment.
@peterkeatingie yes, you can add assert($cm instanceof ClassMetadata); in line 283, this code gets compiled away in production when assertions are off.
|
Thanks @beberlei @SenseException. Is there anything I can do expedite this PR or just wait now? |
|
@peterkeatingie i am waiting from someone with more SLC experience to ack. One question I have though, you made this PR towards 2.8.x branch, but I would consider this a bugfix, so this would be good for the 2.7 branch. What was your approach here? |
No problem. Actually I might have this wrong - I thought our project had 2.8 using composer show but maybe got that wrong or maybe that's for dev, I can't remember. Anyway checked the master lock file now and it's 2.7 alright. Also was following this advice: https://github.com/doctrine/orm#which-branch-should-i-choose. So if that should be against 2.7 then I guess I need to branch that and make a new PR?! |
|
Closing this in favour of #8023 |
The cache key for reading is created based on the entity root name but the cache was written with a key based on the found entity (the child in this case) resulting in cache miss.
Using the same root name when putting fixes the issue. I do believe that any extending instances should have unique identifiers as they will have to share an identifier from the base class. This will mean the key won't be ambiguous - if my thinking is correct.
#7969