This image captures the bug. It shows a Drupal 8 site on the /admin path. The debugger is paused on line 53 contextual.toolbar.js. In this case, we are trying to access Drupal.contextual.collection, but Drupal.contextual is not defined.

It shows a Drupal 8 site on the /admin path. The debugger is paused on line 53 contextual.toolbar.js. In this case, we are trying to access Drupal.contextual.collection, but Drupal.contextual is not defined.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jessebeach’s picture

Status: Active » Needs review
Issue tags: +sprint, +Spark
FileSize
2.41 KB

Here's the patch that fixes it. We need to check for Drupal.contextual before accessing Drupal.contextual.collection.

Wim Leers’s picture

There already is a follow-up patch on the issue that introduced this: #1971108-36: Convert contextual.js to use Backbone (and support dynamic contextual links). It solves it very differently though. This is slightly more complex, but also better for front-end perf.

Let's figure out the best solution here.

nod_’s picture

I'm more for something like #1 than adding the dep to the script.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Same fix as in #1 with a nicer syntax.

nod_’s picture

If it wasn't for #1639012: Guard against broken behaviors it'd be a critical bug :p

Wim Leers’s picture

:)

tim.plunkett’s picture

I only found it because I run hacked core JS to expose stuff like this.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs reroll...

curl https://drupal.org/files/core-js-contextual-error-admin-1988328-3.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1970  100  1970    0     0  37175      0 --:--:-- --:--:-- --:--:-- 42826
error: patch failed: core/modules/contextual/contextual.toolbar.js:46
error: core/modules/contextual/contextual.toolbar.js: patch does not apply
nod_’s picture

Status: Needs work » Closed (works as designed)
Wim Leers’s picture

Status: Closed (works as designed) » Needs work

#9: actually, the solution applied in #914382: Contextual links incompatible with render cache is suboptimal, as you already said in #3. So this patch should still be rerolled, and should remove the dep introduced by #914382: Contextual links incompatible with render cache.

nod_’s picture

Status: Needs work » Needs review
FileSize
2.5 KB

Umm, sure, but in contextual_page_build there is $page['#attached']['library'][] = array('contextual', 'drupal.contextual-links');

Which means that it'll be on the page regardless.

Anyway, here is the patch. Made into an anonymous function otherwise a js checking tool complains about a function defined in a if (because of hoisting and that sort of things).

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

You're right. But it's possible that somebody removes the library in hook_page_alter() for some reason, and then thanks to #11 we still won't break. So it's still better :)

Thanks!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Wim Leers’s picture

Issue tags: -sprint

.

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