Using partially synchronized image fields causes validation errors and php warnings

This has not moved forward for almost a year. Is this still being pursued? This issue is blocking sites from translating content. How can this not be a top priority by now? I would love to fix it myself, but the convoluted Drupal code base takes at least a year to wrap your head around.

I'd be happy to apply a patch as a quick workaround, but am unable to identify the current state of patches proposed here.

Can anyone point me to the most current patch that I can apply?
Thanks.

instead of patch #16, try the MR diff here:
https://git.drupalcode.org/project/drupal/-/merge_requests/1373.diff

@AmdersTwo, that is a normal message, non-translateable fields can only be changed when updating the original language.
I suggest you configure the original language to be the site default to make a consistent UI for your content editors. If you want to get fancy for simple content types try the entity_translation_unified_form module which allows you to edit two or more languages on one node form. There is also a side by side mode available https://drupal.org/project/entity_translation_unified_form

Please review.

tried fixing CCF.

Removing credit for #28 #29 as they failed CC checks.
Also there was no interdiff and no explanation that was changed.
If just a reroll per issue credit guidelines → rerolls don't receive credit either.
Also was previously tagged for tests which weren't added.

Added the tests based on previous contributors work, they seem reasonable and pass on local. I used the simplest approach to solve the issue like @aleix commented on #20 🐛 Using partially synchronized image fields causes validation errors and php warnings Needs work .

I've been using the patch with https://www.drupal.org/project/media_library_edit → module in order to make content editors workflow as easy as possible for translating image (media) entities when editing node content.

Based on https://git.drupalcode.org/project/drupal/-/merge_requests/1373/ with fix for the remaining failing test.

Interesting, CI uses different PHPStan configurations than local. Adjusted accordingly, interdiff shows the fix.

The last submitted patch, 33: 3218426-based-on-1373-MR-33.patch, failed testing. View results →

Random test failure on #33, so setting back to Needs review.

Can the IS be updated please with the proposed solution.

Did my best to update the issue summary.

Here is a different approach. The reason this happens is that when the values of an image item are set as an array, we lose the already set width and height. Instead of recalculating them, the attached patch keeps the existing width and height if it's the same file as already set.

We could do both things as well, just in case, but this is enough to get both the new UI test and my added kernel tests for those specific scenarios to pass.

FWIW, I would love to get rid of those two properties long-term. They really should be on the file entity, there's so much duplication and extra calculation happening with the current scenario. For example, image medias have the thumbnail and the actual image field and even though it points to the same file entity, we calculate width and height twice. The file_entity module has file data storage that's similar to user data, but we could also just add two base fields to file entities in image module I think. The upgrade path to get rid of those properties is not going to be fun though.

Also note that I disagree with phpstan here. Accessing properties through magic __get() is OK and actually more efficient, because we don't need to initialize property objects then. To get rid of those warnings, we could add @property annotations, but that seems out of scope here.

Even though it's out of scope should a follow up be opened and this postponed?

For the CC failure.

@Berdir - Your patch #38 appears to have changes to "core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php" that are not needed.
The attached patch removes the changes to EntityReferenceItem.

Created a new merge request based on the latest MR but targetted to 11.x https://git.drupalcode.org/project/drupal/-/merge_requests/5385

Applying @Berdir's patch now.

I don't understand the cspell failures.

The current MR doesn't fix the issue where the error Non-translatable field elements can only be changed when updating the original language appears if non-translatable fields are hidden and an image field's file property is unchecked, but the alt and/or title properties are checked.

The new conditions in ImageItem::setValue() aren't triggered since the width/height seems to be changing, even though the file reference didn't change. $this->width and $this->height are NULL, $values['width'] and $values['height'] are set to the image's dimensions.

A possible fix could be to create a new ImageFieldItemList for image fields, overriding hasAffectingChanges to not return TRUE if only the width/height changed (since that method 'determines whether the field has relevant changes'). I'll add that to the current MR.

Rebased the MR and resolved the conflict where the phpstan-baseline.neon changed to .phpstan-baseline.php.

Setting back to Needs Work as there are failing tests.

Patch #40 couldn't be applied on Drupal 10.2.5.

I tested the latest MR, and I'm attaching the file I used to test. It works as expected and solves the warnings.

recrit → changed the visibility of the branch 3218426-using-content-moderation to hidden.

Some cleanup - All MRs and patches should be based on 11.x. For that reason, I hid MR 1373 and all patches on this ticket so that the focus can be on MR 5385.

MR 5385:
- Based on 11.x.
- has the changes from the original patch.
- has the setValue approach from patch #40 🐛 Using partially synchronized image fields causes validation errors and php warnings Needs work created by @Berdir. This approach keeps the values if they already exist so that the width and height does not need re-calculated, see #38 🐛 Using partially synchronized image fields causes validation errors and php warnings Needs work for more details.
- has a new ImageFieldItemList::hasAffectingChanges() added by @DieterHolvoet , see #45 🐛 Using partially synchronized image fields causes validation errors and php warnings Needs work for more details.

