Make draft element resolution pluggable - #1991
Draft
markus-moser wants to merge 3 commits into
Draft
Conversation
Seven services independently answered "which state should this user see — what is published, or their unsaved edits?" with the same two inlined lines: look up the newest unpublished version for the user, then swap in its data. The policy was therefore decided seven times over and could only ever have one answer. Move it behind DraftElementResolverInterface, with the existing behaviour as the default implementation. Nothing changes for Pimcore itself; what changes is that a bundle storing pending edits somewhere other than a version can now decorate one service and have every read path follow, instead of each having to be taught separately — or silently keeping the old answer and rendering values the user has already changed. The version lookup stays in ElementProviderTrait for the two detail paths that also report `draftData`: that is the draft IDENTITY the UI labels and deletes by, not the element's state, and the two are allowed to diverge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
markus-moser
force-pushed
the
feat/draft-element-resolver
branch
from
August 13, 2026 08:22
eb3e341 to
fe5326c
Compare
Review follow-up to the extraction. resolve() now answers a ResolvedDraft carrying both halves: the element STATE to render, and the Version that is the draft IDENTITY reported as `draftData`. The two detail paths were asking for those separately, which meant two uncached `SELECT id FROM versions` + `Version::getById()` round trips per element open, and a window in which an autosave landing between the two calls would have `draftData` name a different row than the one rendered. With the identity coming back from the same call, ElementProviderTrait ::getLatestVersionForUser() has no callers left; deleting it also removes the second copy of the "which types carry versions" gate, leaving the resolver as the only place that decides. The gate in UpdateService::getDraftElement() deliberately stays and is now documented. It is narrower than the resolver's on purpose: `useDraftData` arrives on an unvalidated payload array, so delegating assets there would let an asset update swap the live asset for its latest version before saving. Also guards the default resolver against Version::getData() answering null — a pruned payload, or an __PHP_Incomplete_Class after a class rename — which the non-nullable return type would otherwise turn into a 500 on element open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cut the multi-line reasoning prose from the draft-resolver additions (ResolvedDraft, the resolver interface + its default, the UpdateService asset-exclusion note, and the test double) down to single-line constraints. No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Behaviour-preserving refactor. Pimcore's own answer is unchanged and stays the default.
Package A, PR 1 of 2.
The problem
Seven services independently answer "which state should this user see — what is published, or the edits they have not saved yet?", each with the same two inlined lines:
UpdateService,DataObjectService,LayoutService,DocumentService,PropertyService,WorkflowDetailsService,Search\Hydrator\Preview\DataObjectHydrator.The policy is therefore decided seven times over and can only ever have one answer: a draft is a version row. A bundle that stores pending edits anywhere else has to patch each response individually — and every endpoint nobody thought about keeps rendering published values.
The change
Move it behind
DraftElementResolverInterface, with today's behaviour asVersionDraftElementResolver. The seven call sites inject and delegate.Deliberately minimal: no registry, no discriminator, no knowledge of who might replace it. It is an ordinary decoratable service, like
UpdateServiceInterfacealready is — so a bundle decorates one thing and every read path follows.The version lookup stays in
ElementProviderTraitfor the two detail paths that also reportdraftData. That is the draft identity the UI labels and deletes by, not the element's state; the two coincide by default and are allowed to diverge.Verification
614tests (up from 608),1312assertions (up from 1303)VersionDraftElementResolverTest— 6 tests covering version-applied, no-draft passthrough, user scoping, null user, unversioned types skipped, and assets/page-snippetsNote the resolver's own test needed real subclasses rather than mocks:
getLatestVersion()is not declared on the models at all — it lives on the Dao and is reached viaAbstractModel::__call, so a mocked__callsilently answers null and such a test passes while asserting nothing.Three existing test fixtures gained the new constructor argument.
Not in this PR
draftDataremains version-shaped. Making the identity pluggable is a separate, larger conversation.