Entity cache key is built differently on read than on write

[guiwoda]

Related to #1552, cache keys are scattered throughout L2 implementation and are built inconsistently.

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-4003

We use Jira to track the state of pull requests and the versions they got
included in.

[guiwoda]

The DefaultQueryCacheTest has a region stub that doesn't check on key hashes and returns entries without being able to assert on arguments. Switching those tests to Phpunit mocks could have hinted on this error.

[guiwoda]

Also, DefaultQueryCache is way too coupled to everything going on around L2. It has access to the EntityManager, to UnitOfWork, to Persisters... it practically knows the whole ORM.

[Ocramius]

@guiwoda how did you get to this bug? I can't consider the patch without a test scenario that validates it :-\

[guiwoda]

You need a cacheable (nonstrict_rw, but could be any) pair of entities in a INHERITANCE_SINGLE form.

Use a Repository to findBy(['id' => $id]), this will cache entities as it reads them.
Save one entity to trigger its cache eviction.
Read again.

You'll notice that cache updates are always done through the $metadata->rootEntityName, while AbstractEntityPersister::load ends up building EntityCacheKey by the entity name, and not its root name.

[Ocramius]

@guiwoda I am asking for a test case (phpunit test), not a description only, heh :-)

[guiwoda]

I know 😄
Will push a functional test (couldn't do with those non-mocks in the DefaultQueryCacheTest), I guess by tomorrow!

[guiwoda]

@Ocramius there's the test!
It's heavily commented, I found that the situation was harder to reproduce than I thought, mainly because of the in-memory nature of the test, while I could clearly see the problem with a real cache throughout multiple requests.

Let me know if you need anything else!

[Ocramius]

👍

[guiwoda]

Guys, any plans on releasing with this PR and #1552?
They've been merged for forever and have never been tagged.

Thanks!

[Ocramius]

We're working on 2.6, but it's done when it's done.

[guiwoda]

@Ocramius I'm not asking for you guys to do extra work! I really appreciate everything you do for us mortal folks.
I was just pushing for this particular PRs to be tagged 😅

[Ocramius]

@guiwoda if it is in master, it will be on 2.6 - can you check that?

[guiwoda]

@Ocramius yup, they're both merged to master.

Yay! 🎉