[6.x] Ensure set preview images are updated when assets are renamed/replaced/deleted#13013
[6.x] Ensure set preview images are updated when assets are renamed/replaced/deleted#13013duncanmcclean wants to merge 29 commits into
Conversation
Because the facade calls cause issues with mocks in existing tests. Doing it this way avoids (as many) facade calls.
jasonvarga
left a comment
There was a problem hiding this comment.
Unless I've done something silly, this doesn't seem to work at all. I can't get it to rename anything.
Plus, I think it might not work on a second level if you've defined statamic.assets.set_preview_images.folder as the path won't get saved in the blueprint - just the filename. We might need to change that.
When assets are deleted, the reference updater is called, which subsequently attempts to find the configured container for set preview images. However, the default container, `assets` doesn't exist, but if we don't mock it, the tests fail.
jasonvarga
left a comment
There was a problem hiding this comment.
This review was generated via a Claude-powered code review.
Solid, well-tested PR overall. The asset-listener flow is straightforward and the test coverage is comprehensive for the happy paths. A few real bugs and notes below.
Warning — Container is not filtered for blueprint/fieldset updates
src/Listeners/UpdateAssetReferences.php:117-135 calls the updater without filterByContainer($container), and src/Assets/AssetReferenceUpdater.php:76-134 (updateBlueprintFields) never checks the asset's container against Sets::previewImageConfig()['container']. The folder string is the only thing being matched.
Consequence: if statamic.assets.set_preview_images.container is previews but an asset is renamed in some other container (say documents) where the path happens to start with the same folder/, the blueprint's set.image (which logically points at the previews container) will be rewritten — corrupting the reference.
Suggested fix in updateBlueprintFields:
if (
! ($config = Sets::previewImageConfig())
|| $this->container !== $config['container']
|| ! Str::startsWith($this->originalValue, $config['folder'].'/')
) {
return;
}…and have the listener call ->filterByContainer($container) before ->updateReferences(...) on the Blueprint::all() / Fieldset::all() loops, mirroring the entry loop above.
A regression test where two containers share the same folder/filename would catch this.
Warning — Blueprint::all() misses several real namespaces
src/Fields/BlueprintRepository.php:27-37 hardcodes 'navigation', 'assets', 'globals', 'forms' plus collections/taxonomies/additional namespaces, but does not include:
useranduser_group(root-level, used atsrc/Auth/UserRepository.php:66andsrc/Auth/UserGroupRepository.php:19)- anything stored in the root via
Blueprint::in('')
User blueprints can contain bard/replicator fields, so a preview image inside a user blueprint won't be updated. Worth either including the user/user_group blueprints explicitly, or iterating every file under directory() rather than enumerating namespaces.
The hardcoded list is also brittle for the future — any new core blueprint namespace will silently be missed by every reference updater.
Note — setParent(null) in makeBlueprintFromFile is unrelated and undocumented
src/Fields/BlueprintRepository.php:350 adds ->setParent(null) to every blueprint coming out of makeBlueprintFromFile. There's no test exercising it and the PR description doesn't mention it. It only runs when BLINK_FROM_FILE misses (so it doesn't clobber a parent set on a later find against the same path), but it will clobber any parent set during construction by something else that holds the same instance.
If this was needed because Blueprint::all() iterations were producing parent-leaked instances, that motivation should be either in a code comment or split into its own commit so the reasoning is recoverable.
Note — Performance: full blueprint/fieldset enumeration on every asset save
UpdateAssetReferences::replaceReferences now walks Blueprint::all() and Fieldset::all() on every save/replace/delete, regardless of whether statamic.assets.set_preview_images is configured. The early-return inside updateBlueprintFields saves the inner work, but instantiating every blueprint/fieldset still pays disk I/O.
A cheap up-front check at the top of replaceReferences would skip both loops entirely when the feature is off:
if (Sets::previewImageConfig()) {
Blueprint::all()->each(...);
Fieldset::all()->each(...);
}(Combined with the container fix above, this can also short-circuit on container mismatch.)
Note — findFieldsInBlueprintContents matches anywhere type is a string
src/Data/DataReferenceUpdater.php:102-116 walks all nested arrays and triggers on any 'type' => 'bard' / 'type' => 'replicator' key/value pair. It doesn't constrain to "this is actually a fieldtype declaration." In practice, blueprint contents are unlikely to have these strings in validate/if/etc., and the downstream if (! isset($fieldContents['sets'])) skip makes false matches harmless. But it's worth a comment noting the heuristic — or restricting to keys living under field to make the intent obvious.
Note — Old set schema (no group wrapper) is silently skipped
updateBlueprintFields assumes the modern grouped structure (sets.<group>.sets.<setHandle>). A blueprint authored with the older flat schema (sets.<setHandle> directly, no group) hits the ! isset($setGroup['sets']) branch and returns untouched, even if it has an image. If 6.x still loads old-format blueprints from disk this is a gap; if not, ignore.
Tests look good
The cases in tests/Listeners/UpdateAssetReferencesTest.php cover rename, move-out-of-folder, delete, no-config, and the wrong-folder negative case. Two cases worth adding given the findings above:
- Two containers: configure
set_preview_images.container = 'A', place a same-named asset in containerB, rename it, assert no blueprint changes. (Would catch the missing-container-filter bug.) - User blueprint with a bard set image. (Would catch the
Blueprint::all()namespace gap.)
Misc
src/Fields/FieldsetRepository.php:176, 187— refreshing the$fieldsetscache on save/delete is a nice correctness fix; useful independently of this PR.Blueprint::all()is a generally useful new public API; might want a short PHPDoc explaining the namespace ordering since callers (and theit_gets_all_blueprintstest) rely on it.
The blueprint/fieldset reference updater now correctly filters by container to prevent corrupting references when an asset in a different container happens to have the same folder path as the configured set_preview_images folder. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
These root-level blueprints were missing from the enumeration, which meant asset reference updates for set preview images wouldn't be applied to user blueprints containing bard/replicator fields. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…gured Avoids unnecessary disk I/O by short-circuiting the Blueprint::all() and Fieldset::all() loops when set_preview_images isn't configured or when the asset is in a different container. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Documents the reason for clearing the parent when returning blueprints from the blink cache: prevents stale parent references when iterating via Blueprint::all() after previous find() calls. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Documents that the method uses a simple pattern match that could theoretically match non-field structures, but is safe in practice because downstream code guards against false matches. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
I've pushed some changes. Let me know if you need anything else. |
This pull request ensures that Set Preview Images on Bard & Replicator fields are updated when assets are renamed/replaced/deleted, like we do with content.
I've also added
Blueprint::all()which returns blueprints from all collections, taxonomies, etc.Closes #12680