fix(entries): accept stored asset paths on write (#41) - #42
Merged
Conversation
Statamic stores an assets field as a container-relative path, which is what `get` returns, but the validation rules and the fieldtype pipeline both expect the asset ID the CP submits. Sending a value back unchanged failed `mimes` — MimesRule does an Asset::find() on the value — and on a rule-free field would have blown up later in Assets::process(), which calls Asset::findOrFail(). Resolve incoming asset paths to canonical `container::path` IDs in the shared sanitizer, so it covers nested replicator/grid/group/bard fields and the stored data an update merges in as well as top-level fields, and applies to entries, terms and globals alike. Unresolvable values are left untouched so validation reports the real problem rather than silently dropping content.
Extends the #41 fix to the read-side sweep, which had the same root cause: content_validate ran the blueprint's rules against stored values, so every valid single-file assets field produced up to three false errors — the file rules resolve the value with Asset::find(), which a bare path misses, and the fieldtype's own array/max rules expect a list rather than the string Statamic stores. Move the container and ID resolution into a shared ResolvesAssetIds concern, and give the rule pass a normalized copy of the record that recurses through replicator, bard, grid and group. The structural pass keeps the raw values, so a missing_asset finding still quotes what is on disk. assetFindings now resolves single-container fields too, matching the fieldtype.
5 tasks
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
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.
Fixes #41.
The bug
statamic-entries getreturns an assets field the way Statamic stores it — a container-relative path,icons/heart.svg. Everything downstream of a Control Panel form submission expects the other form: the canonicalcontainer::pathasset ID, in a list.So a value round-tripped from
getintoupdatefails:Statamic\Fieldtypes\Assets\MimesRule::passes()doesAsset::find($id), and a bare path finds nothing → "must be a file of type: svg"Assets::process(), which callsAsset::findOrFail()It also hit fields the caller never touched:
updatevalidates the incoming data merged with the entry's stored data, and stored data is all paths, so any other assets field carryingrequired+mimesfailed an unrelated update.The Control Panel bridges this gap in
Assets::preProcess()when it loads the form. Anything handing stored values to the validator without that step has to bridge it itself.Second bug, same root cause
Double-checking the fix turned up
content_validate, which sweeps content already on disk and so hands the validator stored values directly. It reported three false errors for every valid single-file assets field:The last two come from the fieldtype's own
array/max:1rules, which expect a list rather than the bare string Statamic stores.The fix
Container and ID resolution live in a new
ResolvesAssetIdsconcern, used from both sides:SanitizesFieldData, so entries, terms and globals alike) — incoming asset paths are resolved before validation, covering nested replicator, bard, grid and group fields as well as top-level ones, and the stored data an update merges in.ValidatesContentRecords) — the rule pass gets a normalized copy of the record. The structural pass keeps the raw values, so amissing_assetfinding still quotes the reference exactly as it appears on disk.Container resolution mirrors the fieldtype's own: the configured container, or the only one that exists. That last part also fixes
assetFindings, which previously skipped single-container fields entirely. References that resolve to no asset are passed through untouched, so validation reports the real problem instead of silently dropping content. Canonical IDs continue to work unchanged.Tests
tests/Feature/Routers/EntriesAssetRoundTripTest.php— 5 cases covering top-level, replicator, grid and bard-set asset fields on bothcreateandupdate, an unrelated update that must not trip stored paths, plus regression guards (canonical IDs still accepted, missing asset still rejected).ContentValidateTest::test_does_not_report_stored_asset_paths_as_rule_violations— top-level and nested.Verified against
main: 4 of these fail there, all pass here. Full gate green — pint, PHPStan level 9 (no ignores, no baseline), 1106 tests / 5609 assertions, license check.