jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page

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

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?

@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".

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.

Fixing tag.

Bot?

sun marked #1547144: Drupal 7/8 unconditionally loads jquery.js jquery.once.js and drupal.js for pages that don't need it as duplicate, then we need this for D8.

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.

First try to see what testbot has to say

ToDo: clean whitespace

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

Another one

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 :/

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

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

    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.

New patch, still failing on some tests

and some tests contain some ugly code

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.

Same exact thing plus a small tiny fix.

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.

Getting closer but my settings are still wrong.

Settings the pathPrefix correctly.

New patch for testbot

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.

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

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 :(

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.

I cleaned some comments and white space issues

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

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.

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 '';
+    }

Patch for drupal_get_css()

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

I can create new issues for

• drupal_group_js: #1554122: drupal_group_js doesn't check if $javascript is empty
• drupal_array_merge_deep_array: #1554142: drupal_array_merge_deep_array assumes $array is actually an array

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

New patch that fixes #51

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.

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

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

Here's a documented patch.

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.

+++ 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

Thanks also got rid of the useless ternary.

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

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

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

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

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

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.

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

@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?

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

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.

I like.

This still looks backportable to me.

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.

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.

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

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.

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.

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

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

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.

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

@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

Worst performance bug we have on frontend right now.

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

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

Either way, the patch would need to be updated.

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.

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

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

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

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.

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

Here is a big +1 from me.

@#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.

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

#111 Can you have a look at the patch?

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

Reminder to self: add tests for javascript_always_use_jquery and scripts_special

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.

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

go bot

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

+++ 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?

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

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

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

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

Javascript tests should be fixed.

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?

Test was added by #561858: [Tests added] Fix drupal_add_js() and drupal_add_css() to work for AJAX requests too by adding lazy-load to AJAX framework

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

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

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

let's blame the bot, failing tests passed locally

#134 is green, any other concerns?

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

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.

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

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

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

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?

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

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?

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.

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

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

Minor clarification.

Updated to reflect the patch

typos and clarification

Issue summary updated, feel free to improve

Update so maybe somebody can review this

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.

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

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

added extra check

Quick - locally untested - reroll for testbot

Fixed a few typos in summary.

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.

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

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

#162 passes tests. Waiting on David_Rothstein

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.

#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

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.

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

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.

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

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

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

Looks like a testbot fluke.

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

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.

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

Testing :)

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.

I created #2594151: Allow the 'javascript_always_use_jquery' variable to be configured via the Performance page for the second one now. The first is still up for grabs, if someone wants to create an issue suggesting that.

Removing tag - change record was added a long time ago.

The issue summary states in its "API changes" section the following 2 things that have not actually been implemented AFAICT:

- Themes can use scripts_special[] inside their info file to indicate that the included script does not depend on javascript.
- 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.

At some point, there was a comment about filing separate, follow-up issues for these, but could not find any. Can anyone please clarify?