don't move assets when propagating

[i-just]

Description

Assets are being saved to the wrong folder when uploading to a field with 'Restrict assets to a single location' and {site.handle} inside the Asset Location. Able to reproduce on 3.7.67 and 4.3.10.

Don’t move assets if we’re propagating element.

Related issues

#12767

[brandonkelly]

@i-just There might be a case for relocating assets if localizeRelations is true. Thoughts?

[i-just]

Ok, going back to the drawing board:

At the moment (without this PR):

• if “Restrict assets to a single folder” is checked and “Asset location” is set to {site.handle}, the file will get moved to the last site we’re propagating to
• if the “asset location” changes to {site.handle}/test, the asset will get moved to the “test” subfolder
• let’s say the file location is now site3/test; if you have “manage relations on a per-site basis” not checked and you then check this setting, then go to, e.g. site2 entry and upload another file, the old file will stay put, but the new one will get moved to the last site we’re propagating to

With the first version of this PR:

• the file won’t get moved on propagation, but if you go to the site2 entry and upload another file, the old file will get moved to the site2 folder too
• if the “asset location” changes to {site.handle}/test, you go to site3 entry and save; the files will get moved to site3/test
• if you have “manage relations on a per-site basis” not checked and you then check this setting, then go to, e.g. site2 entry and save, the files will move to site2/test

So, the plan here, as I understand, is to have the file not move at all regardless of which site’s entry you’re saving (if the file is already uploaded), not move if we’re propagating, and only move if it’s freshly uploaded or if the “Asset location” has changed?

[brandonkelly]

I think the location setting should be enforced for existing assets as well; otherwise changes to the location setting would never apply to all existing assets.

And that’s exactly why I’m thinking maybe we should still make location changes during propagation, only when localizeRelations is enabled.

[i-just]

@brandonkelly I think I might be struggling with understanding what the end result is supposed to be.

For Craft 3: I discarded my previous change and then tried the following change https://github.com/craftcms/cms/blob/v3/src/fields/Assets.php#L581:

$assetsToMove = ArrayHelper::where($assets, function(Asset $asset) use ($targetFolderId) {
    return $asset->folderId != $targetFolderId;
});

To this:

$assetsToMove = ArrayHelper::where($assets, function(Asset $asset) use ($targetFolderId, $element) {
    return $asset->folderId != $targetFolderId && (!$element->propagating || $this->localizeRelations);
});

With the above, an asset is no longer being moved to the last site we’re propagating to.

It all works fine when localizeRelations is true.

But when singleUploadLocationSubpath is set to {site.handle} and localizeRelations is false:

• if you upload an asset to an entry on site1 - all good
• if you then go to site2 entry and save it, the asset gets moved to the site2 directory
• if you edit the asset field and change singleUploadLocationSubpath from, e.g. {site.handle} to {site.handle}/test, go to the entry on site1, the asset that was previously in a site2 directory, gets moved to site1/test directory.

I think the location setting should be enforced for existing assets as well; otherwise changes to the location setting would never apply to all existing assets.

Do you mean the above behaviour is what you’re after, or am I missing something here?

[brandonkelly]

@i-just This is what I was thinking, with that last comment: https://github.com/craftcms/cms/pull/12777/commits/5d15912874a7bf7fe67c19dcc121d9886cfa78ba

Basically continue doing our relocation logic when propagating, but only for Assets fields that have “Manage relations on a per-site basis” enabled.

Look good to you?

[i-just]

Yeah, I see what you mean; I think maybe I got hung up on trying to fix something that isn’t actually a problem.

We have an assets field with useSingleFolder on, singleUploadLocationSubpath set to {site.handle} and localizeRelations off.

With your latest commit:

• On a new Entry in Site A, upload an image -> save the Entry
• Asset will be saved to the siteA folder all is good here

So my last question is:

• Switch to Site B, upload another asset
• All assets will be moved to the siteB folder Is this the desired behaviour?

[brandonkelly]

• Switch to Site B, upload another asset

• All assets will be moved to the siteB folder Is this the desired behaviour?

Yes, I think so. And if you don’t like that (and really don’t want per-site relations), you could have the subpath set to a specific site handle, based on all the sites the entry exists for, in a particular preferSites order.

That could be done by changing the subpath template to an include like:

{% include '_asset_site_subpath.twig' with {element: object} %}

And then from the include template:

{#- output a supported site handle in order of preference,
    or use the element’s existing site handle as a fallback #}
{%- set preferred = element.find()
   .id(element.id)
   .drafts(null)
   .provisionalDrafts(null)
   .status(null)
   .site('*')
   .preferSites(['siteA', 'siteB', '...'])
   .one() %}
{{- preferred.site.handle ?? element.site.handle -}}

(Those -s won’t be needed as of the next Craft 3 and 4 releases, via 7af51b9072403dc8ef1b70c87994bbc18f009745.)