Issue Summary

I ran into this error using the following code in an attempt to clear any existing messages before adding another via the new JS Messages API.

var messages = new Drupal.Message();
messages.clear(); // Throws exception - Uncaught TypeError: Cannot read property 'querySelectorAll' of null

This is a result of the fact that in the constructor: the messageWrapper can be initialized as null.

Drupal.Message = class {
    constructor(messageWrapper = null) {
      this.messageWrapper = messageWrapper;
    }

But in the clear function there is no check on a null value as exists in other functions such as add().

clear() {
  Array.prototype.forEach.call(
    this.messageWrapper.querySelectorAll('[data-drupal-message-id]'),
    message => {
      this.messageWrapper.removeChild(message);
    },
  );
}

Proposed Resolution

Minimally, the clear function should be updated with a defaultWrapper() call similar to add().

clear() {
  if (!this.messageWrapper) {
    this.messageWrapper = Drupal.Message.defaultWrapper();
  }
  Array.prototype.forEach.call(
    this.messageWrapper.querySelectorAll('[data-drupal-message-id]'),
    message => {
      this.messageWrapper.removeChild(message);
    },
  );
}

However, it might make even more sense to always initialize the messageWrapper in the constructor so that other calls such as select() don't throw this same exception.

Drupal.Message = class {
  constructor(messageWrapper = null) {
    if (!messageWrapper) {
      this.messageWrapper = Drupal.Message.defaultWrapper();
    } else {
      this.messageWrapper = messageWrapper;
    }
  }
CommentFileSizeAuthor
#14 interdiff-3052526-10-14.txt1.3 KBphenaproxima
#14 3052526-14.patch4.08 KBphenaproxima
#10 interdiff-7-10.txt1.01 KBtedbow
#10 3052526-10.patch2.92 KBtedbow
#7 3052526-7.patch2.21 KBtedbow
#7 3052526-7-test-only.patch3.08 KBtedbow
#2 drupal_message_clear_init-3052526-2.patch1.09 KBmalcolm_p

Comments

malcolm_p created an issue. See original summary.

malcolm_p’s picture

malcolm_p commented
StatusFileSize
new drupal_message_clear_init-3052526-2.patch1.09 KB

Patch attached for checking this.messageWrapper in clear().

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
Status: Active » Needs review
malcolm_p’s picture

malcolm_p commented

I ran into this error again on another project, I'd love to see a fix make it into 8.8.x. Any issues with review?

droplet’s picture

droplet commented

I think it should be initialized in the Class Constructor

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
Status: Needs review » Needs work

I think I agree with @droplet.

We can check in the constructor to see if you haven't passed a wrapper, and if not, create the default wrapper. Thanks.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Version: 8.7.0 » 8.8.x-dev
Status: Needs work » Needs review
StatusFileSize
new 3052526-7-test-only.patch3.08 KB
new 3052526-7.patch2.21 KB

Here is a fix in the constructor and test coverage

The last submitted patch, 7: 3052526-7-test-only.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
Status: Needs review » Needs work 
+++ b/core/misc/message.es6.js
@@ -22,7 +22,11 @@
+        this.messageWrapper = Drupal.Message.defaultWrapper();

We probably don't need this same check in `.add()` anymore.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 3052526-10.patch2.92 KB
new interdiff-7-10.txt1.01 KB

@alwaysworking good call

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Nice catch! Looks like the JS maintainers' feedback has been addressed.

Committed and pushed to 8.8.x. Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Fixed » Needs work

Oops. ;) Not quite.

Checking changed files...
yarn run v1.15.2
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/angie.byron/Sites/drupal/core/misc/message.es6.js
[10:58:36] '/Users/angie.byron/Sites/drupal/core/misc/message.es6.js' is being checked.
✨  Done in 1.33s.

/Users/angie.byron/Sites/drupal/core/misc/message.es6.js
  114:32  error  Replace `⏎············options.type⏎··········` with `options.type`  prettier/prettier

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

yarn run v1.15.2
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/angie.byron/Sites/drupal/core/misc/message.es6.js
[10:58:39] '/Users/angie.byron/Sites/drupal/core/misc/message.es6.js' is being checked.
✨  Done in 0.82s.

/Users/angie.byron/Sites/drupal/core/misc/message.es6.js
  114:32  error  Replace `⏎············options.type⏎··········` with `options.type`  prettier/prettier

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

yarn run v1.15.2
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/angie.byron/Sites/drupal/core/modules/system/tests/modules/js_message_test/js/js_message_test.es6.js
[10:58:42] '/Users/angie.byron/Sites/drupal/core/modules/system/tests/modules/js_message_test/js/js_message_test.es6.js' is being checked.
✨  Done in 0.85s.
yarn run v1.15.2
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/angie.byron/Sites/drupal/core/modules/system/tests/modules/js_message_test/js/js_message_test.es6.js
[10:58:45] '/Users/angie.byron/Sites/drupal/core/modules/system/tests/modules/js_message_test/js/js_message_test.es6.js' is being checked.
✨  Done in 0.78s.
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review
StatusFileSize
new 3052526-14.patch4.08 KB
new interdiff-3052526-10-14.txt1.3 KB

Hopefully, this will fix it.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Restoring RTBC when testbot passes. I feel okay with this because I literally just changed a little coding style thing. Functionally, the code is identical.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Ok great, that interdiff seems straight-forward.

Committed and pushed to 8.8.x, FOR REAL THIS TIME. ;) Thanks!

  • webchick committed c6c2ae2 on 8.8.x 
    Issue #3052526 by tedbow, phenaproxima, malcolm_p, alwaysworking,...

Status: Fixed » Closed (fixed)

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