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.

Only local images are allowed.

Files: 
CommentFileSizeAuthor
#11 core-js-contextual-backbone-1971108-11.patch2.5 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 56,088 pass(es).
[ View ]
#3 core-js-contextual-error-admin-1988328-3.patch1.92 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 55,503 pass(es).
[ View ]
#1 contextual-collections-bug-1988328-1.patch2.41 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 55,554 pass(es).
[ View ]
Screenshot_5_6_13_1_39_PM.png265.62 KBjessebeach

Comments

Status:Active» Needs review
Issue tags:+sprint, +Spark
StatusFileSize
new2.41 KB
PASSED: [[SimpleTest]]: [MySQL] 55,554 pass(es).
[ View ]

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

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.

StatusFileSize
new1.92 KB
PASSED: [[SimpleTest]]: [MySQL] 55,503 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Same fix as in #1 with a nicer syntax.

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

:)

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

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

Status:Needs work» Closed (works as designed)

turns out #914382: Contextual links incompatible with render cache fixed it so it's all good.

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.

Status:Needs work» Needs review
StatusFileSize
new2.5 KB
PASSED: [[SimpleTest]]: [MySQL] 56,088 pass(es).
[ View ]

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).

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!

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks!

Issue tags:-sprint

.

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