[Bug] Fixes pimcore/platform-version#467: Key the element reference cache by type as well as id - #2032
Conversation
ReferenceResolver caches resolved references by id alone. Documents, assets
and data objects have independent id sequences, so a document and an asset can
share one id; whichever is resolved first fills the cache and every later
element with that id gets its payload back, whatever type it is.
On an element carrying both an asset property and a document property whose
targets share an id, GET /properties/{elementType}/{id} therefore returns the
same reference for both, and the wrong reference is written back when the
properties are saved. PropertyHydrator and MetadataHydrator both resolve
through this service.
The lookup itself is correct - Property::getData() goes through
Element\Service::getElementById() with the type - so only the cache key has to
carry the type.
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
There was a problem hiding this comment.
🟡 Changes recommended
The root-cause fix is sound, but lacks a regression test covering same-ID references across element types.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes cross-type reference cache collisions by keying entries with the element class and ID.
Changes:
- Uses class-plus-ID cache keys at
ReferenceResolver.php:43,58. - Correctly covers both direct and
resolveData()callers without API changes. - A focused regression test is still missing.
File summaries
| File | Description |
|---|---|
src/Resolver/Element/ReferenceResolver.php |
Prevents assets, documents, and data objects sharing IDs from colliding in the cache. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| { | ||
| if (isset($this->cache[$element->getId()])) { | ||
| return $this->cache[$element->getId()]; | ||
| $cacheKey = $element::class . '_' . $element->getId(); |
There was a problem hiding this comment.
Not adding a unit test here. The change is a cache-key correction with no behaviour of its own to assert beyond the collision itself, and the fix was verified against a real installation
nobodys-tools
changed the title
Fixes pimcore/platform-version#467
ReferenceResolver caches resolved references by id alone. Documents, assets and data objects have independent id sequences, so a document and an asset can share one id; whichever is resolved first fills the cache and every later element with that id gets its payload back, whatever type it is.
On an element carrying both an asset property and a document property whose targets share an id, GET /properties/{elementType}/{id} therefore returns the same reference for both, and the wrong reference is written back when the properties are saved. PropertyHydrator and MetadataHydrator both resolve through this service.
The lookup itself is correct - Property::getData() goes through Element\Service::getElementById() with the type - so only the cache key has to carry the type.
Expected Behavior
Each property resolves against its own element type.
Actual Behavior
Both properties return the SAME element, the one resolved first.
In the UI the wrong reference is displayed.
The stored data is correct throughout, the Properties table keeps
type=document/data=4 and the frontend resolves it correctly.
Only Studio's representation in Properties table is wrong, and saving from that state writes the wrong value back.
Steps to reproduce
On a fresh install this is common - e.g. document 4 and asset 4 both exist.
propAsset - type Asset -> the asset with id 4
propDoc - type Document -> the document with id 4
Note that "propAsset" sorts before "propDoc".