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
- A new variable
javascript_always_use_jquery
. - A new option for drupal_add_js/hook_library
need_jquery
(default TRUE) that allows you to specify if jQuery is needed. - 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.
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:
// 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.
Comment | File | Size | Author |
---|---|---|---|
#170 | drupal-1279226-170.patch | 18.64 KB | David_Rothstein |
#156 | interdiff.txt | 1.66 KB | attiks |
#156 | i1279226-156.patch | 24.08 KB | attiks |
#140 | interdiff.txt | 1.73 KB | attiks |
#140 | i1279226-140.patch | 24.06 KB | attiks |
Comments
Comment #1
Crell CreditAttribution: Crell commentedSeems logical to me, although I'll let someone who knows the JS system better mark it RTBC.
Comment #3
Wim LeersFound a bug: the one-line change in the first patch forgets to send the basic JS settings (
basicPath
andpathPrefix
). Fixed that in this follow-up patch.Also, testbot is broken (it couldn't apply the patch).
Comment #5
Wim LeersOkay, so testbot can't handle patches without prefixes. WTF.
Then this patch should pass testing.
Comment #6
sunNeeds 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?
Comment #8
Wim Leers#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.
Comment #9
sun@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:
Comment #11
Wim LeersLOL! 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.
Comment #12
sunThat 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.
Comment #13
Wim Leers1) 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.
Comment #14
catchFixing tag.
Comment #15
Wim LeersReroll 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).
Comment #16
RobLoachBot?
Comment #18
Wim LeersAs 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.
Comment #19
jcisio CreditAttribution: jcisio commentedsun 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.
Comment #20
catchWhile 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.
Comment #21
attiks CreditAttribution: attiks commentedFirst try to see what testbot has to say
Comment #22
attiks CreditAttribution: attiks commentedToDo: clean whitespace
Comment #24
attiks CreditAttribution: attiks commentedFYI, I tried a manual install and got
Comment #25
attiks CreditAttribution: attiks commentedAnother one
Comment #27
attiks CreditAttribution: attiks commentedI ran into the first problem with testbot, some tests are defined like
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 :/
Comment #28
attiks CreditAttribution: attiks commented@Wim, if you want it back, let me know ;p
Comment #29
nod_And the rest of the theme_token errors comes from this bit in drupal_web_test_case.php:2768
that suppose settings are on all pages, since we removed that it's not the case anymore.
Comment #30
attiks CreditAttribution: attiks commentedNew patch, still failing on some tests
Comment #31
attiks CreditAttribution: attiks commentedand some tests contain some ugly code
Comment #33
ericduran CreditAttribution: ericduran commentedSo 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.
Comment #34
ericduran CreditAttribution: ericduran commentedSame exact thing plus a small tiny fix.
Comment #35
ericduran CreditAttribution: ericduran commentedOk so apparently AJax commands need to render js settings without having to add any files to the page.
Lets give this one a try.
Comment #37
ericduran CreditAttribution: ericduran commentedGetting closer but my settings are still wrong.
Comment #38
ericduran CreditAttribution: ericduran commentedSettings the pathPrefix correctly.
Comment #40
attiks CreditAttribution: attiks commentedNew patch for testbot
Comment #41
attiks CreditAttribution: attiks commentedI 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.
Comment #42
attiks CreditAttribution: attiks commentedSome more information about the approach
API change: you can not longer rely on drupal_add_js to get all the data
Possible API change: Added this to drupal_array_merge_deep_array, but this needs more testing
this is just a wrapper around _drupal_get_js()
common libraries and settings are only added when needed.
merge of the settings is now done by _drupal_add_js
merge of the settings is now done by _drupal_add_js
extra check added to drupal_group_js to make sure $javascript isn't empty.
merge of the settings is now done by _drupal_add_js
Comment #43
nod_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 :(
Comment #44
ericduran CreditAttribution: ericduran commentedHmm, 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.
Comment #45
attiks CreditAttribution: attiks commentedI cleaned some comments and white space issues
Comment #46
attiks CreditAttribution: attiks commentedBasic test added, let's see if the bot likes it
Comment #47
nod_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()
indrupal_add_js
? it's only needed indrupal_js_settings
and I'd rename this functiondrupal_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.
Comment #48
attiks CreditAttribution: attiks commentedSome 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.
Comment #49
attiks CreditAttribution: attiks commentedPatch for drupal_get_css()
Comment #51
attiks CreditAttribution: attiks commentedRerolled patch 49 against 8.x, this will fail on one test (AJAXFrameworkTestCase->testLazyLoadOverriddenCSS()) for overriden css.
Comment #53
attiks CreditAttribution: attiks commentedI can create new issues for
Comment #54
nod_Thanks for splitting things up, that'll make things easier for this one.
Comment #55
attiks CreditAttribution: attiks commentedNew patch that fixes #51
Comment #56
ericduran CreditAttribution: ericduran commentedOk 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.
Comment #58
ericduran CreditAttribution: ericduran commentedOk hopefully this will be correct, I had to rerolled.
Comment #59
ericduran CreditAttribution: ericduran commentedOk here's another tried. I miss script_path which was just added recently.
Comment #60
ericduran CreditAttribution: ericduran commentedHere's a documented patch.
Comment #61
ericduran CreditAttribution: ericduran commentedThe 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.
Comment #62
attiks CreditAttribution: attiks commentedtypo: $prefix isn't defined, should be $pathPrefix
Comment #63
ericduran CreditAttribution: ericduran commentedThanks also got rid of the useless ternary.
Comment #64
attiks CreditAttribution: attiks commentedAfter inspecting the patch it seems that you cannot just add a setting without adding a file. First patch should pass, second should fail.
Comment #65
nod_This is awesome :) I'll review the patch properly after the testbot gives the green light.
Comment #67
ericduran CreditAttribution: ericduran commented@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.
Comment #68
attiks CreditAttribution: attiks commented@eric maybe add it somewhere in the comments, so people know they have to add at least one file?
Comment #69
nod_So the test needs to be changed from
assertTrue
toassertFalse
(or something) intestPageWithOnlyJavascriptSetting
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 :)
Comment #70
attiks CreditAttribution: attiks commentedFrom #48:
I think with @eric's patch this means that the problem still exists, drupal_alter_js isn't going to be called.
Comment #71
nod_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 :)
Comment #72
attiks CreditAttribution: attiks commented@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?
Comment #73
attiks CreditAttribution: attiks commented#63: 1279226-63.patch queued for re-testing.
Comment #74
sunI 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.
Comment #76
sunI like.
This still looks backportable to me.
Comment #78
sunSpin-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.
Comment #79
nod_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. aDrupal = 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.
Comment #80
ericduran CreditAttribution: ericduran commentedHmm, moving that to template_process_html doesn't feel backportable to me. I'll try and test this.
Comment #81
Wim LeersReview + patch reroll for #76 with test fixes coming up in a few hours. (Posting this to prevent double work.)
Comment #82
Wim LeersHere'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.
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()…
When desired, this can easily be overridden using hook_js_alter(): great.
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().Comment #84
attiks CreditAttribution: attiks commentedmy 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.
Adding html5shiv is something is missed, but is needed.
If I find some time I'll try to work on this.
Comment #85
attiks CreditAttribution: attiks commentedAnother 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.
Comment #87
attiks CreditAttribution: attiks commentedAdding the html5shiv inside drupal_get_js was not a good idea, AJAX is still calling drupal_get_js and outputting it.
Comment #88
attiks CreditAttribution: attiks commentedUgly fix, just to make sure this was the only problem
Comment #90
attiks CreditAttribution: attiks commentedComment #91
attiks CreditAttribution: attiks commentedProbably 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.
Comment #92
catchhtmlshiv 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().
Comment #93
attiks CreditAttribution: attiks commented@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
ugly code
Comment #94
attiks CreditAttribution: attiks commentedComment #95
nod_Worst performance bug we have on frontend right now.
Comment #96
nod_hey sun, still on your radar? That'd be nice to get this in D8.
Comment #97
RobLoachI'm not sure how I feel about that "jsonly" flag.
Comment #98
RobLoachEither way, the patch would need to be updated.
Comment #99
sunI 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.
Comment #100
sunSorry, just a plain re-roll. Tried to go and debug further than that, but got heavily distracted.
Comment #102
nod_D8 needs to properly solve that, it's happening in #1737148: Explicitly declare all JS dependencies, don't use drupal_add_js.
Comment #103
sunComment #104
attiks CreditAttribution: attiks commentedSo now we need to decide if this can and will be backported to D7, @catch?
Comment #105
catchThat'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.
Comment #106
Wim LeersI 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.
Comment #107
attiks CreditAttribution: attiks commented#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.
Comment #108
Wim Leers#107: that's *genius*. It's not perfect, but it gets the job done. Does anybody strongly oppose?
Comment #109
nod_Here is a big +1 from me.
Comment #110
seutje CreditAttribution: seutje commented@#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.
Comment #111
David_Rothstein CreditAttribution: David_Rothstein commentedSorry 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 :)
Comment #112
attiks CreditAttribution: attiks commented#111 Can you have a look at the patch?
Comment #113
attiks CreditAttribution: attiks commentedNew 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
Comment #114
attiks CreditAttribution: attiks commentedReminder to self: add tests for javascript_always_use_jquery and scripts_special
Comment #115
RobLoachReminder 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.
Comment #117
attiks CreditAttribution: attiks commented#115 If only ... we need it otherwise we might break themes that have their javascript (depending on jQuery) inside tpl.php files.
Comment #118
attiks CreditAttribution: attiks commentedgo bot
Comment #120
attiks CreditAttribution: attiks commented#118: i1279226-118.patch queued for re-testing.
Comment #121
RobLoachThis 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?
Comment #122
attiks CreditAttribution: attiks commented#121 That's true for Drupal 8, but this is for Drupal 7 ;-)
Comment #124
RobLoachAh, thanks for the explanation ;-) . The jQuery dependency in Drupal 7 does seem like almost a must at this point.
Comment #125
attiks CreditAttribution: attiks commented#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.
Comment #127
attiks CreditAttribution: attiks commentedOnly 5 left, I'm done for today so feel free to help if you can ;-)
Comment #128
attiks CreditAttribution: attiks commentedJavascript tests should be fixed.
Comment #130
attiks CreditAttribution: attiks commentedReroll 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?
Comment #132
attiks CreditAttribution: attiks commentedTest 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
Comment #133
attiks CreditAttribution: attiks commentedI tested this manually both as logged in user and as anon user and it works, so the test in't working :/
Ajax response
Submitting the form a second time behaves strange, the system.js is returned again:
Comment #134
attiks CreditAttribution: attiks commentedWork around added for ajax, drupal_get_js has an extra optional parameter to force the creation of ajaxPageState.
Comment #136
attiks CreditAttribution: attiks commented#134: i1279226-134.patch queued for re-testing.
Comment #137
attiks CreditAttribution: attiks commentedlet's blame the bot, failing tests passed locally
Comment #138
attiks CreditAttribution: attiks commented#134 is green, any other concerns?
Comment #139
attiks CreditAttribution: attiks commentedI 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...
Comment #140
attiks CreditAttribution: attiks commentedNew patch
Comment #141
attiks CreditAttribution: attiks commentedFYI: I created an issue for Advanced CSS/JS Aggregation: #1989710: Add drupal_add_js_page_defaults
Comment #142
mikeytown2 CreditAttribution: mikeytown2 commentedThanks 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
Comment #143
attiks CreditAttribution: attiks commented#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.
Comment #144
mjpa CreditAttribution: mjpa commentedTentatively 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?
Comment #145
attiks CreditAttribution: attiks commentedDid you test this with the variable set? If you don't set the variable it works as without the patch.
Comment #146
attiks CreditAttribution: attiks commentedComment #147
mjpa CreditAttribution: mjpa commentedI 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?
Comment #148
attiks CreditAttribution: attiks commentedIt 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.
Comment #149
no_angel CreditAttribution: no_angel commentedComment #150
mjpa CreditAttribution: mjpa commentedattiks: 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...
Comment #151
attiks CreditAttribution: attiks commented#150 I think you're right, it probably will be a waste of time :/
Comment #151.0
attiks CreditAttribution: attiks commentedMinor clarification.
Comment #151.1
attiks CreditAttribution: attiks commentedUpdated to reflect the patch
Comment #151.2
attiks CreditAttribution: attiks commentedtypos and clarification
Comment #152
attiks CreditAttribution: attiks commentedIssue summary updated, feel free to improve
Comment #153
attiks CreditAttribution: attiks commentedUpdate so maybe somebody can review this
Comment #154
attiks CreditAttribution: attiks commentedComment #155
mikeytown2 CreditAttribution: mikeytown2 commentedStrict 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.Comment #156
attiks CreditAttribution: attiks commented#155 New patch, interdiff contains some more lines because of offset
Comment #158
attiks CreditAttribution: attiks commented#156: i1279226-156.patch queued for re-testing.
Comment #160
attiks CreditAttribution: attiks commentedadded extra check
Comment #161
Wim LeersDo we still think this is realistic to get in Drupal 7? David Rothstein, are you open to it?
Patch no longer applies FWIW.
Comment #161.0
Wim Leers$need_ajaxPageState added as API change
Comment #162
attiks CreditAttribution: attiks commentedQuick - locally untested - reroll for testbot
Comment #163
achtonFixed a few typos in summary.
Comment #164
Fabianx CreditAttribution: Fabianx commentedIt 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.
Comment #165
attiks CreditAttribution: attiks commentedDavid, can you provide feedback, is this acceptable for D7?
Comment #166
attiks CreditAttribution: attiks commentedDavid, can you provide feedback, is this acceptable for D7?
Comment #169
mikeytown2 CreditAttribution: mikeytown2 commented#162 passes tests. Waiting on David_Rothstein
Comment #170
David_Rothstein CreditAttribution: David_Rothstein commentedI 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:
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.
Comment #172
attiks CreditAttribution: attiks commented#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
Comment #173
David_Rothstein CreditAttribution: David_Rothstein commentedThanks 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.
Comment #174
attiks CreditAttribution: attiks commentedTested 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).
Comment #175
attiks CreditAttribution: attiks commentedTo 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.
Comment #176
David_Rothstein CreditAttribution: David_Rothstein commentedThanks 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).
Comment #177
attiks CreditAttribution: attiks commentedBack to RTBC, it was my base theme (omega 4) that did some javascript handling, tested with other themes and no more errors.
Comment #180
Fabianx CreditAttribution: Fabianx commentedComment #183
attiks CreditAttribution: attiks commentedComment #186
attiks CreditAttribution: attiks commentedBack tot RTBC, latest test run is green but didn't report back: https://qa.drupal.org/pifr/test/900068
Comment #189
David_Rothstein CreditAttribution: David_Rothstein commentedLooks like a testbot fluke.
Comment #190
attiks CreditAttribution: attiks commented#189 So you're planning to commit this? ;-)
Comment #193
Fabianx CreditAttribution: Fabianx commentedComment #195
Fabianx CreditAttribution: Fabianx commentedComment #196
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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.
Comment #198
David_Rothstein CreditAttribution: David_Rothstein commentedI wrote up a change notice: https://www.drupal.org/node/2462717
Comment #199
Rajab Natshah CreditAttribution: Rajab Natshah commentedTesting :)
Comment #201
Wim LeersHelp update the Google Analytics module so that it sets
'requires_jquery' => FALSE
: #2509446: Specify 'requires_jquery' => FALSE, to not load jQuery if Google Analytics is used in its basic mode.Comment #202
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.
Comment #203
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedRemoving tag - change record was added a long time ago.
Comment #204
klonosThe 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?