Problem/Motivation

In message.js there is JS that will result in error if a status messages element is not present

 static defaultWrapper() {
      let wrapper = document.querySelector('[data-drupal-messages]');
      if (!wrapper) {
        wrapper = document.querySelector('[data-drupal-messages-fallback]')z;
        wrapper.removeAttribute('data-drupal-messages-fallback');
        wrapper.setAttribute('data-drupal-messages', '');
        wrapper.classList.remove('hidden');
      }
      return wrapper.innerHTML === ''
        ? Drupal.Message.messageInternalWrapper(wrapper)
        : wrapper.firstElementChild;
    }

Prior to #2987444: Ajax errors are not communicated through the UI, core/drupal.messages was only loaded via the status_messages render element. Since that issue, core/drupal.ajax also loads core/drupal.messages, without there necessarily being a status_messages element on the page. If there is no status messages render element and Ajax needs to report a problem, there will be an execution-breaking JS error due to defaultWrapper() being called without the elements it expects available.

This causes #3406612: Exceptions in batch no longer are shown on the page: Javascript error but wasn't notice because there not tests for errors showing up on the batch page

Running into this IRL otherwise is less likely since most sites have a status messages element loading somewhere on any given page. It's more likely for this to occur in an automated test as those are scenarios where only the subjects under testing are on-page.

Steps to reproduce

Proposed resolution

If there is no status_messages render element present on a page then create one fallback element using Javascript to prevent JavaScript errors.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3396099

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

bnjmnm created an issue. See original summary.

tedbow’s picture

tedbow’s picture

Issue summary: View changes
tedbow’s picture

I put a fix in #3406612: Exceptions in batch no longer are shown on the page: Javascript error. This doesn't solve the problem of the library requiring the status_messages render element but it does allow putting that render element on the page without it automatically rendering the service side messages.

The needs of the batch page is that messages the server knows about should not get rendered on this page but if there are ajax calls that results in errors these should be rendered on the page(at least to keep the pre-existing functionality).

Tagging this as Automatic Updates Initiative because we need have error messages on the batch page for Automatic Updates. Otherwise if we did beta testing and users couldn't easily see these error the testing would be much less useful.

So we could solve in the way I did in #3406612: Exceptions in batch no longer are shown on the page: Javascript error or another way here or maybe a temp fix in the AutoUpdates module itself would be ok like we did here #3406812: Ensure that errors are shown on form updates in core 10.1.x

In any case we have to solve it someway to get Automatic Updates into core so bumping this to Major

anybody’s picture

Funny, just ran into this and saw you recently commented ;D

It's not yet totally clear to me yet, what the change was, that caused this. Technically, we require [data-drupal-messages] or [data-drupal-messages-fallback] to be present on the page.

But if not, it should be added by JS as last resort?

My concern with other solutions is, that they may also rely on custom configuration or theme implementations, which we can't really rely on?

Edit: Sorry, the element was present in my case. This was caused by my custom mistake (wrong parameter order in MessageCommand()).

rob230’s picture

I think I am seeing this too, in Drupal 10. When running a migration, if it tries to output a message (e.g. an error), it causes the batch page to break due to a console error: wrapper is null.

static defaultWrapper() {
  let wrapper = document.querySelector('[data-drupal-messages]');
  if (!wrapper) {
    wrapper = document.querySelector('[data-drupal-messages-fallback]');
    wrapper.removeAttribute('data-drupal-messages-fallback');
    wrapper.setAttribute('data-drupal-messages', '');
    wrapper.classList.remove('hidden');
  }
  return wrapper.innerHTML === ''
    ? Drupal.Message.messageInternalWrapper(wrapper)
    : wrapper.firstElementChild;
}

There is no such element on the page with data-drupal-messages or data-drupal-messages-fallback attributes. I would guess the error was introduced in #77245: Provide a common API for displaying JavaScript messages.

Previous behaviour was that the progress bar would disappear and a short error message would be shown with a link to the page you came from and then any messages would be shown on that page. Now it just appears like nothing is happening, the progress bar is stuck, and you have to open the console to see that it had this error with the wrapper variable.

vensires’s picture

Issue tags: +GreeceSpringSprint2024

Utkarsh_33 made their first commit to this issue’s fork.

utkarsh_33’s picture

Status: Active » Needs review
bnjmnm’s picture

Status: Needs review » Needs work

see MR

utkarsh_33’s picture

Status: Needs work » Needs review
bnjmnm’s picture

Status: Needs review » Needs work
utkarsh_33’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs issue summary update

Could the issue summary be updated some please. Mainly proposed solution. Assuming not a UI or Data model change.

Also tagging for tests as mentioned in the summary

wasn't notice because there not tests for errors showing up on the batch page

utkarsh_33’s picture

Issue summary: View changes
utkarsh_33’s picture

Issue summary: View changes
utkarsh_33’s picture

Also

core/modules/system/tests/src/FunctionalJavascript/Batch/ProcessingTest.php

test was added in https://www.drupal.org/project/drupal/issues/3406612 issue with a temporary fix which is now removed and replaced with a concrete JS fix.
If Still there is a requirement for adding test I am happy to do that.

utkarsh_33’s picture

Status: Needs work » Needs review
smustgrave’s picture

Category: Bug report » Task
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests, -Needs issue summary update +Needs Review Queue Initiative

So if I understand correctly this is more about refactoring a temporary solution into a better one, in that case believe task is probably more right category.

Removed the tests tag per #18, if this solution isn't doing anything new then agree probably doesn't need additional coverage.

Thanks for updating the issue summary removing that tag.

Looking at the change and update seems simple enough and didn't break any existing coverage.

nod_’s picture

Status: Reviewed & tested by the community » Needs work

Couple of things to simplify then it's good to go, thanks!

Gauravvvv made their first commit to this issue’s fork.

gauravvvv’s picture

Status: Needs work » Needs review
nod_’s picture

Status: Needs review » Needs work

you need to keep

        wrapper.removeAttribute('data-drupal-messages-fallback');
        wrapper.setAttribute('data-drupal-messages', '');

outside the if (!wrapper)

utkarsh_33’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Feedback appears to be addressed.

  • nod_ committed dfbffd2a on 10.3.x
    Issue #3396099 by Utkarsh_33, nod_, Gauravvvv, bnjmnm, tedbow,...

  • nod_ committed 03f71c73 on 10.4.x
    Issue #3396099 by Utkarsh_33, nod_, Gauravvvv, bnjmnm, tedbow,...

  • nod_ committed 653c5733 on 11.0.x
    Issue #3396099 by Utkarsh_33, nod_, Gauravvvv, bnjmnm, tedbow,...
nod_’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed bff71e54fc to 11.x and 653c57335b to 11.0.x and 03f71c7351 to 10.4.x and dfbffd2a19 to 10.3.x. Thanks!

  • nod_ committed 74d72efa on 11.x
    Issue #3396099 by Utkarsh_33, nod_, Gauravvvv, bnjmnm, tedbow,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.