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
instead of patch #16, try the MR diff here:
https://git.drupalcode.org/project/drupal/-/merge_requests/1373.diff - ๐จ๐ฆ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 10:52am 6 July 2023 - last update
almost 2 years ago Custom Commands Failed - last update
almost 2 years ago Custom Commands Failed - Status changed to Needs work
over 1 year ago 2:23pm 7 July 2023 - ๐บ๐ธ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 7:24am 19 July 2023 - 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 - 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.
The last submitted patch, 33: 3218426-based-on-1373-MR-33.patch, failed testing. View results โ
- 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 1:23pm 24 July 2023 - ๐บ๐ธUnited States smustgrave
Can the IS be updated please with the proposed solution.
- Status changed to Needs review
over 1 year ago 6:03am 25 July 2023 - 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 4:14pm 7 August 2023 - ๐บ๐ธ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.
- Merge request !5385Issue #3218426: Using content moderation and partially synchronized image... โ (Open) created by borisson_
- ๐ง๐ช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.
- ๐ง๐ชBelgium borisson_ Mechelen, ๐ง๐ช
I don't understand the cspell failures.
- ๐ง๐ช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
areNULL
,$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, overridinghasAffectingChanges
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. - First commit to issue fork.
- Status changed to Needs review
12 months ago 9:13am 8 April 2024 - ๐ง๐ชBelgium tim-diels Belgium ๐ง๐ช
Rebased the MR and resolved the conflict where the phpstan-baseline.neon changed to .phpstan-baseline.php.
- Status changed to Needs work
12 months ago 9:25am 8 April 2024 - ๐ง๐ช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 newImageFieldItemList::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
- Status changed to Needs review
12 months ago 3:37pm 10 April 2024 - ๐บ๐ธ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 originalImageItem
code would calculate the width and height inImageItem::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 inImageItem::onChange()
andImageItem::preSave()
.The attached patch can be used for composer builds.
- Status changed to Needs work
12 months ago 1:08pm 15 April 2024 - ๐บ๐ธ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.
- Status changed to Needs review
12 months ago 3:03pm 16 April 2024 - ๐บ๐ธUnited States recrit
@smustgrave All MR comments have been resolved and the Issue Summary has been updated.
- Status changed to Needs work
12 months ago 3:27pm 16 April 2024 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
@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
See https://www.drupal.org/node/2910500 โ . Don't use root user.
- ๐บ๐ธUnited States recrit
@Berdir thank you! that makes sense now. I'll switch it to add permissions to the test user.
- Status changed to Needs review
12 months ago 8:42pm 17 April 2024 - ๐บ๐ธ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 6:41pm 18 April 2024 - ๐บ๐ธUnited States smustgrave
Believe all feedback for this one has been addressed.
- Status changed to Needs work
12 months ago 4:01pm 25 April 2024 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. - Status changed to RTBC
12 months ago 6:38pm 25 April 2024 - ๐บ๐ธUnited States recrit
successfully rebased with the latest 11.x, moving back to RTBC
- Status changed to Needs work
12 months ago 9:49pm 26 April 2024 - ๐บ๐ธ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.
- ๐บ๐ธ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!
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
xjm โ credited larowlan โ .
- ๐บ๐ธ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!
- ๐บ๐ธ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. - Merge request !86193218426: Fix issue with partially translated images and moderation. Re-rolled... โ (Open) created by carolpettirossi
- ๐ง๐ทBrazil carolpettirossi Campinas - SP
I re-rolled patch #49 for Drupal 10.3.x and it's available in the MR 8619 above.
- ๐ฎ๐นItaly trickfun
Same error on drupal 10.2.7 but i can't apply patch MR 8619 or MR 5385
Thank you - ๐บ๐ธ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.
- Patch for 11.x only:
- ๐บ๐ธ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.