Problem/motivation

Drupal 7 includes jquery.js, drupal.js and inlines Drupal.settings on every page even if it isn't needed. This has some serious implications on the performance, especially on mobile connections: 2-3 more requests, ±50KB and at least an extra 200-300 ms extra processing time (just to parse jquery.js)

Proposed solution

This patch will only include jquery.js, drupal.js and will only output Drupal.settings if it's needed. But since there's a chance this will break existing sites, there's a built-in kill switch. By default this patch behaves like older versions of Drupal 7, unless no javascript is added at all (using either drupal_add_js, hook_library or by specifying the javascript file inside module/theme .info file)

But if you set $conf['javascript_always_use_jquery'] = FALSE inside settings.php (or use drush vset javascript_always_use_jquery 0 -y), it will only output the files if they are needed, by checking the new option need_jquery.

API changes

  1. A new variable javascript_always_use_jquery.
  2. A new option for drupal_add_js/hook_library need_jquery (default TRUE) that allows you to specify if jQuery is needed.
  3. Themes can use scripts_special[] inside their info file to indicate that the included script does not depend on javascript.
  4. drupal_get_js has a new parameter $need_ajaxPageState = FALSE needed to force the output of $setting['ajaxPageState']['js'] even if no file is added.

Example

picture_library uses this, because the javascript does not depend on jQuery.

Possible problems

Modules / themes overwriting template_process_html, they now have to include a call to drupal_add_js_page_defaults(), to support backwards compatibility with older Drupal versions (< 7.22), they can use something like:

<?php
 
// Get default javascript.
 
if (function_exists('drupal_add_js_page_defaults')) {
   
drupal_add_js_page_defaults();
  }
?>

Remarks

All existing modules using hook_library will automatically trigger the inclusion of jQuery, because the need_jquery will be set to TRUE.

Original report

Title says it all.

Detailed description

In theme.inc, drupal_theme_initialize() always sets a ajaxPageState setting through drupal_add_js(). In drupal_add_js_code(), the following code runs:

