Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Put into cache using root entity name #8009

Closed
wants to merge 8 commits into from
Closed

Put into cache using root entity name #8009

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a999918
Put into cache using root entity name
goodtravelsoftware-zz Jan 31, 2020
87292e8
Coding standards fixes
goodtravelsoftware-zz Jan 31, 2020
42f20f7
CS fix
peterkeatingie Jan 31, 2020
8120eb3
Fix scrutinizer issues
peterkeatingie Feb 1, 2020
c813064
Move method call, fix alignment
goodtravelsoftware-zz Feb 4, 2020
f8c1f0a
Add return type
goodtravelsoftware-zz Feb 4, 2020
ba0785c
Add space before colon
goodtravelsoftware-zz Feb 4, 2020
e520a98
Use property directly. Update test.
peterkeatingie Feb 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/Doctrine/ORM/Cache/DefaultQueryCache.php
View file
Open in desktop
Expand Up @@ -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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$rootEntityName = $cm->getMetadataValue('rootEntityName');

You should directly use $cm->rootEntityName in line 287, getMetadataValue has a different use-case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rootEntityName on the interface Doctrine\Common\Persistence\Mapping\ClassMetadata suggest that you code against a concrete implementation. How about adding an instanceof check?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterkeatingie yes, you can add assert($cm instanceof ClassMetadata); in line 283, this code gets compiled away in production when assertions are off.


foreach ($result as $index => $entity) {
$identifier = $this->uow->getEntityIdentifier($entity);
$entityKey = new EntityCacheKey($entityName, $identifier);
$identifier = $this->uow->getEntityIdentifier($entity);
$entityKey = new EntityCacheKey($rootEntityName, $identifier);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$entityKey = new EntityCacheKey($rootEntityName, $identifier);
$entityKey = new EntityCacheKey($cm->rootEntityName, $identifier);


if (($key->cacheMode & Cache::MODE_REFRESH) || ! $region->contains($entityKey)) {
// Cancel put result if entity put fail
Expand Down
48 changes: 48 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/Ticket/DDC7969Test.php
View file
Open in desktop
@@ -0,0 +1,48 @@
<?php

declare(strict_types=1);

namespace tests\Doctrine\Tests\ORM\Functional\Ticket;

use Doctrine\ORM\Cache\Region\DefaultMultiGetRegion;
use Doctrine\Tests\Models\Cache\Attraction;
use Doctrine\Tests\Models\Cache\Bar;
use Doctrine\Tests\ORM\Functional\SecondLevelCacheAbstractTest;

class DDC7969Test extends SecondLevelCacheAbstractTest
{
public function testChildEntityRetrievedFromCache() : void
{
$this->loadFixturesCountries();
$this->loadFixturesStates();
$this->loadFixturesCities();
$this->loadFixturesAttractions();

// Entities are already cached due to fixtures - hence flush before testing
$region = $this->cache->getEntityCacheRegion(Attraction::class);

if ($region instanceof DefaultMultiGetRegion) {
$region->getCache()->flushAll();
}

/** @var Bar $bar */
$bar = $this->attractions[0];

$repository = $this->_em->getRepository(Bar::class);

$this->assertFalse($this->cache->containsEntity(Bar::class, $bar->getId()));
beberlei marked this conversation as resolved.
Show resolved Hide resolved

$repository->findOneBy([
'name' => $bar->getName(),
]);

$this->assertTrue($this->cache->containsEntity(Bar::class, $bar->getId()));

$repository->findOneBy([
'name' => $bar->getName(),
]);

// One hit for entity cache, one hit for query cache
$this->assertEquals(2, $this->secondLevelCacheLogger->getHitCount());
}
}