Translations of nodes with comment settings field set to non-translatable returns EntityUntranslatableFieldsConstraint

To reproduce it I also had to select "Hide non translatable fields on translation forms" so I added that to the steps to reproduce.

Still I'm not really sure what you try to achieve. What do you mean with "keep the same settings for comments across all translations of the node"? What settings are you talking about?

Do you want to make the comments non-translatable? In that case, maybe instead make the comment type you use non-translatable.

I think CommentFieldItemList needs to implement it's own hasAffectingChanges that only check if status property has changed. If not it will check all properties, included the computed once like:

• status
• cid
• last_comment_timestamp
• last_comment_name
• last_comment_uid
• comment_count

And these properties will all be set to 0 until the new entity revision has been saved.

hasAffectingChanges in FieldItemList simply check !$this->equals($original_items);
https://git.drupalcode.org/project/drupal/-/blob/8.8.x/core/lib/Drupal/C...
However in the hasAffectingChanges doc it is written:

/**
* Determines whether the field has relevant changes.
*
* This is for example used to determine if a revision of an entity has
* changes in a given translation. Unlike
* \Drupal\Core\Field\FieldItemListInterface::equals(), this can report
* that for example an untranslatable field, despite being changed and
* therefore technically affecting all translations, is only internal metadata
* or only affects a single translation.

https://git.drupalcode.org/project/drupal/-/blob/8.8.x/core/lib/Drupal/Core/Field/FieldItemListInterface.php#L280-288

Adding test.

@matsbla, thanks for working on this!

Reproduced following the "Steps to reproduce" in IS with Simplified Chinese. And confirmed the patch fixed it.

1. +++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
   @@ -238,4 +238,36 @@ protected function doTestTranslationEdit() {
   +   * Test making changes on entity with hidden non-translatable comment field.
   

   Should start with "Tests", "Tests making changes on entity with hidden non-translatable comment field."

2. +++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
   @@ -238,4 +238,36 @@ protected function doTestTranslationEdit() {
   +    $this->assertSession()->pageTextNotContains('Non-translatable fields can only be changed when updating the original language.');
   ...
   +    $this->assertSession()->statusCodeEquals(200);
   

   Micro-optimization: $this->assertSession() called twice, each time it returns a new instance. So save it as a local variable would be better.

Otherwise, it's RTBC to me.

Change the target version to 9.1.x. Addressing #13

A comma should follow the last multiline array item. Found: 'article'

+++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
@@ -238,4 +238,37 @@ protected function doTestTranslationEdit() {
+      'title' => 'Node with non-translatable comment field',
+      'type' => 'article'

Fixing one CS violation. Missing ','

Updating the issue summary with the resolution that @matsbla wrote up in #5.

Reviewed for the bug smash initiative :). I'm not a translation expert, but based on your analysis and the docblock for hasAffectingChanges, this looks like an unambiguous slam dunk.

Setting back to NW, but will keep an eye on the issue to RTBC.

1. +++ b/core/modules/comment/src/CommentFieldItemList.php
   @@ -63,4 +64,11 @@ public function access($operation = 'view', AccountInterface $account = NULL, $r
   +    return $this->status != $original_items->status;
   

   Maybe strict equals here? Especially when dealing with all the weirdness that comes with zeros.

2. +++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
   @@ -238,4 +238,37 @@ protected function doTestTranslationEdit() {
   +  /**
   +   * Tests making changes on entity with hidden non-translatable comment field.
   +   */
   +  public function testNonTranslatableCommentFieldHasAffectingChanges() {
   

   I love this test to cover the bug, but since this is also a public method/API, it might be nice to have a more specific test (maybe in CommentFieldItemList) that calls the method directly, with a few basic test cases? Broad functional tests are super valuable for coverage, but I always appreciate a few integration tests to narrow down the exact problematic code-paths when tracking down test fails.

3. +++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
   @@ -238,4 +238,37 @@ protected function doTestTranslationEdit() {
   +    // Create article translator
   

   Missing full stop?

4. +++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
   @@ -238,4 +238,37 @@ protected function doTestTranslationEdit() {
   +    // Create translation on entity with hidden non-translatable comment field.
   +    $this->drupalPostForm("/node/$nid/translations/add/en/fr", [], t('Save (this translation)'));
   +    $assert_session = $this->assertSession();
   +    $assert_session->pageTextNotContains('Non-translatable fields can only be changed when updating the original language.');
   +    $this->drupalGet("fr/node/$nid");
   +    $assert_session->statusCodeEquals(200);
   

   I wonder if we should assert the positive case here, aka "The page was saved successfully!", since if for whatever reason this message changed, that change wouldn't ever fail this test and thus it may never get updated.

   Alternatively (or additionally), it might be helpful to submit the form WITH a "status" change and assert the validation message does come up.

Here I have fixed point no. 1 and 3 of comment #17.

It still needs work for the remaining points of comment #17.

Thanks for the reviews and help!

assert the positive case here, aka "The page was saved successfully!"

Here is fix for point 4 in comment #17. However, the test already view the translation and assert that is returns code 200, which should confirm that the translation has been successfully saved.

it might be helpful to submit the form WITH a "status" change and assert the validation message does come up.

I don't think it is possible, as the comment field settings are untranslatable and the form is configured with "Hide non translatable fields on translation forms", the field is hidden and can't be changed on the translation form.

+++ b/core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
@@ -238,4 +238,38 @@ protected function doTestTranslationEdit() {
+    $this->drupalGet("fr/node/$nid");
+    $assert_session->statusCodeEquals(200);

I think it would be better to assert an actual change, you could change the title in the translation and make sure that shows up.

that this is an access denied on HEAD if there is no translation is imho a bug and I have an issue that changes it so it does the fallback, so that would then no longer assert much.

Alternatively, we can also remove it now that we assert the success message, which I guess is also sufficient.

Took out the deprecated methods in the test and removed the 200 assert per #23 (which I agree with)

Quick manual test on the site where we ran into this seems to show that this only helps for published nodes. If the original is unpublished, the translation still gives the error.

Didn't dig into this and didn't try to reproduce on a clean install yet, so might be something on this particular project.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

I just encountered this on a project.
I tested on the main branch and it is still valid/relevant so I will create a MR from patch #25.

I was not able to reproduce #26 so I suspect it is project-specific.

I addressed the remaining point 2 from #17, a unit test involved a lot of mocking so I went with a kernel one.
For phpstan, not sure if (or how) we should actually address this, it simply uses the magic methods...

I believe #3025039: New non translatable field on translatable content throws error would also cover this here.
I guess if that one goes in, we should ask ourselves what CommentFieldItemList::hasAffectingChanges really does and if we really want to change it.
OTOH I would not count on that issue to go in... the current solution there is probably not good enough at the moment.

Left some comments on the MR.

Added the following changes to the merge request:

1. Updated the branch with the latest changes to main
2. Configuration setup in CommentTranslationUITest is now set through the API instead of through the user interface
3. Inlined code for $user in CommentTranslationUITest to avoid needing a $user variable
4. Updated the comment for CommentItemTest::testCommentHasAffectingChanges, but the scope of test is very narrow so a comment quickly becomes redundant.

Patch attached without the changes to test code for composer patches.

Think since this is introducing a new function to a used server probably need a CR.

Not sure if this counts as an API change (think not) but the section in the summary appears to be missing saying if it is or not.