if (isset($data)) {
    // Add jquery.js and drupal.js, as well as the basePath setting, the
    // first time a JavaScript file is added.

$data can also be a JavaScript setting. However, the if-test does not check this case. The quoted comment correctly states that we only need to add jquery.js and drupal.js (jquery.once.js is mistakenly not mentioned) as soon as a JavaScript file has added, but does not properly check this.

Hence, we simply need to change the check to:

if (isset($data) && isset($options['type']) && $options['type'] != 'setting') {

(We should also update the comment to include the mention of jquery.once.js, but that is minor.)

Consequences of this bug
It is currently in practice impossible to create any page in Drupal without any JavaScript files being added at all! (Because the theme layer always sets at least one JavaScript setting.)

On basic Drupal sites, this also results in the unnecessary addition and thus loading of JavaScript files, adversely affecting performance (hence tagging as WPO).

Patch attached that fixes this problem.

Files: 
CommentFileSizeAuthor
#170 drupal-1279226-170.patch18.64 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 41,111 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#156 interdiff.txt1.66 KBattiks
#156 i1279226-156.patch24.08 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 40,387 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#140 interdiff.txt1.73 KBattiks
#140 i1279226-140.patch24.06 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 40,397 pass(es).
[ View ]
#134 interdiff.txt1.31 KBattiks
#134 i1279226-134.patch22.15 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 40,367 pass(es).
[ View ]
#130 interdiff.txt1.77 KBattiks
#130 i1279226-130.patch21.01 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 40,385 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#128 interdiff.txt2.98 KBattiks
#128 i1279226-127.patch21.02 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 40,434 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#125 interdiff.txt2.55 KBattiks
#125 i1279226-125.patch18.98 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 40,349 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
#118 interdiff.txt2.02 KBattiks
#118 i1279226-118.patch20.21 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 40,019 pass(es), 48 fail(s), and 12,148 exception(s).
[ View ]
#113 i1279226-113.patch18.85 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 39,913 pass(es), 56 fail(s), and 24,445 exception(s).
[ View ]
#100 drupal8.js-defaults.100.patch17.62 KBsun
FAILED: [[SimpleTest]]: [MySQL] 37,012 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#90 i1279226-90.patch28.21 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 36,520 pass(es).
[ View ]
#88 i1279226-88.patch27.3 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 36,514 pass(es), 0 fail(s), and 46 exception(s).
[ View ]
#85 i1279226-85-tests.patch11.27 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 36,391 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#85 i1279226-85.patch28.68 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 36,457 pass(es), 25 fail(s), and 12 exception(s).
[ View ]
#82 drupal8.js-defaults.82.patch17.31 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 36,496 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#76 drupal8.js-defaults.75.patch12.52 KBsun
FAILED: [[SimpleTest]]: [MySQL] 36,302 pass(es), 11 fail(s), and 0 exception(s).
[ View ]
#74 drupal8.js-defaults.74.patch12.66 KBsun
FAILED: [[SimpleTest]]: [MySQL] 36,276 pass(es), 25 fail(s), and 5 exception(s).
[ View ]
#64 testPageWithOnlyJavascriptSetting.patch2.32 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 35,006 pass(es).
[ View ]
#64 i1279226-64.patch10.76 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 35,006 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#63 1279226-63.patch8.91 KBericduran
PASSED: [[SimpleTest]]: [MySQL] 35,020 pass(es).
[ View ]
#60 1279226-60.patch8.93 KBericduran
PASSED: [[SimpleTest]]: [MySQL] 34,989 pass(es).
[ View ]
#59 1279226-59.patch7.83 KBericduran
PASSED: [[SimpleTest]]: [MySQL] 35,004 pass(es).
[ View ]
#58 1279226-56.patch7.62 KBericduran
FAILED: [[SimpleTest]]: [MySQL] 34,995 pass(es), 5 fail(s), and 4 exception(s).
[ View ]
#56 1279226-55-core-js.patch7.98 KBericduran
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1279226-55-core-js.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#55 i1279226-55.patch23.83 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 34,861 pass(es), 40 fail(s), and 8,604 exception(s).
[ View ]
#55 i1279226-55-combined.patch29.79 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 35,002 pass(es).
[ View ]
#51 i1279226-50.patch27.9 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 34,987 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#49 i1279226-49.patch27.26 KBattiks
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1279226-49.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#49 interdiff_48_49.txt3.37 KBattiks
#48 i1279226-48.patch25.03 KBattiks
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1279226-48.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#46 i1279226-46-test.patch922 bytesattiks
FAILED: [[SimpleTest]]: [MySQL] 35,390 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#46 i1279226-46.patch23.95 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 35,391 pass(es).
[ View ]
#45 i1279226-45.patch23.25 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 35,371 pass(es).
[ View ]
#40 i1279226-40.patch23.08 KBattiks
PASSED: [[SimpleTest]]: [MySQL] 35,360 pass(es).
[ View ]
#38 1279226-36-js-loaded-everywhere.patch5.64 KBericduran
FAILED: [[SimpleTest]]: [MySQL] 33,348 pass(es), 55 fail(s), and 95 exception(s).
[ View ]
#35 1279226-35-js-loaded-everywhere.patch5.6 KBericduran
FAILED: [[SimpleTest]]: [MySQL] 33,352 pass(es), 55 fail(s), and 95 exception(s).
[ View ]
#34 1279226-34-js-loaded-everywhere.patch4.93 KBericduran
FAILED: [[SimpleTest]]: [MySQL] 33,334 pass(es), 119 fail(s), and 116 exception(s).
[ View ]
#33 1279226-33-js-loaded-everywhere.patch4.92 KBericduran
FAILED: [[SimpleTest]]: [MySQL] 33,251 pass(es), 185 fail(s), and 410 exception(s).
[ View ]
#30 i1279226-30.patch13.68 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 35,346 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#25 i1279226-25.patch4.83 KBattiks
FAILED: [[SimpleTest]]: [MySQL] 35,341 pass(es), 14 fail(s), and 64 exception(s).
[ View ]
#21 i1279226-21.patch4.74 KBattiks
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#15 only-js-settings-dont-add-jquery_15.patch11.27 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 37,887 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#13 only-js-settings-dont-add-jquery_13.patch11.01 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 37,577 pass(es), 150 fail(s), and 16,865 exception(s).
[ View ]
#8 only-js-settings-dont-add-jquery_8.patch9.81 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 32,982 pass(es), 1 fail(s), and 1 exception(es).
[ View ]
#5 only-js-settings-dont-add-jquery_5.patch1.83 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 32,939 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#3 only-js-settings-dont-add-jquery_3.patch1.83 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch only-js-settings-dont-add-jquery_3.patch. See the log in the details link for more information.
[ View ]
only-js-settings-dont-add-jquery.patch933 bytesWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch only-js-settings-dont-add-jquery.patch. See the log in the details link for more information.
[ View ]

Comments

Crell’s picture

Status:Active» Needs review

Seems logical to me, although I'll let someone who knows the JS system better mark it RTBC.

Status:Needs review» Needs work

The last submitted patch, only-js-settings-dont-add-jquery.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new1.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch only-js-settings-dont-add-jquery_3.patch. See the log in the details link for more information.
[ View ]

Found a bug: the one-line change in the first patch forgets to send the basic JS settings (basicPath and pathPrefix). Fixed that in this follow-up patch.

Also, testbot is broken (it couldn't apply the patch).

Status:Needs review» Needs work

The last submitted patch, only-js-settings-dont-add-jquery_3.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new1.83 KB
FAILED: [[SimpleTest]]: [MySQL] 32,939 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Okay, so testbot can't handle patches without prefixes. WTF.

Then this patch should pass testing.

sun’s picture

Needs review for the testbot.

I think we want to cover this expectation in a test though.

Lastly, I'm not sure whether it makes sense to output JS settings when no JS is loaded...? Would chuck away some more bytes?

Status:Needs review» Needs work

The last submitted patch, only-js-settings-dont-add-jquery_5.patch, failed testing.

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new9.81 KB
FAILED: [[SimpleTest]]: [MySQL] 32,982 pass(es), 1 fail(s), and 1 exception(es).
[ View ]

#6: this was a work-around I quickly came up with because the Views UI actually broke for me after the first patch. Frankly, the current drupal_add_js() is a mess. The new version of the patch cleans up the confusing parts and separates the setting of default options for JavaScript settings from the setting of the basic JavaScript settings (basePath and pathPrefix) and basic JavaScript files (jquery.js, jquery.once.js, drupal.js).

Existing tests updated. Test added to verify this expectation. Tests ran successfully ran locally.

Finally, in a follow-up patch we should prevent all JavaScript settings output if only JavaScript settings were defined. Since this constitutes an API change, this will apply to D8 only.

sun’s picture

@Wim Leers: I don't really see how we can skip the JS files without also removing the settings.

That is, because without JS files being loaded, this:

jQuery.extend(Drupal.settings,
  1. throws a JS error "jQuery is undefined".
  2. if it doesn't throw 1), then it throws "extend is not a function".
  3. if it doesn't throw 2), then it throws "Drupal is undefined".
  4. if it doesn't throw 3), then it throws "settings is undefined".

Status:Needs review» Needs work

The last submitted patch, only-js-settings-dont-add-jquery_8.patch, failed testing.

Wim Leers’s picture

LOL! Of course. So stupid.

Tests were good, except for one fail in a test in AJAX frameWork. Plus an exception due to a debug leftover in my own test.

Will reroll.

sun’s picture

That said, I think we can only skip the settings, if they are identical to the hard-coded default settings of Drupal core. Any difference to the defaults means that some module/code is trying to do something with the settings. A simple array_diff_key(), ignoring values, should be sufficient though.

Wim Leers’s picture

StatusFileSize
new11.01 KB
FAILED: [[SimpleTest]]: [MySQL] 37,577 pass(es), 150 fail(s), and 16,865 exception(s).
[ View ]

1) New version works as described in #12. Remaining known problem: JS settings are also not added when no other module is added, but ajaxPageState is still necessary. I don't know if this is a situation that can occur — this needs to be investigated.

2) Also new: when adding a piece of *inline* JS, the code is checked to see if it uses either jQuery or Drupal.js. If it uses neither, then the jquery.js, jquery.once.js and Drupal.js are *also* not added. Use case: Google Analytics module relies on (the external) ga.js; doesn't need either jQuery or Drupal.js.
This is *ugly* as hell. What we really need, is a JS dependency system (see #1033392-43: Script loader support in core (LABjs etc.)). Each piece of JS would then mark its dependencies — or knowing Drupal: the drupal_add_js() would most likely mark dependencies.

This is for D7. I seriously doubt this will make it into D7 or D8 core in this current state. Hence the tests also haven't been updated for 2). It's sufficient for my simple use case: my personal site. So hopefully this will be of use to others as well. Also, I hope the concerns raised here will be taken into account for the final D8 solution.

catch’s picture

Issue tags:-Perfomance+Performance

Fixing tag.

Wim Leers’s picture

Version:8.x-dev» 7.x-dev
StatusFileSize
new11.27 KB
FAILED: [[SimpleTest]]: [MySQL] 37,887 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Reroll in which 1) has a bug fixed and 2) doesn't perform the same computation twice.
As such, no longer breaks Views.

(Yes, I'm just hacking around.)

Also changed the version to D7 (forgot to do that in #13).

RobLoach’s picture

Status:Needs work» Needs review

Bot?

Status:Needs review» Needs work

The last submitted patch, only-js-settings-dont-add-jquery_15.patch, failed testing.

Wim Leers’s picture

As I said (see #13), I haven't updated the tests. This won't go in anyway. There's no point in testing it; it's merely a partial solution.

jcisio’s picture

Version:7.x-dev» 8.x-dev
Component:base system» javascript
catch’s picture

While this isn't the prettiest patch in the world, I think it's a sufficiently bad regresssion loading the JS on pages that absolutely don't need it that we should try to get it into 8.x and backported to D7. I've proposed much weirder changes to fix PHP performance regressions in D7 many times over.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new4.74 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

First try to see what testbot has to say

attiks’s picture

ToDo: clean whitespace

Status:Needs review» Needs work

The last submitted patch, i1279226-21.patch, failed testing.

attiks’s picture

FYI, I tried a manual install and got

Additional uncaught exception thrown while handling exception.
Original

Drupal\Core\Database\ConnectionNotDefinedException: The specified database connection is not defined: default in Drupal\Core\Database\Database::openConnection() (line 370 of /home/quickstart/websites/d8.dev/core/lib/Drupal/Core/Database/Database.php).
Additional

Drupal\Core\Database\ConnectionNotDefinedException: The specified database connection is not defined: default in Drupal\Core\Database\Database::openConnection() (line 370 of /home/quickstart/websites/d8.dev/core/lib/Drupal/Core/Database/Database.php).

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new4.83 KB
FAILED: [[SimpleTest]]: [MySQL] 35,341 pass(es), 14 fail(s), and 64 exception(s).
[ View ]

Another one

Status:Needs review» Needs work

The last submitted patch, i1279226-25.patch, failed testing.

attiks’s picture

I ran into the first problem with testbot, some tests are defined like

  $javascript = drupal_add_js('core/misc/collapse.js');

and it uses the return value to run the tests against, but libraries are only added inside drupal_get_js, but that function uses drupal_render, something the testbot doesn't really like :/

attiks’s picture

Assigned:Wim Leers» attiks

@Wim, if you want it back, let me know ;p

nod_’s picture

And the rest of the theme_token errors comes from this bit in drupal_web_test_case.php:2768

<?php
   
if (preg_match('/jQuery\.extend\(Drupal\.settings, (.*?)\);/', $content, $matches)) {
     
$this->drupalSettings = drupal_json_decode($matches[1]);
    }
?>

that suppose settings are on all pages, since we removed that it's not the case anymore.

attiks’s picture

Assigned:attiks» Unassigned
Status:Needs work» Needs review
StatusFileSize
new13.68 KB
FAILED: [[SimpleTest]]: [MySQL] 35,346 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

New patch, still failing on some tests

attiks’s picture

Assigned:Unassigned» attiks

and some tests contain some ugly code

Status:Needs review» Needs work

The last submitted patch, i1279226-30.patch, failed testing.

ericduran’s picture

Assigned:attiks» Unassigned
Status:Needs work» Needs review
StatusFileSize
new4.92 KB
FAILED: [[SimpleTest]]: [MySQL] 33,251 pass(es), 185 fail(s), and 410 exception(s).
[ View ]

So I took a different approach. It seem to me like the real problem is the location the settings are being aggregated. It also seems really hard to do the logic we want with the current flow of the drupal_add_js function.

So instead I separated out the settings to a separate function drupal_js_settings() which handles aggregating the settings and returning it.

I also try to not touch drupal_get_js as the logic in there is sane and it's already accounting for when there isn't any actual javascript files. So instead I delay the retrieval of the settings files till after drupal_get_js checks if there are any JS files.

I didn't do any serious testing but the manual test I did this worked pretty good. I'm sure this will failed some test so lets see what needs to be cleaned up.

I'll love some feedback on this approach.

ericduran’s picture

StatusFileSize
new4.93 KB
FAILED: [[SimpleTest]]: [MySQL] 33,334 pass(es), 119 fail(s), and 116 exception(s).
[ View ]

Same exact thing plus a small tiny fix.

ericduran’s picture

StatusFileSize
new5.6 KB
FAILED: [[SimpleTest]]: [MySQL] 33,352 pass(es), 55 fail(s), and 95 exception(s).
[ View ]

Ok so apparently AJax commands need to render js settings without having to add any files to the page.

Lets give this one a try.

Status:Needs review» Needs work

The last submitted patch, 1279226-35-js-loaded-everywhere.patch, failed testing.

ericduran’s picture

Getting closer but my settings are still wrong.

ericduran’s picture

Status:Needs work» Needs review
StatusFileSize
new5.64 KB
FAILED: [[SimpleTest]]: [MySQL] 33,348 pass(es), 55 fail(s), and 95 exception(s).
[ View ]

Settings the pathPrefix correctly.

Status:Needs review» Needs work

The last submitted patch, 1279226-36-js-loaded-everywhere.patch, failed testing.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new23.08 KB
PASSED: [[SimpleTest]]: [MySQL] 35,360 pass(es).
[ View ]

New patch for testbot

attiks’s picture

I like the idea of drupal_js_settings() (although I prefer drupal_add_js_settings() as name) but we need to make sure that we can still backport this to Drupal 7.

To be clear: I don't even know if my approach is (easily) backportable or not.

attiks’s picture

Some more information about the approach

  • new function _drupal_get_js that returns the structured array, drupal_get_js is just a wrapper now.
  • _drupal_get_js checks if common settings and common libraries need to be added
  • _drupal_get_js does the merge of the settings
  • you can no longer rely on the output of drupal_add_js, always use drupal_get_js or _drupal_get_js
  • there are still some whitespace problems, I would like feedback on the other remarks
+++ b/core/includes/ajax.incundefined
@@ -289,10 +289,10 @@ function ajax_render($commands = array()) {
-  $scripts = drupal_add_js();

API change: you can not longer rely on drupal_add_js to get all the data

+++ b/core/includes/bootstrap.incundefined
@@ -1937,20 +1937,25 @@ function drupal_array_merge_deep_array($arrays) {
+    if (!is_array($array)) {
+      $result[] = $array;

Possible API change: Added this to drupal_array_merge_deep_array, but this needs more testing

+++ b/core/includes/common.incundefined
@@ -4220,18 +4198,90 @@ function drupal_js_defaults($data = NULL) {
function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALSE) {

this is just a wrapper around _drupal_get_js()

+++ b/core/includes/common.incundefined
@@ -4220,18 +4198,90 @@ function drupal_js_defaults($data = NULL) {
+  if (!$libraries_added) {

common libraries and settings are only added when needed.

+++ b/core/includes/common.incundefined
@@ -4257,15 +4307,11 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    $merge = drupal_array_merge_deep_array($items['settings']['data']);
+    $items['settings']['data'] = $merge;

merge of the settings is now done by _drupal_add_js

+++ b/core/includes/common.incundefined
@@ -4355,7 +4401,7 @@ function drupal_pre_render_scripts($elements) {
-            $element['#value'] = 'jQuery.extend(Drupal.settings, ' . drupal_json_encode(drupal_array_merge_deep_array($item['data'])) . ");";
+            $element['#value'] = 'jQuery.extend(Drupal.settings, ' . drupal_json_encode($item['data']) . ");";

merge of the settings is now done by _drupal_add_js

+++ b/core/includes/common.incundefined
@@ -4423,45 +4469,47 @@ function drupal_group_js($javascript) {
+  if (is_array($javascript)) {

extra check added to drupal_group_js to make sure $javascript isn't empty.

+++ b/core/modules/file/file.moduleundefined
@@ -279,8 +279,8 @@ function file_ajax_upload() {
-  $js = drupal_add_js();
-  $settings = call_user_func_array('array_merge_recursive', $js['settings']['data']);
+  $js = _drupal_get_js();

merge of the settings is now done by _drupal_add_js

nod_’s picture

That is awesome :) I'll take some time for a review today, thanks a lot.

patch from ericduran looks simpler, but I don't think i'd be possible to backport that unfortunately :(

ericduran’s picture

Hmm, I rather do the simpler D8 patch. One thing to remember is that drupal_get_js is already different between D7 and D8 as it is now using a render array to render the scripts as apposed to D7 which had all the logic stuck in drupal_get_js.

So pretty much any change to these JS function would need to be completely re-written for D7. If we really want to keep this patch similar than maybe it might be worth back-porting the #865536: drupal_add_js() is missing the 'browsers' option patch which would bring both D7 and D8 drupal_get_js functions inline.

But I wouldn't want to derailed this issue so I'll so give the latest patch a review later today. :)

edit: hmm, it seems the other patch is already marked to be ported I forgot about that. So I guess we don't have to worry about the changes between the two version now.

attiks’s picture

StatusFileSize
new23.25 KB
PASSED: [[SimpleTest]]: [MySQL] 35,371 pass(es).
[ View ]

I cleaned some comments and white space issues

attiks’s picture

StatusFileSize
new23.95 KB
PASSED: [[SimpleTest]]: [MySQL] 35,391 pass(es).
[ View ]
new922 bytes
FAILED: [[SimpleTest]]: [MySQL] 35,390 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Basic test added, let's see if the bot likes it

nod_’s picture

Status:Needs review» Needs work

That's some good work attiks, kudos on making the tests work :)

I reviewed the two patches and I want to go with the simpler one. It feels like what the function should have been. Since it touches two files that's a nice review-friendly patch. attiks I'm worried your patch touches too much things that would be used in contrib. Would you help getting the tests working with ericduran patch please? I've seen you've made the necessary changes to the test class already :)

ericduran can you remove the unneeded url() in drupal_add_js? it's only needed in drupal_js_settings and I'd rename this function drupal_add_js_settings like attiks proposed.

Can you document the new function and copy the current url() comments to the right place?

Thanks guys that's the awesomest JS patch since forever.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new25.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1279226-48.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Some clarification on why I 'fixed' it like this:

I think part of the reason why my solution looks more complex is because I fixed some other things, that needs fixing anyway for the tests to pass:
drupal_group_js: Needs to check if $javascript isn't empty before trying to group.
drupal_array_merge_deep_array: Needs to check if $array is an array before calling the foreach.

I also made a strict distinction between drupal_add_js (adding only) and drupal_get_js (output). This is probably a rather personal feeling but it always feel strange to me to use a setter as a getter. This will probably solves problems like #1448796: ajax_render() discards settings changes from hook_js_alter()

The part of the patch that I don't like is the check for ['ajaxPageState']['css']. To fix this we also need to add a _drupal_get_css to get the structured output so we can add the css in _drupal_get_js. See attached patch, this will fail on one test (AJAXFrameworkTestCase->testLazyLoadOverriddenCSS()) for overriden css.

+++ b/core/includes/common.incundefined
@@ -4232,6 +4226,59 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+  // Decide if we need to output something.
+  // If only ['ajaxPageState']['css'] exists assume nothing is added.
+  if (isset($javascript['settings']) && count($javascript) == 1 && count($javascript['settings']['data']) == 1) {
+    if (isset($javascript['settings']['data'][0]['ajaxPageState']['css']) && count($javascript['settings']['data'][0]['ajaxPageState']) == 1) {
+      return '';
+    }
attiks’s picture

StatusFileSize
new3.37 KB
new27.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch i1279226-49.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch for drupal_get_css()

Status:Needs review» Needs work

The last submitted patch, i1279226-49.patch, failed testing.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new27.9 KB
FAILED: [[SimpleTest]]: [MySQL] 34,987 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Rerolled patch 49 against 8.x, this will fail on one test (AJAXFrameworkTestCase->testLazyLoadOverriddenCSS()) for overriden css.

Status:Needs review» Needs work

The last submitted patch, i1279226-50.patch, failed testing.

attiks’s picture

nod_’s picture

Thanks for splitting things up, that'll make things easier for this one.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new29.79 KB
PASSED: [[SimpleTest]]: [MySQL] 35,002 pass(es).
[ View ]
new23.83 KB
FAILED: [[SimpleTest]]: [MySQL] 34,861 pass(es), 40 fail(s), and 8,604 exception(s).
[ View ]

New patch that fixes #51

ericduran’s picture

StatusFileSize
new7.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1279226-55-core-js.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok so here's a somewhat simpler patch. This tries to keep all the logic inside drupal_add_js that way the return value is still the same and we don't have to change drupal_add_js anywhere.

I still haven't documented the function still trying to see what test needs to be fixed or what is still broken.

The one thing I don't like about this patch is that there's a check at the bottom of drupal_add_js and it essentially resets the settings array even if anything hasn't been added to it. We can fix it after we get a somewhat working patch.

I don't expect this to be missing much but lets see what the testbot thinks.

Status:Needs review» Needs work

The last submitted patch, 1279226-55-core-js.patch, failed testing.

ericduran’s picture

Status:Needs work» Needs review
StatusFileSize
new7.62 KB
FAILED: [[SimpleTest]]: [MySQL] 34,995 pass(es), 5 fail(s), and 4 exception(s).
[ View ]

Ok hopefully this will be correct, I had to rerolled.

ericduran’s picture

StatusFileSize
new7.83 KB
PASSED: [[SimpleTest]]: [MySQL] 35,004 pass(es).
[ View ]

Ok here's another tried. I miss script_path which was just added recently.

ericduran’s picture

StatusFileSize
new8.93 KB
PASSED: [[SimpleTest]]: [MySQL] 34,989 pass(es).
[ View ]

Here's a documented patch.

ericduran’s picture

The last patch has some differences than the previous patch.

I really try to keep the changes to a minimum and touching as little as possible. Here's an overview:

- Added a new drupal_add_js_settings function to use as storage for JavaScript settings.
- Allow drupal_add_js_settings to return settings even if no files have been added. This is required for ajax_render.
- Instead of merging the settings and the javascript in drupal_get_js I merged it right at the end of drupal_add_js. This makes sure drupal_add_js returns the same exact values it used to return before except in the case where no JS is added.
- Had to change 2 test and added 1 to make sure no JS is added unless explicitly stated.

attiks’s picture

Status:Needs review» Needs work
+++ b/core/includes/common.incundefined
@@ -4072,69 +4072,115 @@ function drupal_add_js($data = NULL, $options = NULL) {
+    $scriptPath = $GLOBALS['script_path'];
+    $pathPrefix = '';
+    url('', array('script' => &$scriptPath, 'prefix' => &$pathPrefix));
+    $js_settings['settings'] = array(
+      'data' => array(
+        array('basePath' => base_path()),
+        array('scriptPath' => $scriptPath),
+        array('pathPrefix' => empty($prefix) ? '' : $prefix),

typo: $prefix isn't defined, should be $pathPrefix

ericduran’s picture

Status:Needs work» Needs review
StatusFileSize
new8.91 KB
PASSED: [[SimpleTest]]: [MySQL] 35,020 pass(es).
[ View ]

Thanks also got rid of the useless ternary.

attiks’s picture

StatusFileSize
new10.76 KB
FAILED: [[SimpleTest]]: [MySQL] 35,006 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new2.32 KB
PASSED: [[SimpleTest]]: [MySQL] 35,006 pass(es).
[ View ]

After inspecting the patch it seems that you cannot just add a setting without adding a file. First patch should pass, second should fail.

nod_’s picture

This is awesome :) I'll review the patch properly after the testbot gives the green light.

Status:Needs review» Needs work

The last submitted patch, i1279226-64.patch, failed testing.

ericduran’s picture

Status:Needs work» Needs review

@attiks I did this on purpose. The settings are useless without a file being added first. I'm pretty sure the settings would fail without adding a file because Jquery.extend wont be on the page.

In the case of the ajax_render it hits drupal_add_js_settings directly which helps it retrieve the settings without having to add a file. So for ajax request you can add settings without adding a files, but if you want to render it on the page you need to add a file. This makes sense to me.

attiks’s picture

@eric maybe add it somewhere in the comments, so people know they have to add at least one file?

nod_’s picture

So the test needs to be changed from assertTrue to assertFalse (or something) in testPageWithOnlyJavascriptSetting to please the test bot and since we can't add js settings by themselves anymore.

Like attiks said, this needs to be in the comments. Possibly a change notice too. Once the patch is good to go I'll let the doc people all over this to make sure they won't have to clean up later.

Beside this small issue, it looks great :)

attiks’s picture

From #48:

I also made a strict distinction between drupal_add_js (adding only) and drupal_get_js (output). This is probably a rather personal feeling but it always feel strange to me to use a setter as a getter. This will probably solves problems like #1448796: ajax_render() discards settings changes from hook_js_alter()

I think with @eric's patch this means that the problem still exists, drupal_alter_js isn't going to be called.

nod_’s picture

I hear what you're saying, the scope of this issue is well defined and what you're addressing is your patch is simply out of scope. The problem was there before and it'll stay after. Also we have an issue to scrap the whole JS aggregation thing and replace it with something better, so quick and dirty (and potentially backportable) is good here.

Please submit a patch for the other issue, i'll be happy to review it :)

attiks’s picture

@nod_ the fix for the other issue basically ends up being something like my patch in this issue, since drupal_add_js can no longer add anything by default and drupal_get_js has to add it, siince that's where drupal_alter_js gets called. But let us focus first on this issue ;p

Regarding the test from #64 I would leave it out, I only added it to demonstrate the problem.

Eric's patch from #63 looks good to me, what else needs to be done to RTBC this?

attiks’s picture

#63: 1279226-63.patch queued for re-testing.

sun’s picture

Assigned:Unassigned» sun
StatusFileSize
new12.66 KB
FAILED: [[SimpleTest]]: [MySQL] 36,276 pass(es), 25 fail(s), and 5 exception(s).
[ View ]

I wasn't really happy with the approaches taken here. While @ericduran's made some more sense, the entire problem space can be simplified.

There was one larger mistake with all of the patches: html5shiv.js has no dependency on Drupal or jQuery and needs to be always loaded, because it provides the backwards-compatibility layer for CSS styling.

Note that there is a pretty big comment + @todo framed as question in drupal_get_js(). Namely, whether it wouldn't make even more sense to only add the default libraries in template_process_page(), essentially moving the check for emptiness and conditional addition over there. That is, because these libraries only make sense on an HTML page.

In turn, that would heavily simplify drupal_add_js(), since nothing is loaded by default anymore. Settings can be added anytime, but would not be output if there's no other JS.

Status:Needs review» Needs work

The last submitted patch, drupal8.js-defaults.74.patch, failed testing.

sun’s picture

Title:jquery.js, jquery.once.js and drupal.js added even when no JS code is added through drupal_add_js()» jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page
Status:Needs work» Needs review
Issue tags:-Needs tests+frontend performance
StatusFileSize
new12.52 KB
FAILED: [[SimpleTest]]: [MySQL] 36,302 pass(es), 11 fail(s), and 0 exception(s).
[ View ]

I like.

This still looks backportable to me.

Status:Needs review» Needs work

The last submitted patch, drupal8.js-defaults.75.patch, failed testing.

sun’s picture

Spin-off: #1558706: Make CommonJavaScriptTestCase faster (Novice)

I can only guess that most failures in CommonJavaScriptTestCase are merely caused by assertions looking for the previously hard-coded library files and weights.

The AJAXFrameworkTestCase failure is more concerning, needs analysis.

nod_’s picture

If we go this way, might as well go all the way and put everything in the preprocess function like you suggested. Having a drupal_add_js_page_defaults that doesn't output the same thing all the time isn't very "defaults". The name could be changed a little.

That is another related issue, we could get rid of $.extend() to merge settings, we do merge them on the PHP side and the ajax command can merge them all it want. I'm pretty sure we don't actually need this merge and merging isn't free. a Drupal = Drupal || {settings:{}}; before and some weight magic, would solve the problem $.extend is trying to solve.

That would make things easier and let us output settings on all pages without requiring jQuery, simplifying this part of the mess. Also there are use-cases where settings would be helpful but not the rest of the JS (I would actually use it right now if that was available).

That would be nice but I'm pretty sure that wouldn't be backportable.

ericduran’s picture

Hmm, moving that to template_process_html doesn't feel backportable to me. I'll try and test this.

Wim Leers’s picture

Review + patch reroll for #76 with test fixes coming up in a few hours. (Posting this to prevent double work.)

Wim Leers’s picture

Status:Needs work» Needs review
StatusFileSize
new17.31 KB
FAILED: [[SimpleTest]]: [MySQL] 36,496 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Here's an overview/assessment of this entire issue.

1. I apologize for the mess in the first 18 comments in this issue. I started working on this on September 14, 2011. This was a few days before I had to leave for my internship at Facebook, and I wanted to get my new personal site out there. Hence I had to resort to quick ugly hacks. Also, this was for D7, which is why people pretty much had to start over for the D8 patch.

2. The early D8 patches by @attiks in #21–#30 seem unnecessarily complex. I like the simpler approach @ericduran took in #33–#36. I also like @attiks' enthusiasm in taking @ericduran's patch and getting testbot approval, but I dislike the checking for the presence of "ajaxPageState". We should instead make sure that "ajaxPageState" is only added when needed.

3. @sun's take in #74 really resonated with me. The patch in #76 looks most excellent.

4. @nod_'s review of @sun's patch is useful in getting this done. I agree that drupal_add_js_page_defaults() is somewhat of a misnomer. drupal_add_js_html.

Note: a problem that still exists in @sun's patch: when adding e.g. Google Analytics, other JS files (Drupal/jQuery libs) will still be loaded. I solved this in my original patch in a very hacky, non-universal way. Solving this properly is probably beyond the scope of this issue; we'll need a JS dependency system for that.

Reviewed + rerolled patch which passes all CommonJavaScriptTestCase tests. It still fails the AJAXFrameworkTestCase, but I've found why: see the review below.

+++ b/core/includes/common.inc
@@ -4132,10 +4118,43 @@ function drupal_add_js($data = NULL, $options = NULL) {
+  // If any JavaScript (except settings) has been added, include the required
+  // base libraries.
+  if (array_diff_key($javascript, array('settings' => 0))) {
+    drupal_add_library('system', 'jquery', TRUE);
+    drupal_add_library('system', 'jquery.once', TRUE);
+
+    drupal_add_js('core/misc/drupal.js', array(
+      'group' => JS_LIBRARY,
+      'every_page' => TRUE,
+      'weight' => -1,
+    ));

This is where the aforementioned "Google Analytics JS" problem presents itself: this is a piece of inline JS that doesn't depend on Drupal's JS nor on jQuery. Yet it still triggers this code path to be executed.

Not good.

To solve it properly, we need a JS dependency system (out of scope for this issue). The only way I can think of to solve this problem without that, is by scanning the code for for references to jQuery or Drupal. Obviously, that won't work very reliably.

Conclusion: there's no way to backport any "proper solution" to D7.

The only possible "solution" in D7 is to have each module that adds some JS that doesn't depend on the jQuery/Drupal JS libs to alter the JS at the very latest step and check if any other JS has been added, if not, remove the jQuery/Drupal JS libs. Except that is impossible too, because drupal_add_js_page_defaults() is called from template_process_html()…

+++ b/core/includes/common.incundefined
@@ -4132,10 +4118,43 @@ function drupal_add_js($data = NULL, $options = NULL) {
+  // Unconditionally add the html5shiv to all pages; it is required for
+  // backwards-compatible CSS styling and has no dependencies.

When desired, this can easily be overridden using hook_js_alter(): great.

+++ b/core/includes/theme.incundefined
@@ -2663,7 +2663,10 @@ function template_process_html(&$variables) {
+  // Conditionally add the default Drupal/jQuery libraries to the page.

What we're really doing here, is adding dependencies for other JS. Currently, they're hardcoded, but that should change in the future.
Hence I propose drupal_add_js_dependencies() as a better name.

More importantly, this appears to break the adding of new JS files in AJAX callbacks. I haven't dissected the entire theme/render system, but I suspect template_process_html() is not called in this case.
I.e., this is what breaks the AJAXFrameworkTestCase. More specifically, look at ajax_forms_test_lazy_load_form_submit(). Verify by adding a debug($return); in DrupalWebTestCase::drupalPostAJAX().

Status:Needs review» Needs work

The last submitted patch, drupal8.js-defaults.82.patch, failed testing.

attiks’s picture

my 2c

The ajaxPageState was 'solved' in my latest patch, but I had to split drupal_get_css into 2 parts (drupal_get_css and _drupal_get_css) the same way as my patch does for drupal_get_js. This also solves the problem you have by moving the logic to template_process_html, because it isn't used for AJAX, that's why I've put everything inside _drupal_get_js.

Regarding the problem with GA, maybe it can be solved if the GA module indicates it doesn't need the libraries, by specifying a new option to drupal_get_js, so it can see if it needs to add the default libraries and settings or not. This allows us to backport this patch to D7 while offering a solution for the few contrib modules that add javascript without the need of anything else.

// GA
drupal_add_js($data, array('purejs' => TRUE)); // Default is FALSE

Adding html5shiv is something is missed, but is needed.

If I find some time I'll try to work on this.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new28.68 KB
FAILED: [[SimpleTest]]: [MySQL] 36,457 pass(es), 25 fail(s), and 12 exception(s).
[ View ]
new11.27 KB
FAILED: [[SimpleTest]]: [MySQL] 36,391 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Another patch with support for the 'purejs' setting and no longer the need for #1554122: drupal_group_js doesn't check if $javascript is empty or #1554142: drupal_array_merge_deep_array assumes $array is actually an array.

All heavy work is done inside _drupal_get_js which calls drupal_add_js_page_defaults. This function loops through the javascript to see if the default libraries have to be added or not. If only a 'purejs' file is added it only adds html5shiv, othewise it adds all default settings and libraries.

If nothing is added the html5shiv is added by drupal_get_js, so it only is outputted on HTML pages. I opted for this approach because html5shiv is presentation.

The ajax framework doesn't use drupal_get_js/drupal_get_css but it uses _drupal_get_js/_drupal_get_css. So the first pair of functions is for HTML output, the second pair returns arrays that can easily be manipulated.

The ajaxPageState settings for both js and css are added by _drupal_get_js, because they only need to be added if there's any other javascript added to the page.

Status:Needs review» Needs work

The last submitted patch, i1279226-85.patch, failed testing.

attiks’s picture

Adding the html5shiv inside drupal_get_js was not a good idea, AJAX is still calling drupal_get_js and outputting it.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new27.3 KB
FAILED: [[SimpleTest]]: [MySQL] 36,514 pass(es), 0 fail(s), and 46 exception(s).
[ View ]

Ugly fix, just to make sure this was the only problem

Status:Needs review» Needs work

The last submitted patch, i1279226-88.patch, failed testing.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new28.21 KB
PASSED: [[SimpleTest]]: [MySQL] 36,520 pass(es).
[ View ]
attiks’s picture

Probably of topic but isn't html5shiv supposed to be in the theme, the more I think about it, the more sense it makes to put html5shiv inside the html.tpl.php file, since it's solely meant for HTML and has to be loaded on all pages. If people want to change or remove it they can just change the html.tpl.php inside their theme. If they want to use a newer version or another solution they can do it the same way.

Related issue #1077878: Add HTML5shiv to core

Edit: link fixed.

catch’s picture

htmlshiv isn't the only file that gets needlessly added for non-HTML pages, we also need to move module css/js additions out of system_init().

attiks’s picture

Status:Needs review» Needs work

@catch, I asked for html5shiv because I added the following code, which isn't pretty. I agree with your comment, so I created a feature request #1565262: Only add js/css if the output is HTML

+++ b/core/includes/common.incundefined
@@ -4190,18 +4225,61 @@ function drupal_js_defaults($data = NULL) {
+    // Unconditionally add the html5shiv to all pages; it is required for
+    // backwards-compatible CSS styling and has no dependencies.
+    // @TODO: find a better way
+    if ($scope == 'header' && !isset($_POST['ajax_page_state'])) {
+      if ($library = drupal_get_library('system', 'html5shiv')) {
+        if ($library['js']) {
+          $js = $library['js'];
+          $key = key($js);
+          $js[$key]['data'] = $key;
+          $js[$key]['type'] = 'file';
+          $js[$key]['preprocess'] = FALSE;
+          $js[$key]['cache'] = TRUE;
+          $items = $js;
+        }
+      }

ugly code

attiks’s picture

Status:Needs work» Needs review
nod_’s picture

Priority:Normal» Major

Worst performance bug we have on frontend right now.

nod_’s picture

hey sun, still on your radar? That'd be nice to get this in D8.

RobLoach’s picture

I'm not sure how I feel about that "jsonly" flag.

RobLoach’s picture

Status:Needs review» Needs work

Either way, the patch would need to be updated.

sun’s picture

Issue tags:-needs backport to D7

I really do not understand - despite very elaborate reviews and extensive reasoning - why we're hopping back and forth and back between fundamentally different patches and solution proposals? The "two cents" mentioned in #84 do not invalidate the approach taken. The result is zero progress.

I'm removing the needs backport tag, since at least @nod_ and me seem to be in common agreement on several other issues that it's time to stop caring for D7 and make more speedy progress on D8 instead.

Please note that this does not mean that this patch cannot be backported in the end. However, it means that we're changing the fundamental requirements for resolving this issue: Don't care for "backportability" to D7, find the best solution for D8 instead. Normally, that's actually a given for all issues, but especially for this issue, I've the impression that all patches so far cared too much for finding a solution that can be backported cleanly.

Will try to re-roll @Wim's latest patch against HEAD in a minute.

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new17.62 KB
FAILED: [[SimpleTest]]: [MySQL] 37,012 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Sorry, just a plain re-roll. Tried to go and debug further than that, but got heavily distracted.

Status:Needs review» Needs work

The last submitted patch, drupal8.js-defaults.100.patch, failed testing.

nod_’s picture

Version:8.x-dev» 7.x-dev
Priority:Major» Normal

D8 needs to properly solve that, it's happening in #1737148: Explicitly declare all JS dependencies, don't use drupal_add_js.

sun’s picture

Assigned:sun» Unassigned
attiks’s picture

Status:Needs work» Needs review

So now we need to decide if this can and will be backported to D7, @catch?

catch’s picture

Assigned:Unassigned» David_Rothstein

That's not up to me, but I think this is an issue where it's worth kicking over to David before the patch gets to RTBC for a look at the general approach here. The D8 solution is impossible to backport, so it's whether we can find a Drupal 7 solution that's non-intrusive/acceptable enough to fix the bug.

Wim Leers’s picture

I don't think it's possible to do a non-intrusive D7 solution. D7 contrib assumes Drupal.js and jquery.js are loaded on every page.

Partial solutions are possible (that's what you call "acceptable enough" I think, @catch), complete solutions not. The simplest approach possible in D7 is what I did in #15, IIRC.

attiks’s picture

#106 we can make it an optional behaviour, so by default it works like it is now, but by setting an option (inside settings.php) you can override it.

I think we should solve this for D7 as well because it's killing performance on mobile devices and it's at least a year before D8 arrives.

Wim Leers’s picture

#107: that's *genius*. It's not perfect, but it gets the job done. Does anybody strongly oppose?

nod_’s picture

Here is a big +1 from me.

seutje’s picture

@#107: makes sense to me, on a mobile-heavy project, you're usually already watching rather closely what js contrib is dropping in the page and you could easily know beforehand if changing that option would break stuff.

David_Rothstein’s picture

Assigned:David_Rothstein» Unassigned

Sorry for not commenting here earlier, but yeah, #107 definitely seems like it nails it. It's really sad if we can't fix this any other way for Drupal 7, but I guess it's true that people might be pasting <script> tags into node bodies and other terrible things that rely on jQuery always being there and we can't remove that out from under them. But making it a setting is definitely safe.

I'd go so far as to say this should probably be a checkbox on the performance page, actually? (Rather than just something hidden away in settings.php.)

I haven't really gotten my head around the actual patches in this issue though :)

attiks’s picture

#111 Can you have a look at the patch?

attiks’s picture

StatusFileSize
new18.85 KB
FAILED: [[SimpleTest]]: [MySQL] 39,913 pass(es), 56 fail(s), and 24,445 exception(s).
[ View ]

New patch based on @Sun's latest D8 patch

It allows you to set a variable javascript_always_use_jquery (default TRUE) to disable to automatic adding of jquery.js

For themes there's a new tag for the info file: scripts_special[], this allows theme's to include polyfills without triggering the loading of jquery

attiks’s picture

Reminder to self: add tests for javascript_always_use_jquery and scripts_special

RobLoach’s picture

Reminder to self: Remove the javascript_always_use_jquery variable. We don't need JavaScript on every page, and having a variable just for jQuery use seems quite silly.

Status:Needs review» Needs work

The last submitted patch, i1279226-113.patch, failed testing.

attiks’s picture

#115 If only ... we need it otherwise we might break themes that have their javascript (depending on jQuery) inside tpl.php files.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new20.21 KB
FAILED: [[SimpleTest]]: [MySQL] 40,019 pass(es), 48 fail(s), and 12,148 exception(s).
[ View ]
new2.02 KB

go bot

Status:Needs review» Needs work
Issue tags:-Performance, -frontend performance, -WPO

The last submitted patch, i1279226-118.patch, failed testing.

attiks’s picture

Status:Needs work» Needs review
Issue tags:+Performance, +frontend performance, +WPO

#118: i1279226-118.patch queued for re-testing.

RobLoach’s picture

+++ b/includes/common.incundefined
@@ -4104,71 +4109,46 @@ function drupal_region_class($region) {
  *     file will be aggregated. Defaults to TRUE.
+ *   - need_jquery: If TRUE jquery.js and drupal.js are added automatically.

This is why we introduced the "dependencies" key of hook_library_info. I thoguht we removed all calls to drupal_add_js()? Could we add library_dependencies[] or something like it to .info files?

attiks’s picture

#121 That's true for Drupal 8, but this is for Drupal 7 ;-)

Status:Needs review» Needs work

The last submitted patch, i1279226-118.patch, failed testing.

RobLoach’s picture

Ah, thanks for the explanation ;-) . The jQuery dependency in Drupal 7 does seem like almost a must at this point.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new18.98 KB
FAILED: [[SimpleTest]]: [MySQL] 40,349 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
new2.55 KB

#124 It sure is, performance on mobile is way better, and even on desktop there's some improvements.

new patch still a problem with ajax and some strange error in dblog.

Status:Needs review» Needs work

The last submitted patch, i1279226-125.patch, failed testing.

attiks’s picture

Only 5 left, I'm done for today so feel free to help if you can ;-)

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new21.02 KB
FAILED: [[SimpleTest]]: [MySQL] 40,434 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
new2.98 KB

Javascript tests should be fixed.

Status:Needs review» Needs work

The last submitted patch, i1279226-127.patch, failed testing.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new21.01 KB
FAILED: [[SimpleTest]]: [MySQL] 40,385 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.77 KB

Reroll to remove 'core' in a test, fix notices

Still one failing test in AJAX framework: "Page state now has the modules/system/system.js file. - ajax.test - 195 - AJAXFrameworkTestCase->testLazyLoad()"

@WimLeers, @Sun any ideas?

Status:Needs review» Needs work

The last submitted patch, i1279226-130.patch, failed testing.

attiks’s picture

I tested this manually both as logged in user and as anon user and it works, so the test in't working :/

Ajax response

0: {command:settings, settings:{basePath:/, pathPrefix:,…}, merge:true}
command: "settings"
merge: true
settings: {basePath:/, pathPrefix:,…}
ajaxPageState: {theme:bartik, theme_token:GXQYLJH8QhIjbtJhg3ErmV3lvI6bMK4rJ-md0jz9g8o,…}
ajax_forms_test_lazy_load_form_submit: "executed"
basePath: "/"
pathPrefix: ""
1: {command:insert, method:prepend, selector:head,…}
command: "insert"
data: "<style type="text/css" media="all">@import url("http://langfall2.ubuntu006.attiks.office/modules/system/system.admin.css?mmbkbb");</style>↵"
method: "prepend"
selector: "head"
settings: null
2: {command:insert, method:prepend, selector:head,…}
command: "insert"
data: "<script type="text/javascript" src="http://langfall2.ubuntu006.attiks.office/modules/system/system.js?mmbkbb"></script>↵"
method: "prepend"
selector: "head"
settings: null

Submitting the form a second time behaves strange, the system.js is returned again:

0: {command:settings, settings:{basePath:/, pathPrefix:,…}, merge:true}
command: "settings"
merge: true
settings: {basePath:/, pathPrefix:,…}
ajaxPageState: {theme:bartik, theme_token:n0A3O_1mLNkOQZmmDFtzGGIx228XciJpnGJZXQsDlns}
ajax_forms_test_lazy_load_form_submit: "executed"
basePath: "/"
pathPrefix: ""
1: {command:insert, method:prepend, selector:head,…}
command: "insert"
data: "<script type="text/javascript" src="http://langfall2.ubuntu006.attiks.office/modules/system/system.js?mmbkbb"></script>↵"
method: "prepend"
selector: "head"
settings: null
attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new22.15 KB
PASSED: [[SimpleTest]]: [MySQL] 40,367 pass(es).
[ View ]
new1.31 KB

Work around added for ajax, drupal_get_js has an extra optional parameter to force the creation of ajaxPageState.

Status:Needs review» Needs work
Issue tags:-Performance, -frontend performance, -WPO

The last submitted patch, i1279226-134.patch, failed testing.

attiks’s picture

Status:Needs work» Needs review
Issue tags:+Performance, +frontend performance, +WPO

#134: i1279226-134.patch queued for re-testing.

attiks’s picture

let's blame the bot, failing tests passed locally

attiks’s picture

#134 is green, any other concerns?

attiks’s picture

I did some quick tests using a site with the picture module, both css and js aggregation on. With the page I'm saving 3 requests for js files and ±50KB, win!

Page speed scores are around 90/100, which could be improved by optimizing images: https://developers.google.com/speed/pagespeed/insights#url=http_3A_2F_2F...

Off-topic
One interesting remark about the use of ?itok: "Resources with a "?" in the URL are not cached by some proxy caching servers"
https://developers.google.com/speed/pagespeed/insights#url=http_3A_2F_2F...

attiks’s picture

StatusFileSize
new24.06 KB
PASSED: [[SimpleTest]]: [MySQL] 40,397 pass(es).
[ View ]
new1.73 KB

New patch

  • Renaming $files to $aggregate_files because $files is used for something else as well.
  • Added an extra check in drupal_build_js_cache to make sure we have content to add.
attiks’s picture

FYI: I created an issue for Advanced CSS/JS Aggregation: #1989710: Add drupal_add_js_page_defaults

mikeytown2’s picture

Thanks for the patch, I've committed it to AdvAgg. A potential issue with this patch is the usage of hook_ajax_render_alter() in other modules. I did a search and most of the implementations of it seem fine (#1989710-4: Add drupal_add_js_page_defaults). The one that would need testing is http://drupal.org/project/dialog/ - dialog.module

attiks’s picture

#142 All modules using hook_library (as dialog does) will be fine, since there's an option 'need_jquery' (default TRUE) that forces the loading of jQuery no matter what.

mjpa’s picture

Status:Needs review» Needs work

Tentatively setting to needs work as I'm not sure it's related (or a valid issue) but..

With the patch applied, the ajaxPageState setting is still added even if the misc/ajax.js file is not added - if we're clearing up the JS that Drupal adds, would it not make sense to only add the ajaxPageState setting when needed?

attiks’s picture

Did you test this with the variable set? If you don't set the variable it works as without the patch.

attiks’s picture

Status:Needs work» Needs review
mjpa’s picture

I tried with javascript_always_use_jquery set to FALSE - I can't see any others mentioned in the patch.

From the patch, the ajaxPageState is added if there are non-default JS files added, but isn't the ajaxPageState only there for ajax.js? If it is, shouldn't it only be added if ajax.js is added to the page?

attiks’s picture

It should only be added when needed, but that's a different issue. From the moment settings are outputted it will output the page state as well.

If you want this to be changed, create a new issue, because trying to solve this is in this issue will make it harder to get this committed.

no_angel’s picture

mjpa’s picture

attiks: that's why I tentatively put this issue to needs work - I accept it's a separate issue.

I would create an issue for it but from my experience of the core issue queue, it's a waste of time creating an issue for something small...

attiks’s picture

#150 I think you're right, it probably will be a waste of time :/

attiks’s picture

Issue summary:View changes

Minor clarification.

attiks’s picture

Issue summary:View changes

Updated to reflect the patch

attiks’s picture

Issue summary:View changes

typos and clarification

attiks’s picture

Issue summary updated, feel free to improve

attiks’s picture

Update so maybe somebody can review this

attiks’s picture

Priority:Normal» Major
mikeytown2’s picture

Status:Needs review» Needs work

Strict warning: Only variables should be assigned by reference in drupal_add_js_page_defaults() $javascript = &drupal_add_js();. Using $javascript = &drupal_static('drupal_add_js', array()); makes the error go away.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new24.08 KB
FAILED: [[SimpleTest]]: [MySQL] 40,387 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new1.66 KB

#155 New patch, interdiff contains some more lines because of offset

Status:Needs review» Needs work
Issue tags:-Performance, -frontend performance, -WPO, -Needs change record

The last submitted patch, i1279226-156.patch, failed testing.

attiks’s picture

Status:Needs work» Needs review

#156: i1279226-156.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Performance, +frontend performance, +WPO, +Needs change record

The last submitted patch, i1279226-156.patch, failed testing.

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new24.1 KB
PASSED: [[SimpleTest]]: [MySQL] 40,451 pass(es).
[ View ]
new503 bytes

added extra check

Wim Leers’s picture

Do we still think this is realistic to get in Drupal 7? David Rothstein, are you open to it?


Patch no longer applies FWIW.

Wim Leers’s picture

Issue summary:View changes

$need_ajaxPageState added as API change

attiks’s picture

StatusFileSize
new24.06 KB
PASSED: [[SimpleTest]]: [MySQL] 41,058 pass(es).
[ View ]

Quick - locally untested - reroll for testbot

achton’s picture

Issue summary:View changes

Fixed a few typos in summary.

Fabianx’s picture

Status:Needs review» Reviewed & tested by the community

It looks great to me, I think there is no real BC break, if you use agg modules, you probably have JS in every page anyway.

attiks’s picture

David, can you provide feedback, is this acceptable for D7?

attiks’s picture

David, can you provide feedback, is this acceptable for D7?

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 162: i1279226-162.patch, failed testing.

Status:Needs work» Needs review

Fabianx queued 162: i1279226-162.patch for re-testing.

mikeytown2’s picture

Status:Needs review» Reviewed & tested by the community

#162 passes tests. Waiting on David_Rothstein

David_Rothstein’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new13.99 KB
FAILED: [[SimpleTest]]: [MySQL] 41,069 pass(es), 13 fail(s), and 0 exception(s).
[ View ]
new18.64 KB
FAILED: [[SimpleTest]]: [MySQL] 41,111 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

I looked this over and am a bit confused. I thought from #107 onward there was agreement that we shouldn't change the default behavior of Drupal 7 (adding jQuery to all pages), but rather only provide an optional setting that allows it to be changed? But the patch changes the default behavior pretty significantly, as can be seen from the tests (but then provides an optional setting that allows changing it even more)... This looks like it will definitely break things - any site adding JavaScript in a template file, a node body, etc., may be relying on jQuery being there regardless of whether jQuery is actually needed by anything that calls drupal_add_js().

I also think the "scripts_special" changes (for .info files) look a bit out of scope for this issue. Adding JavaScript that doesn't require jQuery is not necessarily that common; do we really need a special way for people to do that in an .info file vs. just having them call drupal_add_js() directly? At the very least, can we discuss that in a separate followup issue?

I think the new optional setting to drupal_add_js() looks nice.

Overall, I guess I am just confused why this needs such a major refactoring to achieve. After looking at this issue in some depth now, I wrote the attached patch instead which is a lot simpler (the size is almost entirely due to the tests). It just does this:

  1. In drupal_add_js(), do not add jQuery/etc if the JavaScript item being added doesn't require jQuery and if the 'javascript_always_use_jquery' variable is set to FALSE.
  2. JavaScript settings are marked as not requiring jQuery by default. However, since they don't work without jQuery, drupal_get_js() makes sure to return nothing if the only things that have been added are JavaScript settings.

Is there anything wrong with this much simpler approach? So far the tests seemed to pass/fail as expected when I did it locally. (The tests are based pretty heavily on the test cases from the earlier patch, although I expanded them a bit.)

On top of this I'd still love to see a UI on the performance page for setting the 'javascript_always_use_jquery' variable, but that can certainly be a followup issue also.

The last submitted patch, 170: drupal-1279226-170-TESTS-ONLY.patch, failed testing.

attiks’s picture

Status:Needs review» Reviewed & tested by the community

#171

- OK to discuss scripts_special in a follow up
- Approach looks good to me
- Let's handle the UI in a follow up as well, although it will be a dangerous setting

David_Rothstein’s picture

Thanks for the review! I think we need to hold on to this one for a little bit longer to see if there are any more reviews (since the patch changed pretty significantly compared to the previous ones).

But hopefully we should be able to get it committed early in the next release cycle.

attiks’s picture

Status:Reviewed & tested by the community» Needs work

Tested on a multi lingual site and getting the following error: Notice: Undefined index: type in locale_js_alter() (line 911 of /data/disk/o1/static/attiks2/modules/locale/locale.module).

attiks’s picture

To clarify, if your site uses multiple languages and to interface language is not the default language, jQuery will always be loaded. Once this is committed I'll create a new issue to solve this.

David_Rothstein’s picture

Status:Needs work» Needs review

Thanks for the additional testing. How did you reproduce the PHP notice exactly? I just tried with a multilingual site now and didn't see it anywhere... and looking at the code it's not obvious to me why the changes here would result in a missing 'type' in any of the JavaScript items, so I'm stumped.

I also was able to get jQuery to not be loaded on such a site. To clarify, I had English as the default language and URL prefix detection set up so that the site would display in Spanish. Visiting the Spanish URL as an anonymous user still did not load jQuery when I had the 'javascript_always_use_jquery' variable set to 0. But it sounds like your setup was a bit more complicated? I definitely suspect that there are JavaScript items in core that are "requiring jQuery" after this patch that don't strictly need to be, and yes, removing those would be good to do (here if it's obvious, or else in a followup).

attiks’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC, it was my base theme (omega 4) that did some javascript handling, tested with other themes and no more errors.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 170: drupal-1279226-170.patch, failed testing.

Status:Needs work» Needs review
Fabianx’s picture

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 170: drupal-1279226-170.patch, failed testing.

Status:Needs work» Needs review

attiks queued 170: drupal-1279226-170.patch for re-testing.

attiks’s picture

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 170: drupal-1279226-170.patch, failed testing.

Status:Needs work» Needs review

attiks queued 170: drupal-1279226-170.patch for re-testing.

attiks’s picture

Status:Needs review» Reviewed & tested by the community

Back tot RTBC, latest test run is green but didn't report back: https://qa.drupal.org/pifr/test/900068

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 170: drupal-1279226-170.patch, failed testing.

Status:Needs work» Needs review
David_Rothstein’s picture

Status:Needs review» Reviewed & tested by the community

Looks like a testbot fluke.

attiks’s picture

#189 So you're planning to commit this? ;-)

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 170: drupal-1279226-170.patch, failed testing.

Status:Needs work» Needs review

Fabianx queued 170: drupal-1279226-170.patch for re-testing.

Fabianx’s picture

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 170: drupal-1279226-170.patch, failed testing.

Fabianx’s picture

Status:Needs work» Reviewed & tested by the community
David_Rothstein’s picture

Status:Reviewed & tested by the community» Fixed
Issue tags:+7.36 release notes, +7.36 release announcement

OK, let's do it. Committed to 7.x - thanks!

Tagging this so I remember to add a change notice about this before putting out the Drupal 7.36 release announcement.

Followups discussed above to possibly file:
- Adding a "scripts_special" key to info files
- Adding a user interface for setting the new 'javascript_always_use_jquery' variable.

  • David_Rothstein committed f41ecaf on 7.x
    Issue #1279226 by attiks, ericduran, Wim Leers, sun, David_Rothstein,...
David_Rothstein’s picture

I wrote up a change notice: https://www.drupal.org/node/2462717

RajabNatshah’s picture

Testing :)

Status:Fixed» Closed (fixed)

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