Using partially synchronized image fields causes validation errors and php warnings

Created on 11 June 2021, almost 4 years ago
Updated 3 June 2023, almost 2 years ago

Problem/Motivation

This seems to be a regression of ๐Ÿ› Multiple image upload breaks image dimensions Fixed .

Steps to reproduce:

A translatable and moderated node type with an image field, that has the image field set to do property synchronization of the file/fid property (which internally includes width and title).

Trying to save a translation then results in "Non-translatable field elements can only be changed when updating the original language." and there is also a php notice: Message Notice: Undefined index: width in Drupal\content_translation\Plugin\Validation\Constraint\ContentTranslationSynchronizedFieldsConstraintValidator->hasSynchronizedPropertyChanges() (line 153...

Apparently width/height aren't set anymore in $items. I didn't yet investigate why that happens exactly, doesn't quite make sense to me yet, as it is a defined property, it should always be set?

Reverting ๐Ÿ› Multiple image upload breaks image dimensions Fixed fixes the problem.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

๐Ÿ› Bug report
Status

Needs work

Version

11.0 ๐Ÿ”ฅ

Component
Image systemย  โ†’

Last updated 5 days ago

Created by

๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 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.

  • ๐Ÿ‡จ๐Ÿ‡ฆCanada joseph.olstad

    @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

  • Status changed to Needs review almost 2 years ago
  • last update almost 2 years ago
    Custom Commands Failed
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia Ajeet Tiwari

    Please review.

  • last update almost 2 years ago
    Custom Commands Failed
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia Ajeet Tiwari

    tried fixing CCF.

  • Status changed to Needs work over 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    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.

  • Status changed to Needs review over 1 year ago
  • last update over 1 year ago
    Build Successful
  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland sokru

    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.

  • last update over 1 year ago
    Build Successful
  • Open in Jenkins โ†’ Open on Drupal.org โ†’
    Environment: PHP 8.1 & MySQL 5.7 updated deps
    last update over 1 year ago
    Custom Commands Failed
  • last update over 1 year ago
    Custom Commands Failed
  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland sokru

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

  • last update over 1 year ago
    29,820 pass, 2 fail
  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland sokru

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

  • last update over 1 year ago
    29,824 pass
  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland sokru

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

  • Status changed to Needs work over 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Can the IS be updated please with the proposed solution.

  • Status changed to Needs review over 1 year ago
  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland sokru

    Did my best to update the issue summary.

  • last update over 1 year ago
    Custom Commands Failed
  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland

    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.

  • Status changed to Needs work over 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

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

    For the CC failure.

  • last update over 1 year ago
    Custom Commands Failed
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit

    @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.

  • last update over 1 year ago
    Custom Commands Failed
  • last update over 1 year ago
    29,201 pass, 27 fail
  • First commit to issue fork.
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium borisson_ Mechelen, ๐Ÿ‡ง๐Ÿ‡ช

    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.

  • Pipeline finished with Failed
    over 1 year ago
    Total: 161s
    #49291
  • Pipeline finished with Failed
    over 1 year ago
    Total: 164s
    #49314
  • Pipeline finished with Failed
    over 1 year ago
    Total: 154s
    #49319
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium borisson_ Mechelen, ๐Ÿ‡ง๐Ÿ‡ช

    I don't understand the cspell failures.

  • Pipeline finished with Failed
    over 1 year ago
    Total: 143s
    #49346
  • Pipeline finished with Failed
    over 1 year ago
    Total: 244s
    #49349
  • Pipeline finished with Failed
    over 1 year ago
    Total: 868s
    #49365
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium dieterholvoet Brussels

    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.

  • Pipeline finished with Failed
    about 1 year ago
    Total: 166s
    #85997
  • Pipeline finished with Failed
    about 1 year ago
    Total: 164s
    #86006
  • First commit to issue fork.
  • Pipeline finished with Failed
    12 months ago
    Total: 494s
    #140648
  • Status changed to Needs review 12 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium tim-diels Belgium ๐Ÿ‡ง๐Ÿ‡ช

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

  • Pipeline finished with Failed
    12 months ago
    Total: 667s
    #140664
  • Status changed to Needs work 12 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium tim-diels Belgium ๐Ÿ‡ง๐Ÿ‡ช

    Setting back to Needs Work as there are failing tests.

  • ๐Ÿ‡ง๐Ÿ‡ทBrazil carolpettirossi Campinas - SP

    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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit

    recrit โ†’ changed the visibility of the branch 3218426-using-content-moderation to hidden.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit

    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
    
  • Pipeline finished with Failed
    12 months ago
    Total: 178s
    #142978
  • Pipeline finished with Success
    12 months ago
    Total: 668s
    #142989
  • Status changed to Needs review 12 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit

    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.

  • Status changed to Needs work 12 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit
  • Pipeline finished with Success
    12 months ago
    Total: 2132s
    #148228
  • Pipeline finished with Success
    12 months ago
    Total: 632s
    #148279
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit
  • Status changed to Needs review 12 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit

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

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit
  • Status changed to Needs work 12 months ago
  • 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.

  • Pipeline finished with Failed
    12 months ago
    Total: 1948s
    #149328
  • Pipeline finished with Failed
    12 months ago
    #149462
  • Pipeline finished with Failed
    12 months ago
    Total: 1091s
    #149475
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit

    @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.

  • ๐Ÿ‡จ๐Ÿ‡ญSwitzerland berdir Switzerland
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit

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

  • Pipeline finished with Success
    12 months ago
    Total: 993s
    #149526
  • Status changed to Needs review 12 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit

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

  • Status changed to RTBC 12 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Believe all feedback for this one has been addressed.

  • Status changed to Needs work 12 months ago
  • 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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit

    "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.

  • Pipeline finished with Success
    12 months ago
    Total: 626s
    #156717
  • Status changed to RTBC 12 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit

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

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm
  • Status changed to Needs work 12 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm

    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!

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia pradhumanjainOSL

    pradhumanjain2311 โ†’ made their first commit to this issueโ€™s fork.

  • Pipeline finished with Success
    11 months ago
    Total: 597s
    #158835
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm

    @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!

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm

    @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!

  • Pipeline finished with Failed
    11 months ago
    Total: 671s
    #168967
  • Pipeline finished with Success
    11 months ago
    Total: 607s
    #169066
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit

    @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.

  • ๐Ÿ‡ง๐Ÿ‡ทBrazil carolpettirossi Campinas - SP

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

  • Pipeline finished with Failed
    9 months ago
    Total: 180s
    #213262
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly trickfun

    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.

  • Pipeline finished with Failed
    7 months ago
    Total: 832s
    #266318
  • Pipeline finished with Failed
    7 months ago
    Total: 896s
    #266323
  • Pipeline finished with Failed
    7 months ago
    Total: 132s
    #266335
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit

    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.
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States recrit

    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.

  • Pipeline finished with Failed
    7 months ago
    Total: 539s
    #266340
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada joseph.olstad

    Tests were passing, now failing.

Production build 0.71.5 2024