[Data Objects] Report inheritable flag and inherited value per field in inheritance data - #2033
Merged
Merged
Conversation
…in inheritance data Adds two additive properties to InheritanceData (pimcore/platform-version#448): - `inheritable`: false only for field types that cannot take part in inheritance (supportsInheritance() === false) or have no data adapter, so a client can tell an overridden value from a non-inheritable field. - `inheritedValue`: the value the field inherits, or would inherit if its own value were removed, from the nearest ancestor holding a non-empty value, normalized to the same shape as objectData. Null when no ancestor holds one, the field is not inheritable, or resolution wasn't requested. Resolving it costs a walk up the tree for every field holding an own value, so it is opt-in. The request travels through the recursion as FieldContextData::shouldResolveInheritedValue() (constructor argument `resolveInheritedValue`, default false), which the localized fields, object brick and classification store adapters pass on to their child contexts and getContextObjectFromElement() preserves for ancestors. InheritanceServiceInterface::getInheritanceData() gained `bool $resolveInheritedValues = false`; DataService turns it on for the data object detail response. The grid column resolvers keep the default and no longer pay for the extra walk; their `inheritance` reports `inheritedValue: null`. InheritanceServiceInterface also gains getFieldInheritanceData(), which returns the complete InheritanceData for a single field. The objectData value normalization moves from DataService into DetailValueTrait so InheritanceService can reuse it without a circular service dependency; DataService keeps its public methods as delegates. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Split a line exceeding 120 characters in ClassificationStoreAdapter and extract the nested ternary in InheritanceService::getFieldInheritanceData into independent statements for readability. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Classification-store eligibility and nested ancestor traversal can produce incorrect inheritance metadata.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Verdict: Needs changes. The PR adds inheritance eligibility and optional normalized ancestor values to data-object field metadata.
Changes:
- Extends
InheritanceDatawithinheritableandinheritedValue. - Centralizes detail-value normalization and propagates opt-in resolution through nested fields.
- Adds unit coverage and upgrade documentation.
Review contract:
- Claim: Expose field inheritance capability and inherited values.
- Root cause: Addressed at the service/DTO boundary, but the leaf helper omits eligibility checks (
InheritanceService.php:103-119). - Call sites: Most are covered; classification-store keys can be misreported (
ClassificationStoreAdapter.php:175). - Boundary: Service and adapter layers are appropriate.
- Compatibility: Changes are additive and internal; the new argument defaults to existing behavior.
- Tests: Scalar cases are covered, but nested missing-ancestor traversal and direct-helper eligibility are not.
- Docs: Adapter and upgrade documentation are updated.
- Risk: Missing intermediate object bricks can prevent inherited-value discovery (
InheritanceService.php:135).
File summaries
| File | Description |
|---|---|
tests/Unit/DataObject/Service/InheritanceServiceTest.php |
Adds inheritance-service tests. |
src/DataObject/Util/Trait/DetailValueTrait.php |
Centralizes value normalization. |
src/DataObject/Service/InheritanceServiceInterface.php |
Extends the inheritance API. |
src/DataObject/Service/InheritanceService.php |
Resolves eligibility, origins, and inherited values. |
src/DataObject/Service/DataService.php |
Enables inherited values for details. |
src/DataObject/Data/Model/InheritanceOrigin.php |
Adds an origin value object. |
src/DataObject/Data/Model/InheritanceData.php |
Adds new metadata fields. |
src/DataObject/Data/Model/FieldContextData.php |
Carries the resolution opt-in. |
src/DataObject/Data/Adapter/ObjectBricksAdapter.php |
Propagates the opt-in to brick fields. |
src/DataObject/Data/Adapter/LocalizedFieldsAdapter.php |
Propagates it to localized fields. |
src/DataObject/Data/Adapter/ClassificationStoreAdapter.php |
Resolves metadata for store keys. |
doc/03_Extending/06_Data_Objects/01_Field_Definition_Adapters.md |
Documents adapter integration. |
doc/02_Installation_and_Configuration/05_Upgrade.md |
Adds the upgrade note. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Empty-chain behavior regresses, and unresolved nested-field cases can return incorrect inheritance metadata.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
…by Copilot review getFieldInheritanceData() is called directly by ClassificationStoreAdapter for classification-store key definitions, bypassing the adapter/supportsInheritance eligibility guard that processFieldDefinition() applies. Field types without an adapter (e.g. calculatedValue keys) could therefore be reported as inheritable and have an inherited value resolved for them. Apply the same guard directly in the leaf helper. The ancestor walk also lost track of an object brick once an intermediate ancestor did not have that brick added: FieldContextData::getContextObjectFromElement() resolved the container to null and, having no locator left, could neither retry the same container on a further ancestor nor tell the caller that this was a missing container rather than no container at all - which made getValidFieldValue() fall back to reading an unrelated, same-named root field. FieldContextData now keeps the container field name (and object brick type) independently of the currently resolved value, so the walk can reacquire the same container from a grandparent, and exposes isContainerContextUnresolved() so InheritanceService treats a missing intermediate container as empty instead of reading the wrong field. Adds regression coverage for both cases. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The refactored origin traversal regresses existing metadata for all-empty ancestor chains.
Review details
Suppressed comments (1)
src/DataObject/Service/InheritanceService.php:179
- The nullable origin changes existing behavior for an all-empty inheritance chain. Previously
getOriginId()recursed to the terminal ancestor and returned that ancestor's ID; nowfindOrigin()propagatesnull, so this fallback returns the originally requested object's ID. Consequently the existingobjectId/inheritedpair changes from the top ancestor/trueto the child/false, despite the additive-compatibility claim. Please preserve the terminal-ancestor fallback for metadata while using a separate nullable result to represent “no ancestor value,” and cover a multi-level all-empty chain in the regression test.
return $this->findOrigin($object, $fieldDefinition, $key, $contextData)?->getObject()->getId()
?? $object->getId();
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…empty chain The previous getOriginId() recursion returned the terminal ancestor's id when neither the object nor any ancestor held a value for a field, and the PR's upgrade note promises objectId/inherited keep that meaning. The refactor to InheritanceService::findOrigin() introduced in this branch instead returned null in that case, so getFieldInheritanceData() fell back to the starting object's own id and reported the field as not inherited - a behavior change Copilot review caught. findOrigin() now always resolves to an InheritanceOrigin, falling back to the terminal ancestor (with its empty value) instead of null. findParentOrigin(), which only backs the actual resolved inherited value and has no such legacy fallback to preserve, keeps returning null when no ancestor holds a value. getInheritedValue() now also treats an origin with an empty value as "nothing to inherit" so this terminal fallback can never be surfaced as inheritedValue. Adds regression coverage for the whole-chain-empty case on both getFieldInheritanceData()/processFieldDefinition() and getOriginId(). Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Localized inherited values can expose unauthorized languages, and unsupported grid fields do not receive the promised metadata.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/DataObject/Service/InheritanceService.php:85
- The new
inheritable: falseresult is never emitted for ordinary grid fields whosesupportsInheritance()is false:Grid/Column/Resolver/DataObject/AdapterResolver.php:116-118skipsprocessFieldDefinition()and leavesinheritanceasnull. Thus aurlSluggrid column does not carry the two advertised properties and still cannot be distinguished from missing inheritance metadata. Route grid fields through this method whenever class inheritance is enabled, and add coverage for a non-inheritable grid field.
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
…nguages LocalizedFieldsAdapter::getFieldInheritance() built its per-language result over every language configured on the class (ToolResolverInterface::getValidLanguages()), regardless of the current user's language-view permission. When resolveInheritedValue was opted in, this meant a restricted user could receive an ancestor's actual localized value for a language they have no view permission for, even though the same object's regular field data is filtered through LanguageServiceInterface::getUserAllowedLanguages() for exactly that reason (see resolveLocalizedData() a few lines below). Found by Copilot review. Only the value-carrying path now uses the permission-filtered language list; the inheritable/inherited flags without resolveInheritedValue keep covering every configured language, since no value crosses the wire in that case. Adds regression coverage for both the restricted and the metadata-only case. Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Grid call sites can omit or misreport the newly advertised inheritable flag.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/DataObject/Data/Model/InheritanceData.php:32
- This default makes the existing fallback at
Grid/Column/Resolver/DataObject/ClassificationStoreResolver.php:107-110reportinheritable: truewithout examining the key definition. Classification stores supportcalculatedValuekeys (ClassificationStoreAdapter.php:509-510), which this PR explicitly treats as non-inheritable, so an absent/inactive calculated key can emit incorrect metadata. Update that fallback to derive the flag through the field definition/service rather than relying on this default.
src/DataObject/Service/InheritanceService.php:85 - Top-level grid columns never reach this new
inheritable: falsebranch:Grid/Column/Resolver/DataObject/AdapterResolver.php:116-117only calls the service whensupportsInheritance()is already true. Consequently aurlSluggrid column still returnsinheritance: null, so clients cannot obtain the advertised per-column flag. Remove that caller-side guard (while retaining the class-level inheritance check) and let this service produce the non-inheritable metadata.
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes in this pull request
Resolves pimcore/platform-version#448
Every
inheritanceData.metaDataentry of the data object detail response (and theinheritanceof a grid column) now carries two additional properties next toobjectIdandinherited:inheritable(bool): whether the field type can take part in inheritance at all.falsefor field types whosesupportsInheritance()returnsfalse(e.g.urlSlug,calculatedValue,fieldcollections) and for field types without a Studio data adapter, so a client can tell an overridden value (inherited: false, inheritable: true) apart from a field that can never inherit (inheritable: false).inheritedValue(mixed): the value the field inherits — or would inherit if its own value were removed — from the nearest ancestor that holds a non-empty value, normalized to the same shape asobjectData. Resolving it costs a walk up the tree for every field holding an own value, so it is opt-in:nullwhen no ancestor holds a value, when the field is not inheritable, or when it was not requested. The data object detail response requests it; grid columns do not and always reportnull.Implementation:
FieldContextData::shouldResolveInheritedValue()(constructor argumentresolveInheritedValue, defaultfalse), which the localized fields, object brick and classification store adapters pass on to their child contexts andgetContextObjectFromElement()preserves for ancestors.InheritanceServiceInterface::getInheritanceData()gainedbool $resolveInheritedValues = false;DataServiceturns it on for the data object detail response.InheritanceServiceInterfacegainsgetFieldInheritanceData(), returning the completeInheritanceDatafor a single field; the origin walk is shared so an inherited value costs no additional tree walk.DataServiceintoDetailValueTraitsoInheritanceServicecan reuse it without a circular service dependency;DataServicekeeps its public methods as delegates.InheritanceServiceTest.Additional info
Verified locally:
InheritanceServiceTest(15 cases): all pass.Unitsuite: 38 unrelated pre-existing errors (Pimcore\Model\Version\CoauthorContextnot found inPatcher/Updatertests) — caused by this worktree's vendor copy being pinned topimcore/pimcore v2026.2.5, stale relative to2026.x, not by this change.Not verified: end-to-end against a running Studio instance.
🤖 Generated with Claude Code