feat(tools): reference an existing file from a content element - #865
Conversation
github-actions
Bot
added
documentation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #865 +/- ##
============================================
- Coverage 86.51% 85.61% -0.90%
- Complexity 9957 10034 +77
============================================
Files 575 576 +1
Lines 32177 32513 +336
============================================
Hits 27837 27837
- Misses 4340 4676 +336
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Review record — no Copilot review exists (monthly, account-wide quota), so the green gate is not a read of this change. This is a writing tool touching FAL and the DataHandler, so the record is longer than usual. The finding I would most want a reviewer to check, because it overturns my own design. The tool wrote through the DataHandler twice, justified by a probe showing a single pass leaving the counter short and the reference sorted first. Disabling the second pass changed no test. The probe had run without Authorisation, which is the part worth being paranoid about. Two independent gates against the acting user, both verified by controls that remove them:
Both refuse in the same words as "does not exist", so a refusal never confirms a uid — asserted by a test that runs an absent file and an unreachable one and compares the strings. The silent-drop class, which is where the sibling writer was bitten. Discarding is a rollback. Deleting the row alone leaves the element's counter naming a reference that is gone — the exact inconsistency this tool exists to avoid. A test caught it. Six controls, each observed (read-back texts, counter restore, extension check, ambiguity branch, storage gate, page permission). Worth naming: an earlier run of this harness reported two as undetected, and both were failed Unreachable branch, made reachable in the test rather than left as a claim. No core CType offers two of Counting sweep: 47 → 48 tools and six → seven writers, across Gates run locally at 8.2 including Rector, plus the full unit suite. Not merging on this record — I hold no merge authorisation for this PR. Assisted by claude-code:claude-opus-5 — Session |
|
Self-review: 6c7d198 The review this pull request demands is unsatisfiable (Copilot quota wall or repeated bot failures on this head). Per the documented fallback, the diff on this head was reviewed by the PR author; this comment is the on-the-record attestation the merge gate reads back. It stops matching on the next push. |
6c7d198 to
5dbcab1
Compare
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
|
Self-review: 5dbcab1 The review this pull request demands is unsatisfiable (Copilot quota wall or repeated bot failures on this head). Per the documented fallback, the diff on this head was reviewed by the PR author; this comment is the on-the-record attestation the merge gate reads back. It stops matching on the next push. |
The seventh purpose-built writer, and the first that creates a sys_file_reference. Demo content with images stopped at the upload: nothing could make an uploaded file part of the site, because the files group is read-only by design and the only FAL write was set_file_alternative_text on a metadata row that already existed. It appends one existing sys_file to a content element's image, assets or media field through the DataHandler as the acting backend user, in the live workspace and the default language. No upload, no move, no rename. Both ends are authorised against that user: the element's page needs content-edit rights, and the file has to lie in a permitted storage inside their own file mounts. Either failure is refused in the same words as an element that does not exist, so a refusal never confirms a uid. Three refusals the issue did not name, each measured against the live TCA rather than assumed: The field must accept the file. Each file field declares its own extension list and they differ sharply — image takes fourteen, assets twenty-seven, media anything. A .docx on image is a relation the FormEngine would reject, so it is refused rather than written. sys_language_uid has to be in the reference payload. Without it a non-admin's write dies inside checkRecordEditAccess() on a missing languageField. Stated rather than defaulted, and it pins the tool to the default language like the other FAL writer. title, alternative and description are exclude fields, so the DataHandler drops them for a user without the grant — silently, with an empty errorLog. The read-back checks them as well as the relation, or the tool would report a caption it did not write. Discarding is a rollback, not a delete. When the read-back fails the reference row is removed AND the element's counter is written back to the list it had before the call; removing only the row leaves exactly the inconsistent state this tool exists to avoid. There is no `position` argument. The write appends by construction. A correction to my own design, caught by a control rather than by review: it wrote through the DataHandler twice, because a probe had shown a single pass leaving the counter one short and the new reference sorting first. That probe ran without $GLOBALS['LANG'] — an environment this tool refuses to run in. Disabling the second pass changed no test, which is how the artefact surfaced. One pass now, and the docblock says what was measured instead of what I remembered. Functional rather than unit, and not by preference: the interesting behaviour is what ends up in sys_file_reference AND in the element's own counter, and the silent-drop defect is invisible to a mock. Six controls, each observed: removing the text read-back, the counter restore, the extension check, the ambiguity branch, the storage gate or the page permission each fails a test. No core CType offers two of the three fields — textpic and image show image, textmedia shows assets, nothing shows two — so the ambiguity branch is unreachable with core content types. The test registers its own two-field type rather than leaving that branch an untested claim. Every counting surface moves with it: 47 tools to 48 and six writers to seven, across Tools.rst, README, seven landing-page data files in both languages, and the four tests that pin those numbers. Closes #834 Assisted-by: claude-code:claude-opus-5 Agent-Session: https://claude.ai/code/session_01MNg1MysJVugv1xo2husknU Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
5dbcab1 to
f0b47ad
Compare
|
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
|
Self-review: f0b47ad The review this pull request demands is unsatisfiable (Copilot quota wall or repeated bot failures on this head). Per the documented fallback, the diff on this head was reviewed by the PR author; this comment is the on-the-record attestation the merge gate reads back. It stops matching on the next push. |
Closes #834. The seventh purpose-built writer, and the first that creates a
sys_file_reference. Demo content with images stopped at the upload: nothing could make an uploaded file part of the site, because thefilesgroup is read-only by design (ADR-047) and the only FAL write wasset_file_alternative_texton a metadata row that already existed.A control caught my own design, and the design lost
The tool wrote through the DataHandler twice, and I had a measurement to justify it: a probe showed a single-pass write leaving the element's counter one short and the new reference sorting first. Both issue comments say so.
Then I disabled the second pass and every test still passed.
The probe had run without
$GLOBALS['LANG']. In that environment the defect is real; in a complete backend environment — the only one this tool will run in, becauserefuseWithoutBackendEnvironment()checks — a single datamap carrying the parent list and theNEW_placeholder produces the right count and the rightsorting_foreign. One pass now,substNEWwithIDsonly to identify the row for the read-back, and the docblock says what was measured rather than what I remembered.The issue comments are corrected rather than left standing.
Three refusals the description did not name
Each came from the live TCA, not from reasoning about it:
imagetakes fourteen extensions,assetstwenty-seven,mediaanything. A.docxonimageis a relation the FormEngine would reject, so it is refused rather than written.sys_language_uidmust be in the payload.checkRecordEditAccess()on a missinglanguageField. Stated rather than defaulted, which also pins the tool to the default language like the other FAL writer.title,alternative,descriptionareexcludefields.errorLog. Without checking them the tool reports a caption it did not write.That last one is also what made the read-back testable: remove the grant and the guard fires.
Discarding is a rollback, not a delete
When the read-back fails, removing the reference row is not enough — the element's counter still names it, which is exactly the inconsistent state this tool exists to avoid producing.
discard()now writes the parent field back to the list that was there before the call. A test caught that too.Six controls, each observed
discard()stops restoring the counterRestored tree: 13 tests green. An earlier run of this same harness reported two of these as undetected; both were failed
sedanchors, so the test had run against an unmodified file. The harness now compares the checksum before and after and says "anchor missed" instead of "undetected" — a control that cannot fail is worse than none.One measured fact worth recording
No CType that ships with the core offers two of
image/assets/media.textpicandimageshowimage,textmediashowsassets, nothing shows two. The "several fields → name the one you mean" branch is therefore unreachable with core content types, so the test registers a two-field type of its own rather than leaving the branch an untested claim.There is no
positionargument: the write appends by construction, and an argument nothing reads is worse than none.Verification
-s functional -d sqlite(this class)-s functional(tool surfaces)-s unit(full)-s phpstan-s cgl -n-s rector -n(pinned 8.2, run locally)composer ci:test:changelogEvery counting surface moves with it: 47 tools → 48, six writers → seven, across
Tools.rst,README.md, seven landing-page data files in both languages, and the four tests that pin those numbers.Assisted by claude-code:claude-opus-5 — Session