fix(content-system): scope the layout write memo to the write that opened it - #20675
Martin Krzykawski (MartinKrzykawski) wants to merge 2 commits into
Conversation
…ened it LayoutWriteContext rides the caller-owned Context, which outlives any single write, and pairs commands to trees positionally. Nothing tied an entry to the write that produced it and nothing cleared one when a write failed between normalize() and the validation event, so a later write to the same row appended its tree behind the stale one and consume() handed out the stale one first. The consequence is not cosmetic: the gate judged a tree the write did not carry. A write carrying an unresolvable layout was accepted because the leftover tree was resolvable, storing a row the served-implies-resolvable invariant says cannot exist. The memo now belongs to its write. WriteContext is minted per repository call and both sides can reach it — the serializer through WriteParameterBag, the validator through PreWriteValidationEvent::getWriteContext() — so that instance identifies the write. A memo from an earlier write is replaced by the serializer and counts as absent to the validator. ContentLayoutWriteMemoLifetimeTest drives a write that dies during normalize, then a second write of an unresolvable tree on the same Context. Verified by falsification: with both ownership checks removed the second write is accepted and the unresolvable layout is stored.
|
OpenAPI Snapshot
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/experience-studio #20675 +/- ##
=========================================================
Coverage ? 69.66%
=========================================================
Files ? 6085
Lines ? 210639
Branches ? 9652
=========================================================
Hits ? 146737
Misses ? 61084
Partials ? 2818
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The integration test only covers the serializer side: once the serializer
replaces a foreign memo, the gate always sees its own, so removing the gate's
ownedBy() left every test green. Two unit tests close that — a layout command
gated against an earlier write's memo now fails with layoutWriteMemoMissing,
and the skip path leaves that memo untouched instead of draining it. Both fail
with the check removed.
Also folded in while here: the memo helper becomes an instance method, so the
file no longer mixes self:: and $this-> on adjacent lines; the createDataStack()
paragraph names the failure the ownership check actually produces there (a loud
layoutWriteMemoMissing from the cloned WriteContext, not a silent mispairing);
and the serializer's inline comment gives way to a {@see} on the method
docblock, where it does not restate LayoutWriteContext's own prose.
00ec654 to
ec2cbef
Compare
1. Why is this change necessary?
Layout/LayoutWriteContextcarries the already-decoded layout tree from the field serializer to the write gate. It rides the caller-ownedContext, which outlives any single write, and pairs commands to trees positionally: a key holds every tree remembered under it, andconsume()hands out the oldest.Nothing tied an entry to the write that produced it, and nothing cleared one when a write failed between
normalize()andPreWriteValidationEvent. So a later write to the same row appended its tree behind the leftover and the gate consumed the leftover instead.The consequence is not a memory note — it is a gating hole. The gate judged a tree the write did not carry, so a write carrying an unresolvable layout was accepted because the leftover happened to be resolvable, storing a row that served-implies-resolvable says cannot exist.
Item 8 of #19954.
2. What does this change do, exactly?
The memo now belongs to its write.
WriteContextis minted once per repository call and both sides already reach it — the serializer throughWriteParameterBag::getContext(), the gate throughPreWriteValidationEvent::getWriteContext()— so that instance identifies the write.LayoutWriteContexttakes its owningWriteContextand exposesownedBy()(identity comparison).StoredElementListFieldSerializer::memoize()replaces a memo owned by an earlier write instead of appending to it.ContentLayoutWriteValidatortreats a memo owned by another write as absent.Thirteen lines of logic across three files; fewer is not possible, because the writer and the reader have to agree about the same object. The accumulation ceiling drops from the
Context's whole lifetime to one write's rows.This does not make the memo empty immediately after a failed write — the entries are dropped when the next write replaces the memo. They are unreachable from that moment on, which is what the gating guarantee needs; the class docblock states it that way rather than claiming more.
3. Describe each step to reproduce the issue or behaviour.
On
feat/experience-studio, reusing oneContextfor both writes:create()twocontent_layoutrows in one call, where the second row carries'layout' => 'not-a-tree'. Row one is memoized, row two throws insidenormalize(), the write aborts andPreWriteValidationEventnever fires — row one's tree stays on theContext.upsert()row one again on the sameContext, this time with an element whose type is unresolvable against the layout's root source.ContentLayoutWriteMemoLifetimeTest::testWriteAfterAFailedWriteIsJudgedAgainstItsOwnTreedrives exactly that.4. Please link to the relevant issues (if any).
relates #19954
5. Checklist
feat/experience-studio;LayoutWriteContextis@internalFalsification detail: both ownership checks are covered, and each by the test that actually reaches it.
ContentLayoutWriteMemoLifetimeTest::testWriteAfterAFailedWriteIsJudgedAgainstItsOwnTreefail — the second write is accepted and the unresolvable layout is stored.ContentLayoutWriteValidatorTest::testForeignOwnedMemoIsNotConsumedandtestForeignOwnedMemoIsNotDrainedUnderTheSkipStatefail.The second pair was added after review: the integration test alone left the gate's check green when removed, because the serializer had already replaced the foreign memo by the time the gate ran.