Multiple warning messages when having untranslatable fields

Testing that is easy, it doesn't just happen on ajax requests. It also happens if you just submit the form, then you see that message on the node page. there's propably a test looking for this message. Extend it to make sure that the message doesn't show up after submitting.

I think we have to use #theme status_messages instead of hardcoded the classes.

Basically what \Drupal\Core\Render\Element\StatusMessages::renderMessages() does. It's a bit unfortunate that we have to duplicate the label s (at least the one we actually need).

I also think that this isn't really a warning, there's nothing wrong/problematic, nothing could be done differently, so a blue info would work better but status messages can't do that. I'd suggest to change that elsewhere if we think it should be.

Also added a screenshot to the summary that shows how this looks after saving which is the problem that's much easier to reproduce.

What you are missing now is the #status_headings label for warning, I think we should include that too. as long as we are displaying it as a warning message.

Looks good to me now.

No opinion on whether this should be added to 8.6 or not, it is IMHO a bug but not that important. And also a minor behavior change, in case someone was e.g. trying to change that specific message.

Lets see what our frontend maintainers think about this approach :)

Created #3004334: Messages about hidden untranslatable fields and pending revisions as a follow-up.

Also realized that drupal.org actually has the same bug, the security warning also repeats when you upload a file :)

Hmmm. I think we should have a more generic fix as this can happen with any message like this. Shouldn't the AJAX code that adds the messages to the screen check to see if the message is already there? This is similar to the $repeat functionality. I checked to see if #77245: Provide a common API for displaying JavaScript messages addresses this but it doesn't.

+++ b/core/modules/content_translation/src/ContentTranslationHandler.php
@@ -595,7 +595,15 @@ public function entityFormSharedElements($element, FormStateInterface $form_stat
-      $this->messenger->addWarning($this->t('Fields that apply to all languages are hidden to avoid conflicting changes. <a href=":url">Edit them on the original language form</a>.', [':url' => $url]));
+      $message['warning'][] = $this->t('Fields that apply to all languages are hidden to avoid conflicting changes. <a href=":url">Edit them on the original language form</a>.', [':url' => $url]);
+      $element['hidden_fields_warning_message'] = [
+        '#theme' => 'status_messages',
+        '#message_list' => $message,
+        '#weight' => -100,
+        '#status_headings' => [
+          'warning' => $this->t('Warning message'),
+        ]
+      ];

Doesn't this change move the message from the common status message block to it's own block. That feels a bit off.

> Shouldn't the AJAX code that adds the messages to the screen check to see if the message is already there? This is similar to the $repeat functionality. I checked to see if #77245: Provide a common API for displaying JavaScript messages addresses this but it doesn't.

It's not just ajax that is affected, it's enough to simply submit the form and then you see the message again on the node page.

I don't think this can be solved generically/automatically. If anything, we'd need a rule/guideline that form-building must not use the message API.

> Doesn't this change move the message from the common status message block to it's own block. That feels a bit off.

It does, but I don't think that is wrong. It's not a notification message that should pop up wherever the theme wants it, it belongs specifically to that form.. e.g. imagine edit forms in dialogs or so.

1. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -595,7 +595,15 @@ public function entityFormSharedElements($element, FormStateInterface $form_stat
        if ($display_warning && !$form_state->isSubmitted() && !$form_state->isRebuilding()) {
   

   Do we need the !$form_state stuff now then?

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -595,7 +595,15 @@ public function entityFormSharedElements($element, FormStateInterface $form_stat
   +      $message['warning'][] = $this->t('Fields that apply to all languages are hidden to avoid conflicting changes. <a href=":url">Edit them on the original language form</a>.', [':url' => $url]);
   +      $element['hidden_fields_warning_message'] = [
   +        '#theme' => 'status_messages',
   +        '#message_list' => $message,
   +        '#weight' => -100,
   +        '#status_headings' => [
   +          'warning' => $this->t('Warning message'),
   +        ]
   +      ];
   

   Isn't one issue that if there is something that is adding a message to the page - for example the node access rebuild message then this change will break how multiple messages are normally listed together? Ie. normally all the warnings are grouped together.

That's true, but I do believe that a warning message for this is anyway the wrong pattern, which is why I created #3004334: Messages about hidden untranslatable fields and pending revisions.

A warning message indicates a problem/something that user did wrong or at least that he needs to be careful about or so. That's IMHO not the case here, everything is working exactly like it should, we just don't have any UX pattern for telling the user that and why some fields are now not visible :)

Reroll due to a conflict in the test, no other changes. There was a test fail on 8.9, so this is likely going to fail too.

patch #25 Failed to Apply.
Added new patch please review.

Forgot to update the version, #25 should apply on 9.2

Thanks for the update @Berdir
as #25 Custom command failed try to solve this added new patch.
sorry forgot to update patch no.

Fixing failed tests of patch #29.

Here is the fix for failed test patch of #31.

This issue still occurs on Drupal 9.5.x, I rerolled the patch from #33.

Hi Patch #37 applied successfully on Drupal 10.1.x
The patch work properly for me.
Thanks.

From what I can tell the points in #21 have not been addressed/answered. Maybe @berdir answers #21.2

Tagging for usability review for the phrase.

I discussed this a bit with @laurii at DevDays23 and he pointed me to the fact that something very similar has been done in \Drupal\ckeditor5\Plugin\Editor\CKEditor5::buildConfigurationForm() if for slightly different reasons. But I think that would have similar problems as well with the message repeating on submit and ajax operations.

I still think it makes sense to solve this bug and then think about a generic solution or not using warning messages for this at all. We discussed that the new toggletip might give us some points.

I also cleaned up the patch and removed a lot of unrelated changes and I removed the $form_state check, that should indeed not be required anymore.

Cleaned up a the kernel test, the previous fix did work before, but only because it didn't actually test the display message part anymore. Now with the removed form state checks, the previous two cases also had the message form element, I fixed them and removed the logic for display warning completely.

No fond of those kind of tests, I don't think they're testing much and are hard to understand and maintain.

Removing the needs usability review tag. I don't think there's something to review here, that you no longer get duplicated messages or a message after saving the node doesn't needs a usability review. The issue that would need one is the follow-up that I created that wants to use a different pattern than warning messages for this.

Ok, we can actually clean up quite a bit more in that test as we no longer need those two flags at all.

meh.

Ran the tests without the fix and got

ContentTranslationUntranslatableFieldsTest

Behat\Mink\Exception\ResponseTextException : The text "Fields that apply to all languages are hidden to avoid conflicting changes." appears in the text of this page, but it should not.

ContentTranslationHandlerTest passes without issue.

The fix matches the IS of transforming message to array.

Think this is good!

Committed 29ae244 and pushed to 11.x. Thanks!

Not backporting to 10.1.x because of the small behavior change here.

I updated the code comment on commit. Here's interdiff for that.

Patch updated for 10.2.x branch.