[Documents] Do not send link editables as internal without an element… - #4087
[Documents] Do not send link editables as internal without an element…#4087codingioanniskrikos wants to merge 5 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
🟡 Changes recommended
Internal links can still be emitted without the required internal type.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates document-link serialization to safely downgrade stale internal links to direct links.
Changes:
- Requires an internal ID before serializing links as internal.
- Emits explicit
nullvalues and preserves the stale path. - Changes malformed internal links to
linktype: 'direct'.
Review assessment:
- Root cause is only partially addressed:
internalTyperemains unchecked at line 93. - Serialization is handled at the correct boundary with compatible type widening.
- The centralized API transformation covers known callers.
- No focused regression test was added; documentation changes are unnecessary.
File summaries
| File | Description |
|---|---|
assets/js/src/core/modules/element/dynamic-types/definitions/document/editable/types/dynamic-type-document-editable-link.tsx |
Adjusts document-link API serialization for stale internal targets. |
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.
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
🟡 Changes recommended
The behavioral fix lacks a regression test covering the reported failure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
@codingioanniskrikos
Hi! Thanks for this - the diagnosis is spot on, and the guard sits in the right place. It
matches the invariant the rest of the feature already assumes: use-link-data-type.ts:57
gates openLink on exactly the same two fields, and link-value-converter.ts:53-54 always
sets internal and internalType together. Sending internalType: null instead of
undefined is the load-bearing part — with the key present-but-null,
Link.php:306 (setDataFromEditmode) short-circuits cleanly and the backend's own
normalisation runs instead of reading a missing key.
Three things before this can go in:
-
Base branch. The linked issue reports Affected Version 2026.2, but this targets
2026.x. Bugfixes land on the active release line and forward-merge up, so please
retarget to2026.2— otherwise 2026.2 users don't get the fix. -
Regression test.
transformValueForApiis a pure function and there are jest tests
throughout this tree already (e.g.
document/editable/components/renderlet-editable/renderlet-content.test.tsx), so this is
cheap. Four cases: valid internal stays internal; internal + id but nointernalType→
direct; internal +internalTypebut no id → direct; direct unchanged. Please also
assert'internalType' in result— theundefinedkey being dropped byJSON.stringify
is the exact mechanism of this bug, and a plain deep-equal won't catch a regression. -
Changelog.
DocumentLinkEditableValueis re-exported through the public SDK
(js/src/sdk/modules/element/index.ts:169), so wideninginternalTypetostring | null
is a public type change — external bundles assigning it tostring | undefinedwill now
fail to compile. The widening is the right call (narrowing it is what caused the bug), it
just shouldn't land silently.
Thanks in advance! :)
… an internal link
transformValueForApi is pure, so the four relevant shapes are cheap to pin down: a fully resolved internal link stays internal, an internal link missing either the id or the type is downgraded to a direct one, and a direct link is left alone. Each downgrade case asserts 'internalType' in result rather than comparing the object as a whole. An undefined internalType is dropped by JSON.stringify and never reaches the backend, which is the mechanism of the bug being fixed here; a deep-equal assertion would not catch a regression back to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ation DocumentLinkEditableValue is re-exported through the public SDK entry point (js/src/sdk/modules/element/index.ts), so widening internalType from string | undefined to string | null | undefined is a public type change. External bundles that assign the property to a string | undefined variable will no longer compile against it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fc6eb8a to
f42ff12
Compare
The two existing downgrade cases each drop only one of the two fields, so neither reproduces the shape reported in pimcore#459: a link whose target document was deleted keeps linktype: 'internal' but has lost both the id and the type. Assert that shape directly - internal false, internalId and internalType null, linktype 'direct', and the path it used to resolve to preserved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Resolves pimcore/platform-version#459
A link editable was sent to the backend as
internal: trueeven when no element was behind it, andinternalTypewas sent asundefined, whichJSON.stringifydrops. This happens with a link whose target document was deleted: it still haslinktype: 'internal', but no id and no type left.The backend then trusts that flag and reads
internalType/internalIdwithout checking them, which breaks rendering and saving of the whole document.Now
internalis only claimed when an id is present. Otherwise the value is sent as a direct link, andinternalTypeis sent asnullinstead ofundefined.