Updated: Comment #0

Problem/Motivation

In Drupal 7, drupal.js, jquery.js and Drupal.settings were added to all pages, with no way to turn it off. This was reported in #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page. It was fixed by requiring that each piece of JS declares its dependencies, using the library system: #1737148: Explicitly declare all JS dependencies, don't use drupal_add_js. Hurray!

Sadly, this is now broken, because we weren't carefully watching this. Because we didn't have test coverage.

(I broke this: #2120117: drupalSettings.user.uid added to page even if there is no other JS; drupal.node-new-comments-link lib added for anon users.)

Proposed resolution

Write test to ensure this does not happen again.

Remaining tasks

None.

User interface changes

None.

API changes

None.

#2120117: drupalSettings.user.uid added to page even if there is no other JS; drupal.node-new-comments-link lib added for anon users

CommentFileSizeAuthor
#10 test_no_js_anon_standard-2120457-10.patch2.93 KBwim leers
#10 interdiff.txt1023 byteswim leers
#1 test_no_js_anon_standard-2120457-1.patch2.7 KBwim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
Issue tags: +JavaScript
StatusFileSize
new test_no_js_anon_standard-2120457-1.patch2.7 KB

This patch should fail, because it's currently broken in HEAD. Once #2120117: drupalSettings.user.uid added to page even if there is no other JS; drupal.node-new-comments-link lib added for anon users lands, this patch should be re-tested and it should come back green.

Status: Needs review » Needs work

The last submitted patch, test_no_js_anon_standard-2120457-1.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Yay, the expected failure! :P

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Postponed

So it's actually postponed on #2120117: drupalSettings.user.uid added to page even if there is no other JS; drupal.node-new-comments-link lib added for anon users then. Or maybe it's worth merging the two directly?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Indeed postponed on that issue.

Let's not merge them; they have different goals.

catch’s picture

catch
he/him
Primary language English
commented
Status: Postponed » Needs review
catch’s picture

catch
he/him
Primary language English
commented

#1: test_no_js_anon_standard-2120457-1.patch queued for re-testing.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

Perfect :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/system/lib/Drupal/system/Tests/Common/NoJavaScriptAnonymousTest.php
@@ -0,0 +1,74 @@
+      'name' => 'No JavaScript for anonymous users in Standard profile',
+      'description' => 'Tests that anonymous users are not served any JavaScript in the Standard installation profile.',

The name and description of the test are out of sync with what the test is asserting.

+  protected function assertNoJavaScriptExceptHtml5Shiv() {

Since this needs a small re-roll anyway to add some docs to the assertion function, could these docs explain why the shiv is ok?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new interdiff.txt1023 bytes
new test_no_js_anon_standard-2120457-10.patch2.93 KB

I don't see what's wrong with the name and description of the test at all?

Docs added to explain why the HTML5 shiv is ok.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

I think Angie means that the description says 'no JavaScript' but the actual test ensures that only the html5shiv is loaded - so it's not really 'no javascript'.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

#11: Well… the goal really is "no JavaScript". And the fact is that in every browser except IE 6, 7 and 8, no JavaScript is loaded! So for all intents and purposes, Drupal is not loading any JavaScript: there's no Drupal-specific behaviors or logic or magic being added, only a small piece of JS to make a crappy browser not break completely.
Once we drop IE8 support entirely, the HTML5 shiv will go away as well.

So I think the name is still accurate.

nod_’s picture

nod_
Primary language French
Location Lille
commented

FYI: core: #1993334: Add HTML5shiv to Stable and Classy only , ie8 module #1999948: Add HTML5shiv.

catch’s picture

catch
he/him
Primary language English
commented

OK I think that's reasonable, will leave this for a day or so in case Angie disagrees.

webchick’s picture

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

"+ * The HTML5 shiv is necessary for e.g. the tag which Drupal 8 uses
+ * to work in older browsers like Internet Explorer 8."

Cool, I think that covers it, thanks.

Committed and pushed to 8.x!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -sprint

Hurray :)

Status: Fixed » Closed (fixed)

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