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

Second level cache stores identifier with correct type even if findById is called with wrong identifier type #1556

Merged
merged 1 commit into from
Nov 25, 2015
Merged

Second level cache stores identifier with correct type even if findById is called with wrong identifier type #1556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/Doctrine/ORM/Cache/DefaultEntityHydrator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public function __construct(EntityManagerInterface $em)
public function buildCacheEntry(ClassMetadata $metadata, EntityCacheKey $key, $entity)
{
$data = $this->uow->getOriginalEntityData($entity);
$data = array_merge($data, $key->identifier); // why update has no identifier values ?
$data = array_merge($data, $metadata->getIdentifierValues($entity)); // why update has no identifier values ?
Copy link
Member

Choose a reason for hiding this comment

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

is $key->identifier generally wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure why is the line even there, I did not find the case when identifier would not be in the original entity data. But this is the line, that causes the described issue.

And in general, why should the cached entity data contain data from cache key...

Copy link
Member

Choose a reason for hiding this comment

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

There's only one use case where this can happen, which is through "loadById()", where you don't yet have an entity reference to retrieve the information through UnitOfWork.

Deferring to Hydrator indeed solves the problem, but I'd argue this is not the ideal solution, but rather find a common ground in the loadById in specific somehow. Requires further investigation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guilhermeblanco I suppose you mean in Doctrine\ORM\Cache\Persister\EntityAbstractEntityPersister::loadById? I'm not sure how to fix it in there. If you give me some hint how, I will try to fix the issue there.


foreach ($metadata->associationMappings as $name => $assoc) {
if ( ! isset($data[$name])) {
Expand Down
36 changes: 33 additions & 3 deletions tests/Doctrine/Tests/ORM/Cache/DefaultEntityHydratorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public function testBuildCacheEntryAssociation()
$this->assertArrayHasKey('name', $cache->data);
$this->assertArrayHasKey('country', $cache->data);
$this->assertEquals(array(
'id' => 11,
'id' => 12,
'name' => 'Bar',
'country' => new AssociationCacheEntry(Country::CLASSNAME, array('id' => 11)),
), $cache->data);
Expand Down Expand Up @@ -147,9 +147,39 @@ public function testBuildCacheEntryNonInitializedAssocProxy()
$this->assertArrayHasKey('name', $cache->data);
$this->assertArrayHasKey('country', $cache->data);
$this->assertEquals(array(
'id' => 11,
'id' => 12,
'name' => 'Bar',
'country' => new AssociationCacheEntry(Country::CLASSNAME, array('id' => 11)),
), $cache->data);
}
}

public function testCacheEntryWithWrongIdentifierType()
{
$proxy = $this->em->getReference(Country::CLASSNAME, 11);
$entity = new State('Bat', $proxy);
$uow = $this->em->getUnitOfWork();
$entityData = array('id'=> 12, 'name'=>'Bar', 'country' => $proxy);
$metadata = $this->em->getClassMetadata(State::CLASSNAME);
$key = new EntityCacheKey($metadata->name, array('id'=>'12'));

$entity->setId(12);

$uow->registerManaged($entity, array('id'=>12), $entityData);

$cache = $this->structure->buildCacheEntry($metadata, $key, $entity);

$this->assertInstanceOf('Doctrine\ORM\Cache\CacheEntry', $cache);
$this->assertInstanceOf('Doctrine\ORM\Cache\EntityCacheEntry', $cache);

$this->assertArrayHasKey('id', $cache->data);
$this->assertArrayHasKey('name', $cache->data);
$this->assertArrayHasKey('country', $cache->data);
$this->assertSame($entity->getId(), $cache->data['id']);
$this->assertEquals(array(
'id' => 12,
'name' => 'Bar',
'country' => new AssociationCacheEntry(Country::CLASSNAME, array('id' => 11)),
), $cache->data);
}

}