Failing Test for MR 5385: core/modules/image/tests/src/Kernel/ImageItemTest.php:137

    1) Drupal\Tests\image\Kernel\ImageItemTest::testImageItem
    Failed asserting that null matches expected 88.
    
    /builds/issue/drupal-3218426/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
    /builds/issue/drupal-3218426/core/modules/image/tests/src/Kernel/ImageItemTest.php:137
    /builds/issue/drupal-3218426/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Failing Test for MR 5385: core/modules/image/tests/src/Kernel/ImageItemTest.php:137

    1) Drupal\Tests\image\Kernel\ImageItemTest::testImageItem
    Failed asserting that null matches expected 88.
    
    /builds/issue/drupal-3218426/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
    /builds/issue/drupal-3218426/core/modules/image/tests/src/Kernel/ImageItemTest.php:137
    /builds/issue/drupal-3218426/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

This test explicitly sets the width to NULL before re-saving the image. The width does not get calculated in the new ImageItem::onChange() method since it does nothing when the changed property is either "width" or "height" allowing others to set the values. The original ImageItem code would calculate the width and height in ImageItem::preSave() if either the width or height is missing.

Updates completed:
- Created a method to ensure that the width and height are set - ImageItem::ensureImageDimensions().
- Used the new method in ImageItem::onChange() and ImageItem::preSave().

The attached patch can be used for composer builds.

Based on issue summary not super clear what the problem is. Its best to mention the issue vs putting a link for reviewer to track down.

But left a few nitpicky comments on the MR.

Test coverage certainly there.

@smustgrave All MR comments have been resolved and the Issue Summary has been updated.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

@smustgrave I have rebased the branch with 11.x. The new test "ImageOnTranslatedEntityTest::testSyncedImagesWithTranslatablePropertiesAndContentModeration" is now failing for a very odd issue - the root user 1 cannot access "fr/node/1/edit". This was not an issue prior to the rebase so my thinking is something changed in 11.x. The root user should not be denied, so it is very odd. I've tried a few code changes to the tests, but none have worked.

See https://www.drupal.org/node/2910500 → . Don't use root user.

@Berdir thank you! that makes sense now. I'll switch it to add permissions to the test user.

MR rebased to 11.x, tests have been updated to not use the root user.

Believe all feedback for this one has been addressed.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

"Needs Review Queue Bot" rejection - this was caused by 11.x updating the ImageItem plugin definition to use PHP attributes instead of annotations.
I'm working on rebasing to fix the conflicts.

successfully rebased with the latest 11.x, moving back to RTBC

OK, it's a bit difficult to follow what's going on here; apologies if any of my questions on the MR are answered elsewhere on the issue.

If we're copying a bunch of code from `equals()`, shouldn't we be factoring out some helper methods? (In a BC way, of course.)

Left a number of nitpicks and suggestions on the new test.

Is the test coverage exposed anywhere? (Failing test without the fix.)

Thanks everyone!

pradhumanjain2311 → made their first commit to this issue’s fork.

@catch, @larowlan, and I discussed the added return typehinting for the subclass and decided to keep it, as per 🌱 [Meta] Implement strict typing in existing code Active . Thanks!

xjm → credited catch → .

xjm → credited larowlan → .

@pradhumanjain2311, ideally my suggestions would be reviewed by a contributor to the issue for whether they're correct, rather than accepted without review by someone who's not involved with the issue. I won't be adding credit for that. Thanks!

@DieterHolvoet You had added a "ImageFieldItemList.php" list class to fix an issue that you mentioned in #45 🐛 Using partially synchronized image fields causes validation errors and php warnings Needs work .
There is test coverage for the scenario that you mentioned in `ImageOnTranslatedEntityTest::testSyncedImagesWithTranslatableProperties` and `ImageOnTranslatedEntityTest::testSyncedImagesWithTranslatablePropertiesAndContentModeration`.
If I set the list handler back to `FileFieldItemList`, then the tests still pass.

Please test with setting the list handler back to `FileFieldItemList` to see if the current updates fix your issue without the need for a new list class.
If a new list class is still needed, then we will need test coverage for the issue that you see.

I re-rolled patch #49 for Drupal 10.3.x and it's available in the MR 8619 above.

Same error on drupal 10.2.7 but i can't apply patch MR 8619 or MR 5385
Thank you

MR 8619 patch attached that applies on 10.3.

Renaming to patch :)

Attached are static patches that can be used for composer builds.

• Patch for 11.x only: drupal-11.x-MR-5385--20240827-1.patch
• Patch for 10.3.x, 11.x: drupal-11.x-10.3.x-MR-5385--without-phpstan-changes--20240827-1.patch - This is the MR 5385 for 11.x without the phpstan changes so that it can be applied to 10.3.x also.

In general, dev work should be done on the MR 5385 for the latest 11.x branch since this incorporates all the discussion thus far in this issue. Once approved, the changes would be back ported to any older branches.

Tests were passing, now failing.

I'm using the patch for 10.3 from number #80 and it's working for me in that the error goes away. I tried re-running the tests again to see if it was a random fail as it didn't seem related but I got more failures. E.g.

Drupal\Tests\migrate\Functional\MigrateNoMigrateDrupalTest::testExecutionNoMigrateDrupal
Behat\Mink\Exception\ResponseTextException: The text "Migration was
successful." was not found anywhere in the text of the current page.

Is failing because of this:

Drupal\Component\Plugin\Exception\PluginException: Plugin (d6_field_formatter_settings) instance class "Drupal\migrate_drupal\Plugin\migrate\FieldMigration" does not exist. in Drupal\Component\Plugin\Factory\DefaultFactory::getPluginClass() (line 97 of core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php). 

At least that one doesn't seem related to this change. Not sure about the others.