diff --git a/CHANGELOG.md b/CHANGELOG.md index 23b437e..88ce32d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed +- **Asset field values are round-trip safe** (#41) — `get` returns an assets field the way Statamic stores it, as a container-relative path (`icons/heart.svg`), but both the validation rules and the fieldtype pipeline expect the form the Control Panel submits, the asset ID (`assets::icons/heart.svg`). Sending a value straight back to `create`/`update` therefore failed file rules such as `mimes` — `MimesRule` does an `Asset::find()` on the value and a bare path finds nothing — and on rule-free fields it would have broken later in `Assets::process()`, which calls `Asset::findOrFail()`. Incoming asset paths are now resolved to canonical IDs before validation, in nested replicator, grid, group and bard set fields as well as top-level ones, and in the entry's stored data that an update merges in — so an unchanged asset field elsewhere in the blueprint no longer fails an update that never touched it. Values that resolve to no asset are left alone so validation still reports the real problem instead of silently dropping content + +- **`content_validate` no longer flags every valid assets field** — The read-side sweep ran the blueprint's rules against stored values, which meant it reported up to three errors for a perfectly valid single-file assets field: "must be a file of type: svg" (the file rules resolve the value with `Asset::find()`, which a bare path misses), plus "must be an array" and "must not have more than 1 items" from the fieldtype's own rules, which expect a list. Asset references are now bridged to canonical IDs for the rule pass, through nested replicator, bard, grid and group fields as well as top-level ones. The structural pass still sees the stored values verbatim, so a `missing_asset` finding keeps quoting the reference as it appears on disk +- **`content_validate` resolves single-container asset fields** — An assets field that omits `container` where the site has exactly one was skipped by the missing-asset check; it now resolves the same way the fieldtype does + ## [2.9.0] - 2026-08-27 ### Added diff --git a/src/Mcp/Tools/Concerns/ResolvesAssetIds.php b/src/Mcp/Tools/Concerns/ResolvesAssetIds.php new file mode 100644 index 0000000..ae8cd91 --- /dev/null +++ b/src/Mcp/Tools/Concerns/ResolvesAssetIds.php @@ -0,0 +1,90 @@ + + */ + protected function normalizeAssetFieldValue(Field $field, mixed $value): array + { + $values = match (true) { + is_array($value) => array_values($value), + is_string($value) && $value !== '' => [$value], + default => [], + }; + + if ($values === [] || ($container = $this->assetFieldContainer($field)) === null) { + return $values; + } + + return array_map(fn (mixed $item): mixed => $this->resolveAssetId($item, $container), $values); + } + + /** + * Rewrite one stored reference to its canonical `container::path` ID. + */ + protected function resolveAssetId(mixed $reference, string $container): mixed + { + if (! is_string($reference) || $reference === '' || str_contains($reference, '::')) { + return $reference; + } + + $id = $container . '::' . ltrim($reference, '/'); + + return Asset::find($id) ? $id : $reference; + } + + /** + * Resolve the container an assets field points at, mirroring the fieldtype's + * own resolution: the configured container, or the only one that exists. + */ + protected function assetFieldContainer(Field $field): ?string + { + $configured = $field->get('container'); + + if (is_string($configured) && $configured !== '') { + return $configured; + } + + $containers = AssetContainer::all(); + + if ($containers->count() !== 1) { + return null; + } + + $only = $containers->first(); + + return $only instanceof AssetContainerContract ? $only->handle() : null; + } +} diff --git a/src/Mcp/Tools/Concerns/SanitizesFieldData.php b/src/Mcp/Tools/Concerns/SanitizesFieldData.php index 9bcb648..2c87f72 100644 --- a/src/Mcp/Tools/Concerns/SanitizesFieldData.php +++ b/src/Mcp/Tools/Concerns/SanitizesFieldData.php @@ -15,6 +15,8 @@ trait SanitizesFieldData { + use ResolvesAssetIds; + /** * Keys that are entry-level properties, not blueprint data fields. * @@ -117,7 +119,8 @@ private function sanitizeFieldValue(Field $field, mixed $value, bool $allowLegac 'grid' => $this->sanitizeGridValue($field, $value, $allowLegacyCoercion, $path), 'replicator' => $this->sanitizeReplicatorValue($field, $value, $allowLegacyCoercion, $path), 'table' => $this->sanitizeTableValue($value, $allowLegacyCoercion, $path), - 'terms', 'entries', 'users', 'assets', 'checkboxes' => $this->sanitizeRelationshipValue($value), + 'assets' => $this->normalizeAssetFieldValue($field, $value), + 'terms', 'entries', 'users', 'checkboxes' => $this->sanitizeRelationshipValue($value), default => $value, }; } diff --git a/src/Mcp/Tools/Concerns/ValidatesContentRecords.php b/src/Mcp/Tools/Concerns/ValidatesContentRecords.php index 2fc7558..ef72c9d 100644 --- a/src/Mcp/Tools/Concerns/ValidatesContentRecords.php +++ b/src/Mcp/Tools/Concerns/ValidatesContentRecords.php @@ -38,6 +38,8 @@ */ trait ValidatesContentRecords { + use ResolvesAssetIds; + /** Keys a replicator/bard/grid row carries that are not blueprint fields. */ private const SET_META_KEYS = ['id', 'type', 'enabled']; @@ -54,7 +56,10 @@ trait ValidatesContentRecords protected function validateRecord(Fields $fields, array $data, RecordRef $record): array { return [ - ...$this->ruleFindings($fields, $data, $record), + // The rule pass sees asset references bridged to the form the rules + // expect; the structural pass sees the stored values verbatim, so a + // finding still quotes what is actually on disk. + ...$this->ruleFindings($fields, $this->withResolvedAssetIds($fields, $data), $record), ...$this->structuralFindings($fields, $data, '', $record), ]; } @@ -376,11 +381,11 @@ private function optionFindings(Field $field, mixed $value, string $path, Record */ private function assetFindings(Field $field, mixed $value, string $path, RecordRef $record): array { - $container = $field->get('container'); + $container = $this->assetFieldContainer($field); // Without a container we cannot resolve the reference at all; that is a // blueprint problem, which statamic-blueprints validate already reports. - if (! is_string($container) || $container === '') { + if ($container === null) { return []; } @@ -408,6 +413,136 @@ private function assetFindings(Field $field, mixed $value, string $path, RecordR return $findings; } + /** + * Return a copy of the stored values with every assets reference rewritten + * to the canonical ID the validation rules expect. + * + * Statamic's validator recurses into nested fields on its own, but it does + * so over whatever values it was handed, so the bridge has to be applied to + * the whole tree up front. Nothing but assets values is touched. + * + * @param array $data + * + * @return array + */ + private function withResolvedAssetIds(Fields $fields, array $data): array + { + foreach ($fields->all() as $handle => $field) { + if (! is_string($handle) || ! $field instanceof Field || ! array_key_exists($handle, $data)) { + continue; + } + + $value = $data[$handle]; + + $data[$handle] = match ($field->type()) { + 'assets' => $this->normalizeAssetFieldValue($field, $value), + 'replicator' => $this->withResolvedAssetIdsInSets($field, $value), + 'bard' => $this->withResolvedAssetIdsInBard($field, $value), + 'grid' => $this->withResolvedAssetIdsInRows($field, $value), + 'group' => is_array($value) + ? $this->withResolvedAssetIds($this->nestedFields($field), $this->stringKeyed($value)) + : $value, + default => $value, + }; + } + + return $data; + } + + private function withResolvedAssetIdsInSets(Field $field, mixed $value): mixed + { + if (! is_array($value)) { + return $value; + } + + $sets = $this->flattenedSets($field); + + return array_map(function (mixed $block) use ($sets): mixed { + if (! is_array($block) || ! is_string($type = $block['type'] ?? null) || ! isset($sets[$type])) { + return $block; + } + + return $this->withResolvedAssetIds(new Fields($sets[$type]), $this->stringKeyed($block)); + }, $value); + } + + /** + * Bard keeps a set's own values under `attrs.values`; the rest of the tree + * is rich-text nodes with nothing to resolve. + */ + private function withResolvedAssetIdsInBard(Field $field, mixed $value): mixed + { + if (! is_array($value)) { + return $value; + } + + $sets = $this->flattenedSets($field); + + return array_map(function (mixed $node) use ($sets): mixed { + if (! is_array($node) || ($node['type'] ?? null) !== 'set') { + return $node; + } + + $attrs = $node['attrs'] ?? null; + $values = is_array($attrs) ? ($attrs['values'] ?? null) : null; + + if (! is_array($values) || ! is_string($type = $values['type'] ?? null) || ! isset($sets[$type])) { + return $node; + } + + $attrs['values'] = $this->withResolvedAssetIds(new Fields($sets[$type]), $this->stringKeyed($values)); + $node['attrs'] = $attrs; + + return $node; + }, $value); + } + + private function withResolvedAssetIdsInRows(Field $field, mixed $value): mixed + { + if (! is_array($value)) { + return $value; + } + + $fields = $this->nestedFields($field); + + return array_map( + fn (mixed $row): mixed => is_array($row) + ? $this->withResolvedAssetIds($fields, $this->stringKeyed($row)) + : $row, + $value + ); + } + + /** + * Resolve the child fields of a grid or group field. + */ + private function nestedFields(Field $field): Fields + { + $config = $field->get('fields'); + + return new Fields(is_array($config) ? array_values($config) : []); + } + + /** + * Re-key a nested block or row so it carries the handle-keyed shape the + * walk expects. YAML can hand back numeric-looking keys, which PHP casts + * to integers on the way in. + * + * @param array $value + * + * @return array + */ + private function stringKeyed(array $value): array + { + $keyed = []; + + foreach ($value as $key => $item) { + $keyed[(string) $key] = $item; + } + + return $keyed; + } + /** * Resolve a replicator/bard field's sets to [set handle => field configs]. * diff --git a/tests/Feature/Routers/ContentValidateTest.php b/tests/Feature/Routers/ContentValidateTest.php index 06f392a..fdf2fd6 100644 --- a/tests/Feature/Routers/ContentValidateTest.php +++ b/tests/Feature/Routers/ContentValidateTest.php @@ -192,6 +192,60 @@ public function test_accepts_numeric_option_keys_stored_as_strings(): void $this->assertCount(1, $this->findingsOfType($this->validate(), 'invalid_option')); } + public function test_does_not_report_stored_asset_paths_as_rule_violations(): void + { + config(['filesystems.disks.assets' => [ + 'driver' => 'local', + 'root' => storage_path('framework/testing/disks/assets'), + ]]); + Storage::fake('assets'); + Storage::disk('assets')->put('icons/heart.svg', ''); + + AssetContainer::make('media')->title('Media')->disk('assets')->save(); + + Blueprint::make('validated-assets') + ->setNamespace("collections.{$this->collectionHandle}") + ->setContents([ + 'title' => 'Validated Assets', + 'tabs' => ['main' => ['sections' => [['fields' => [ + ['handle' => 'title', 'field' => ['type' => 'text']], + ['handle' => 'icon', 'field' => [ + 'type' => 'assets', + 'container' => 'media', + 'max_files' => 1, + 'validate' => ['required', 'mimes:svg'], + ]], + ['handle' => 'blocks', 'field' => [ + 'type' => 'replicator', + 'sets' => ['content' => ['sets' => ['card' => ['fields' => [ + ['handle' => 'card_icon', 'field' => [ + 'type' => 'assets', + 'container' => 'media', + 'max_files' => 1, + 'validate' => ['required', 'mimes:svg'], + ]], + ]]]]], + ]], + ]]]]], + ]) + ->save(); + + Entry::make() + ->collection($this->collectionHandle) + ->slug('valid-icons') + ->blueprint('validated-assets') + ->data([ + 'title' => 'Valid Icons', + 'icon' => 'icons/heart.svg', + 'blocks' => [ + ['type' => 'card', 'id' => 'block-1', 'card_icon' => 'icons/heart.svg'], + ], + ]) + ->save(); + + $this->assertSame([], $this->findingsOfType($this->validate(), 'rule_violation')); + } + public function test_reports_asset_references_that_no_longer_resolve(): void { config(['filesystems.disks.assets' => [ diff --git a/tests/Feature/Routers/EntriesAssetRoundTripTest.php b/tests/Feature/Routers/EntriesAssetRoundTripTest.php new file mode 100644 index 0000000..57f0688 --- /dev/null +++ b/tests/Feature/Routers/EntriesAssetRoundTripTest.php @@ -0,0 +1,239 @@ +router = new EntriesRouter; + + config(['filesystems.disks.assets' => [ + 'driver' => 'local', + 'root' => storage_path('framework/testing/disks/assets'), + ]]); + + Storage::fake('assets'); + Storage::disk('assets')->put('icons/heart.svg', ''); + Storage::disk('assets')->put('icons/star.svg', ''); + + AssetContainer::make('assets')->title('Assets')->disk('assets')->save(); + + $this->collectionHandle = 'pages-' . bin2hex(random_bytes(4)); + Collection::make($this->collectionHandle)->title('Pages')->save(); + + Blueprint::make('pages')->setNamespace("collections.{$this->collectionHandle}")->setContents([ + 'fields' => [ + ['handle' => 'title', 'field' => ['type' => 'text']], + ['handle' => 'icon', 'field' => [ + 'type' => 'assets', + 'container' => 'assets', + 'max_files' => 1, + 'validate' => ['required', 'mimes:svg'], + ]], + ['handle' => 'gallery', 'field' => [ + 'type' => 'assets', + 'container' => 'assets', + 'validate' => ['mimes:svg'], + ]], + ['handle' => 'rows', 'field' => [ + 'type' => 'grid', + 'fields' => [ + ['handle' => 'row_icon', 'field' => [ + 'type' => 'assets', + 'container' => 'assets', + 'max_files' => 1, + 'validate' => ['required', 'mimes:svg'], + ]], + ], + ]], + ['handle' => 'story', 'field' => [ + 'type' => 'bard', + 'sets' => [ + 'main' => [ + 'sets' => [ + 'figure' => [ + 'fields' => [ + ['handle' => 'figure_icon', 'field' => [ + 'type' => 'assets', + 'container' => 'assets', + 'max_files' => 1, + 'validate' => ['required', 'mimes:svg'], + ]], + ], + ], + ], + ], + ], + ]], + ['handle' => 'page_builder', 'field' => [ + 'type' => 'replicator', + 'sets' => [ + 'main' => [ + 'sets' => [ + 'icon_cards' => [ + 'fields' => [ + ['handle' => 'card_icon', 'field' => [ + 'type' => 'assets', + 'container' => 'assets', + 'max_files' => 1, + 'validate' => ['required', 'mimes:svg'], + ]], + ], + ], + ], + ], + ], + ]], + ], + ])->save(); + + Stache::refresh(); + } + + private function makeEntry(string $id): void + { + Entry::make() + ->id($id) + ->collection($this->collectionHandle) + ->slug($id) + ->data(['title' => 'Home']) + ->save(); + } + + public function test_update_accepts_relative_asset_paths(): void + { + $this->makeEntry('home'); + + $result = $this->router->execute([ + 'action' => 'update', + 'collection' => $this->collectionHandle, + 'id' => 'home', + 'data' => [ + 'icon' => 'icons/heart.svg', + 'gallery' => ['icons/heart.svg', 'icons/star.svg'], + 'page_builder' => [ + ['id' => 'set-1', 'type' => 'icon_cards', 'enabled' => true, 'card_icon' => 'icons/heart.svg'], + ], + 'rows' => [ + ['id' => 'row-1', 'row_icon' => 'icons/star.svg'], + ], + 'story' => [ + [ + 'type' => 'set', + 'attrs' => [ + 'id' => 'node-1', + 'values' => ['type' => 'figure', 'figure_icon' => 'icons/star.svg'], + ], + ], + ], + ], + ]); + + $this->assertTrue($result['success'], json_encode($result['errors'] ?? [])); + + $entry = Entry::find('home'); + $this->assertNotNull($entry); + $this->assertSame('icons/heart.svg', $entry->get('icon')); + $this->assertSame(['icons/heart.svg', 'icons/star.svg'], $entry->get('gallery')); + $this->assertSame('icons/heart.svg', $entry->get('page_builder')[0]['card_icon']); + $this->assertSame('icons/star.svg', $entry->get('rows')[0]['row_icon']); + $this->assertSame('icons/star.svg', $entry->get('story')[0]['attrs']['values']['figure_icon']); + } + + public function test_update_still_accepts_canonical_asset_ids(): void + { + $this->makeEntry('canonical'); + + $result = $this->router->execute([ + 'action' => 'update', + 'collection' => $this->collectionHandle, + 'id' => 'canonical', + 'data' => ['icon' => 'assets::icons/heart.svg'], + ]); + + $this->assertTrue($result['success'], json_encode($result['errors'] ?? [])); + $this->assertSame('icons/heart.svg', Entry::find('canonical')?->get('icon')); + } + + public function test_create_accepts_relative_asset_paths(): void + { + $result = $this->router->execute([ + 'action' => 'create', + 'collection' => $this->collectionHandle, + 'data' => [ + 'title' => 'Created', + 'slug' => 'created', + 'icon' => 'icons/star.svg', + ], + ]); + + $this->assertTrue($result['success'], json_encode($result['errors'] ?? [])); + + $id = $result['data']['entry']['id']; + $this->assertSame('icons/star.svg', Entry::find($id)?->get('icon')); + } + + public function test_update_of_an_unrelated_field_does_not_trip_stored_asset_paths(): void + { + $this->makeEntry('stored'); + + Entry::find('stored')?->merge([ + 'icon' => 'icons/heart.svg', + 'page_builder' => [ + ['id' => 'set-1', 'type' => 'icon_cards', 'enabled' => true, 'card_icon' => 'icons/star.svg'], + ], + ])->save(); + + $result = $this->router->execute([ + 'action' => 'update', + 'collection' => $this->collectionHandle, + 'id' => 'stored', + 'data' => ['title' => 'Renamed'], + ]); + + $this->assertTrue($result['success'], json_encode($result['errors'] ?? [])); + + $entry = Entry::find('stored'); + $this->assertSame('Renamed', $entry?->get('title')); + $this->assertSame('icons/heart.svg', $entry?->get('icon')); + } + + public function test_unresolvable_asset_path_still_fails_validation(): void + { + $this->makeEntry('missing'); + + $result = $this->router->execute([ + 'action' => 'update', + 'collection' => $this->collectionHandle, + 'id' => 'missing', + 'data' => ['icon' => 'icons/does-not-exist.svg'], + ]); + + $this->assertFalse($result['success']); + $this->assertStringContainsString('Icon', $result['errors'][0]); + } +}