Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.x]: fields/merge asset fields merges selected assets #15869

Closed
jannisborgers opened this issue Oct 9, 2024 · 6 comments
Closed

[5.x]: fields/merge asset fields merges selected assets #15869

jannisborgers opened this issue Oct 9, 2024 · 6 comments
Assignees
Labels

Comments

@jannisborgers
Copy link

What happened?

Description

When running fields/auto-merge and merging two asset fields which are part of the same entry-type, their content relations are merged into the persisting field.

I have an entry type doubleImageSection (former matrix block) which had fields called leftImage and rightImage. I took a screenshot of the fields/auto-merge output as a todo list for later:

Example

Before the field merge, the doubleImageSection entry type was used in a matrix field and the two fields had one image each:

After running the merge command, I ended up with the persisting leftImage field holding both of the images:

Max-relations ignored

Note that I had max relations set to 1 for both original fields, a setting which persisted as expected, so validation fails and saving the entry is not possible without manually fixing the issue.

Project config

The result of the merge command looks okay:

Migration file rightImage_into_leftImage:

<?php

namespace craft\contentmigrations;

use Craft;
use craft\migrations\BaseFieldMergeMigration;

/**
 * m241009_074950_merge_rightImage_into_leftImage migration.
 */
class m241009_074950_merge_rightImage_into_leftImage extends BaseFieldMergeMigration
{
    public string $persistingFieldUid = '7eb8fc2d-f108-4d1a-a08d-eb1c8d7ad2ba';
    public string $outgoingFieldUid = 'ad9df33a-32b2-4550-a98c-278ed9479ca4';
}

Config file for doubleImageSection entry type:

           -
             dateAdded: '2024-10-08T09:09:41+00:00'
             elementCondition: null
             fieldUid: 7eb8fc2d-f108-4d1a-a08d-eb1c8d7ad2ba
             handle: null
             includeInCards: false
             instructions: null
             label: 'Linkes Bild'
             providesThumbs: true
             required: true
             tip: null
             type: craft\fieldlayoutelements\CustomField
             uid: 6336c364-3cf8-4589-9958-a558712d9629
             userCondition: null
             warning: null
             width: 100
          -
             dateAdded: '2024-10-08T09:09:41+00:00'
             elementCondition: null
-            fieldUid: ad9df33a-32b2-4550-a98c-278ed9479ca4
-            handle: null
+            fieldUid: 7eb8fc2d-f108-4d1a-a08d-eb1c8d7ad2ba
+            handle: rightImage
             includeInCards: false
-            instructions: null
+            instructions: 'Fügen Sie hier das rechte Bild ein.'
             label: 'Rechtes Bild'
             providesThumbs: false
             required: true
             tip: null
             type: craft\fieldlayoutelements\CustomField
             uid: 6a889e12-40df-4a8a-b8e2-0ba377dadf28
             userCondition: null
             warning: null
             width: 100

Steps to reproduce

I rolled back to before the merge and can confirm that this happened with the auto-merge command, not with the Craft 4 to 5 upgrade itself:

  1. Add entry type
  2. Add asset fields leftImage and rightImage, max relations set to 1
  3. Use the entry type in a matrix field and select one asset for each field
  4. Merge the fields, persist leftImage

Expected behavior

  • Entry type with two instances of the leftImage asset field, but one with field handle override rightImage
  • Entry type instance has same content as before: One asset in each field instance

Actual behavior

  • Correct: Entry type with two instances of the leftImage asset field, but one with field handle override rightImage
  • Incorrect: Entry type instance has two assets in leftImage, no asset in rightImage

Possible explanation

Could the source be that the field handle override of the non-persisting field is ignored when running the content migration, and instead the content migration merges the content of both fields into the field based on the original field handle (now leftImage for both field instances)?

Craft CMS version

5.4.6

PHP version

8.2.24

Operating system and version

Darwin 23.3.0

Database type and version

MySQL 8.0.27

Image driver and version

Imagick 3.7.0 (ImageMagick 7.1.1-38)

Installed plugins and versions

AsyncQueue 4.0.0
Contact Form 3.1.0
Contact Form Extensions 5.0.0
Contact Form Honeypot 2.1.0
Craft Commerce 5.1.3
Dumper 5.0.1
Knock Knock 3.0.1
PayPal Checkout for Craft Commerce 3.0.1
Redactor 4.2.0
Vite 5.0.1

@williamhibberd
Copy link

Experiencing the same thing here

@brandonkelly brandonkelly self-assigned this Oct 9, 2024
@brandonkelly
Copy link
Member

I’m able to reproduce this, and looking into how we can guard against it, but for now: the way to work around this is to run the appropriate resave/* command(s) before merging your relation fields, so existing relations get saved into the content JSON for each source element.

For example if both of the to-be-merged relation fields are used by a foo entry type, you should run:

php craft resave/entries --type=foo

(This should be done on all environments, before the merge/before the changes get deployed to them.)

brandonkelly added a commit that referenced this issue Oct 10, 2024
@brandonkelly
Copy link
Member

Made some changes for Craft 5.5 which will help avoid this issue: (571c620)

  • Added a resave/all command, which calls each of the other resave/* commands.
  • All native resave/* commands (including all) now support a --with-fields argument, which accept a list of global field handles. Any elements that have a field layout where any of those fields are present will be resaved.
  • The fields/merge and fields/auto-merge commands now detect whether you’re merging relation fields, and if so, prompt to resave elements which use those fields before merging them. After the merge, it will also output a warning that resave/all --with-fields=handleA,handleB should be run on other environments before the change is deployed to them.

@mihob
Copy link

mihob commented Oct 28, 2024

@brandonkelly Is it sufficient to run resave/entries with a current version of Craft 4 before updating?
I would like to prepare a complete update to Craft 5 (including field merges).

@brandonkelly
Copy link
Member

@mihob Yep, just make sure to run it on all environments.

@brandonkelly
Copy link
Member

Craft 5.5.0 is out with those changes 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants