Problem
- CSS can be conditionally loaded for limited
'browsers'
(e.g., IE) only, but JS cannot. - #1077878: Add HTML5shiv to core cannot be done without conditional comment support for JS.
- Modules like No IE6 cannot load JavaScript files for specific browsers.
Proposed solution
- Make it consistent with drupal_add_css
- Hacking a #browsers condition into the current code of drupal_get_js() doesn't really make sense.
- Instead, further align drupal_get_js() with drupal_get_css(), as originally proposed in #330082-20: Refactor drupal_get_js() to use render layer similar to drupal_get_css() already.
- Introduce #type 'scripts' -- equivalent to existing #type 'styles'.
- As already the case for #type 'styles' / drupal_get_css(), properly separate the current processing of JS into a #group_callback and #aggregate_callback.
Side-benefit: Default behavior can be changed and improved in contrib.
- Only do the necessary to align #type 'scripts' with 'styles', and to introduce support for 'browsers'.
A major long-term benefit of doing so is that we're finally able to abstract and merge further parts of the #group_callback and #aggregate_callback logic into common helper functions -- paving the way to get to the long-standing idea of having a single generalized system for "support files"/"page requisites" in Drupal core.
Current State
The last patch submitted #136 has been RTBC.
This patch does everything proposed. It also passed all the JS test, and a couple of new test were added.
Seems good to go.
Comments
Comment #1
arianek CreditAttribution: arianek commentedsubscribe
Comment #2
rfaysubscribe
Comment #3
katbailey CreditAttribution: katbailey commentedI had a quick look at this and it seems that the root of the problem (or discrepancy between how it works for css and for js) is that the css is treated as a renderable array and so has pre_render functions added, whereas the js is not and is just output directly.
In drupal_get_css, we have:
Then in system_element_info(), which is where pre_render functions for the various renderable element types are defined:
And the drupal_pre_render_styles function in turn does some crazy processing to deal with IE's 31 css files limitation, including changing the element's type to 'html_tag', which then will get another pre_render function as per system_element_info():
So at least this explains how drupal_pre_render_conditional_comments gets called (I think! :-P).
And there's no room for adding a pre-render function for js because it is not passed through drupal_render, so we'd need to restructure drupal_get_js entirely :-/
At least that's how it seems to me.
Comment #4
altrugon CreditAttribution: altrugon commentedUpsss... thank you so much katbailey, I never thought the problem was going to big that big. Now I see why I couldn't find the relation between drupal_pre_render_conditional_comments() and drupal_add_css(), this goes quiet far out of my knowledge.
I'm happy to help but as I said this is quiet a bit for me and I don't think I can solve it alone.
Comment #5
altrugon CreditAttribution: altrugon commentedI'm going to bump this to critical because after katbailey's comment this looks like a release issue.
Comment #6
dmitrig01 CreditAttribution: dmitrig01 commentedComment #7
RobLoachI'm not sure this is the right way around it. jQuery provides jQuery.support and jQuery.browser, which might be a better way aroudn this problem. It makes complete sense to have this for CSS, but for JavaScript, using $.browser might be a better option. Thoughts?
Comment #8
mfer CreditAttribution: mfer commentedThis is neither a bug nor is it critical. It would be a nice feature to have. But, it is a feature. And, js support has worked for years without it.
The $scripts variable is built in template_process_html. That means moduleName_process_html and/or themeName_process_html both have the opportunity to alter the scripts before rendering. This is very similar to how you could alter it with preprocess previously.
Comment #9
dmitrig01 CreditAttribution: dmitrig01 commentedhm, i'd have to side with mfer. Though, I disagree with robloach
Comment #10
altrugon CreditAttribution: altrugon commentedThank you guys,
The solution proposed by mfer is what I was looking for because only IE browsers will read the scripts, but the solutions that Rob Loach offer is not bad either however all browsers will read the script and only IE will be able to render the code.
Comment #11
rfayLooks to me like there was an x-post and altrugon didn't mean to set the category or priority back.
Comment #12
altrugon CreditAttribution: altrugon commentedupsss... sorry, the page refresh didn't update the settings fieldset.
Thank you rfay.
Comment #13
masagin CreditAttribution: masagin commentedI was quite surprised that the 'browser' option is missing in 'drupal_add_js'. I was pretty sure it's going to work same way as 'drupal_add_css' does. Browser sniffing with $.browser is not an option. Not only its usage is discouraged by jQuery itself but forcing other browsers to load IE fixing scripts (or whole libraries) is a bit harsh.
It would be nice to have the 'browser' option in both add_css and add_js...at least because themers will probably expect it that way.
I will try mfer's solution for now but I'm sad. :(
Comment #14
mfer CreditAttribution: mfer commented@bganicky I understand your desire for a 'browser' option. Truthfully, the JS and CSS systems are going to get a bit of rework in D8 and this could be good to be part of that.
But, D7 is feature frozen. If these were found some time ago I'm sure it would have gone in. Now it's just too late. We need to wait for the D8 thaw to add it.
Comment #15
Jeff Burnz CreditAttribution: Jeff Burnz commentedSubscribe.
Comment #16
Mark TrappSubscribe.
Comment #17
JohnAlbinThis is needed for #1077878: Add HTML5shiv to core
Comment #18
Jacinesub
Comment #19
sunOn jQuery.support/.require, I guess that HTML5shiv should be loaded before the DOM initializes? (or rather, before jQuery hits the DOM and might potentially evaluate/calculate widths/heights of elements)
Thanks @katbailey for the analysis. Looks like we need to add a corresponding #type
scripts
that implements the identical logic. -- That said, I can only hope that we've implemented sufficient tests for the crazy and extremely complex AJAX lazy-loading code concerning scripts, so these changes won't break existing functionality (which is a pain to debug).Comment #20
sreynen CreditAttribution: sreynen commentedAttached is a first stab at restructuring drupal_get_js() to run output through drupal_render() rather than theme(). With aggregation off it works as expected for drupal_add_js() calls like this, the same format drupal_add_css() uses:
It doesn't do any conditional comments with aggregation on yet, so that still needs work.
Comment #21
alexanderpas CreditAttribution: alexanderpas commentedMajor, since #1077878: Add HTML5shiv to core depends on this, which is important for HTML5 in D8
Comment #22
jessebeach CreditAttribution: jessebeach commentedsubscribe
Comment #23
ericduran CreditAttribution: ericduran commentedsub.
Comment #24
ericduran CreditAttribution: ericduran commentedswitching to needs-review so the testbot can run with the patch.
Comment #25
ericduran CreditAttribution: ericduran commentednever mind, this patch is not really complete.
(sorry :( )
Comment #26
ericduran CreditAttribution: ericduran commentedThis is probably the most hack-y patch I've ever written. Anyways here it goes.
This patch separates out any browser specific files from the regular flows and renders those at the end. It also doesn't change the theme('html_tag") everywhere for a drupal_render it just calls render when it's ready to render the specific browser js files.
This works as your would expect it. You pass in
You get
I don't in anyway think this is complete but at the same time I don't think we need to change all the theme('html_tag') calls to render just because we need the drupal_pre_render_conditional_comments function to run for those specific scripts.
Comment #27
sreynen CreditAttribution: sreynen commentedWhat's the down side to changing everything to render? There's already a check for browsers in the pre render function, so checking that again to selectively use render introduces redundancy we should avoid. Are we trying to avoid render for some reason?
This also doesn't seem to do any aggregation on browser-specific JS. Is that intentional?
Comment #28
ericduran CreditAttribution: ericduran commented@sreynen, The pre_render doesn't need to run in any of the js except for the browser specific ones, so is just extra work for no reason. Same applies for switching everything to render, I'm going to safely guess that calling render is going to be a bit slower then the theme function which is pointless because we don't need any of the render functionality anywhere except the browser specific js files.
I didn't aggregate browser-specific js on purpose. I'm not sure on the best practice but browser specific js are usually not aggregated, maybe minified. But yes that was done on purpose.
Comment #29
sreynen CreditAttribution: sreynen commentedIf there's a significant performance problem with drupal_render(), that makes sense to avoid it where we can. But then why do we use it universally for CSS? Should drupal_get_css() also be updated (in a separate issue) to selectively use drupal_render() like this or is there something different there?
Comment #30
ericduran CreditAttribution: ericduran commentedIt was just the way I decided to write the patch :-/ I guess we'll see what other people think.
Comment #31
JacineTagging.
Comment #32
sunHacking a #browsers condition into the current code doesn't really make sense. We need to align drupal_get_js() with drupal_get_css(). That's quite a major challenge and change.
array('#type' => 'scripts', '#items' => $elements)
passed to drupal_render().In the end, the flow and processing of drupal_get_css() and drupal_get_js() will look fairly similar, and we'll (finally!) likely be able to abstract and merge most of the processing into common helper functions for both. However, that does not have to be part of this issue.
Comment #33
ericduran CreditAttribution: ericduran commented@sun, makes sense. /me stops hacking crap.
Comment #34
iflista CreditAttribution: iflista commented#26: 865536-drupal_add_js-add-browser-options.patch queued for re-testing.
Comment #35
jessebeach CreditAttribution: jessebeach commentedJS filed added with a "browser" option loads before core JS files:
https://skitch.com/jesse.beach/f89qr/google-chrome
Without the "browser" option, the theme JS file loads after the core files:
https://skitch.com/jesse.beach/f89qk/developer-tools-http-drupal.dev
The $browser_output was being prepended to the scripts output. I updated the patch and appended the browser output instead. Otherwise, the patch is unchanged.
Comment #36
aspilicious CreditAttribution: aspilicious commentedThis needs work, see #32.
Comment #37
ericduran CreditAttribution: ericduran commented#32 is not an easy task.
Patch to come in a day or two. Right now is not working lol.
Comment #38
ericduran CreditAttribution: ericduran commentedugg, this issue is getting me mad lol. Still not quiet there.
Comment #39
jessebeach CreditAttribution: jessebeach commentedFeel free to post a partial patch if you want to collaborate. I can't promise that I can add much, but maybe I can help a little.
Comment #40
ericduran CreditAttribution: ericduran commentedOk, here's an un-complete patch.
This is as stable as I can get it, I'm still messing with the groups and thats wayyyy broken, so I didn't want to put that one up.
On this patch the order is completely out of whack, and the settings are loaded 1st.
This is still missing the group and aggregate function which is where most of the real logic will be moved too. /me is struggling but I'll keep trying.
Comment #41
ericduran CreditAttribution: ericduran commentedThis is a better patch with what I believe to be a proper working group function and also maybe proper aggregation of the groups
Comment #42
ericduran CreditAttribution: ericduran commentedhere's another patch for anyone interested in helping.
This is by no ways finish, also make sure you have devel install, I'm sure I have some dsm's in there.
This patch sets up the proper groups, the aggration function is currently not working correct.
Also I print out what the new output is going to be. Is getting there, but still not there yet.
Comment #43
ericduran CreditAttribution: ericduran commentedAnother patch, getting there, there's still something wrong with the grouping, I'm loosing the key somewhere which is causing each file to be render either individually or empty.
#Needs-more-eyes ;)
Comment #44
sunWow, amazing progress! Thanks, @ericduran!!
That's exactly the direction we need to go. Probably still some heavy lifting required, but I'm confident we'll get there. I'll try to loop in @effulgentsia, who is the person most familiar with this code to my knowledge.
Comment #45
ericduran CreditAttribution: ericduran commentedThanks @sun.
Here's an actual working patch. This one is actually ready for testing. Still needs tons of clean-up and there's some to-dos in there. But all the old functionality should be working now.
I tested some ajax, but not a lot. Everything seems to work as expected.
I guess I just needed sleep.
Comment #46
ericduran CreditAttribution: ericduran commentedComment #47
ericduran CreditAttribution: ericduran commentedThis is for functionality review only. I need to get some actual work done now, so we'll see what test are failing so we can get those fixed and then we can move on from there.
TODO:
- better variable names (I got lazy, sorry)
- Test for browsers options, - I haven't even touch this part yet
- There's some functionality in the aggrate_js that should be in the group_js.
- Comments!!
Comment #48
ericduran CreditAttribution: ericduran commentedLol, this actually works, is cleanup and testing time.
Comment #49
ericduran CreditAttribution: ericduran commentedThis is another patch, that is ready for testing.
I'll love to see @effulgentsia thoughts on this patch ;)
This should handle the browser option properly now too.
What this patch does:
- Add #type 'scripts' to system_element_info(), using a #group_callback like #type 'styles'.
- Make drupal_get_js() invoke that #group_callback
- Replace custom theme() calls with array('#type' => 'scripts', '#items' => $elements) passed to drupal_render().
- added drupal_aggregate_js, drupal_group_js to group the js and the aggregation
- Add the browser option into drupal_js_defaults, and also added it into the standard javascript array for misc/drupal.js, and settings.
Comment #50
sunThanks!
These two seem to be obsolete in the #pre_render function.
This could be reverted to save some bytes ;)
#type should come first.
I've the impression that these two hunks got lost -- perhaps I'm missing/overlooking something though.
Trailing white-space and partially exceeding 80 chars (elsewhere, too).
16 days to next Drupal core point release.
Comment #51
ericduran CreditAttribution: ericduran commentedChanges:
- Removed - $processed since its pointless now
- Reverted the $js_version_string to its previous position to avoid a change.
- Move #type to be the 1st key in the array.
- Removed trailing whitespace, and anything over 80 chars
- The file groups got moved to to drupal_group_js
- The drupal_build_js_cache was moved to drupal_aggregate_js but there is no check like there was before for case when the file isn't created.
Still needs work, but this one is a bit closer.
Comment #52
altrugon CreditAttribution: altrugon commentedThank you so much for all the work you are putting on this ericduran.
Comment #53
effulgentsia CreditAttribution: effulgentsia commentedI haven't read the full discussion here, but I totally support the direction of this patch. Consistency++. A year ago, after the corresponding refactoring of drupal_get_css() went in, I asked in #330082-20: Refactor drupal_get_js() to use render layer similar to drupal_get_css() if there was sufficient incentive to refactor drupal_get_js() along the same lines, but it wasn't clear there was a strong enough use-case to justify the investment of time. Great to see the 'browsers' use-case creating that justification, and thank you, @ericduran, for running with it.
#51 looks really close to me. Here's some feedback, but at this point, this is just about fine tuning. Additionally, I'm a little concerned we don't have adequate automated tests for all the combinations of options and how they affect aggregation and tag order. I don't know if we want to make creating those tests a requirement of this patch.
Although this is just a copy/paste from elsewhere, might as well fix the typo: s/off/of/
The comment doesn't make sense for this code. Perhaps something like "Render the HTML needed to load the JavaScript."
Neither $index nor $files are used in this function, so can be removed. It's not good practice for a #pre_render callback to return an entirely new array (i.e., $new_element). Instead, add each new element to $elements, and return $elements.
This is making an assumption that there's only a single setting item within $group, and it's at index 0. While this is a reasonable assumption, given that drupal_add_js() creates just one setting item period, and the 'setting' type isn't grouped within drupal_group_js(), I think it would be clearer to loop this the same way that you do for 'inline' and 'external'. In fact, I wonder if the code could be shortened by doing the common things for setting, inline, and external just once.
Copy/paste artifact? We're not outputting LINK tags here.
I like the addition of the 'browsers' option, but why only for files? Seems like 'inline' and 'external' should benefit from it too. Given the way drupal_add_js() squashes 'setting' items, that one may need further discussion to support a 'browsers' option.
Legacy issue, but we're ignoring 'defer' on aggregated files. Not sure if we want to fix that as part of this issue (for example, by making drupal_add_js() set 'preprocess' to FALSE when 'defer' is TRUE, similarly to what it already does for 'cache'), or defer (sorry, couldn't resist) it to another issue.
This comment seems out of place here.
s/script/scripts/
Remove the sentence about "two purposes", since here there's only one purpose.
Seems this can be shortened to just 'file' and not 'file'.
In other parts of core, we refer to this variable as $javascript (singular), so for consistency, let's do the same here. Yes, it's an array with usually more than one item, so if we want to globally change it to plural throughout core, let's open a new issue to discuss that.
Needs a code comment explaining why.
Minor nit, but would a construct more like
if ($item['type'] == 'file' && $item['preprocess']) {...} else {...}
be easier to read?It does not do the last sentence.
Since there's only one 'case', does it make sense to remove the switch construct and add the 'type' condition to the if expression?
Alternatively, how about changing drupal_build_js_cache() to use $item['data'] instead of the array index? That would make it consistent with drupal_build_css_cache().
Powered by Dreditor.
Comment #54
omercioglu CreditAttribution: omercioglu commentedSub
Comment #55
ericduran CreditAttribution: ericduran commented@effulgentsia, wow, thanks for that. I'll go through an implement those recommendation, I'll also clean up and add proper comments.
Comment #56
sunFYI: I've updated the issue summary.
I agree that we should support the 'browsers' option for 'inline' and 'external', too. However, I'd leave out support for 'setting', as that deserves a separate discussion and possibly requires larger changes.
Comment #57
catchBringing these two into line would be great.
I'm crossposting #1014086-47: Stampedes and cold cache performance issues with css/js aggregation which already has some code to move some of the css/js rendering stuff to helper functions. As others have said that can be done separately.
Comment #58
JacineTagging.
Comment #59
ericduran CreditAttribution: ericduran commentedHere's a patch witch takes into account all of @effulgentsia comments,
I'll post up a review in a sec.
Comment #60
ericduran CreditAttribution: ericduran commentedOk so Here's what the latest patch does #59
I'll try to do another check tomorrow to make sure I didn't miss anything.
Comment #61
JacineStill needs to be reviewed and tested. Updating to current sprint.
Comment #62
effulgentsia CreditAttribution: effulgentsia commentedGetting very close.
By removing this, and replacing with the logic in this patch, Drupal.settings is no longer rendered after all other JavaScript. I believe HEAD is wrong for forcing settings to be last in this way, and that the patch is correct. If it's important for Drupal.settings to be last, then within drupal_add_js(), the settings object should be given a JS_SETTINGS group that is defined as a larger integer than JS_THEME.
This shouldn't be evaluated at the group level, since each item can have its own version, and when site-wide aggregation is disabled, it's the item version that matters.
The whole fewer tags business was to deal with an IE limitation specifically surrounding LINK and STYLE tags. For JS, the only reason for grouping is to aggregate. So, how about "A function to call to group #items. Following this function, #aggregate_callback is called to aggregate items within the same group into a single file."
Remove the "output a LINK tag ..." bit. "The group has been aggregated into a single file." is sufficient.
Might be nice to add a comment above this: "The group has not been aggregated into a single file. For example, aggregation might be disabled for a site during development."
Remove trailing whitespace.
Either remove the line break between these lines, or insert another one.
s/$javascripts/$javascript/
Comment #63
aspilicious CreditAttribution: aspilicious commentedTiny issue, trailing whitespace
6 days to next Drupal core point release.
Comment #64
stevetweeddale CreditAttribution: stevetweeddale commentedTagging for the current HTML5 sprint
Comment #65
dynamicdan CreditAttribution: dynamicdan commentedIs this going to miss the boat for D7?
Seems like it's already done but not 100% approved :(
FYI, this is like 2 years over due. I'm been supporting IE7 using conditional js for ages.... how did this miss the original function design? Some dev thought it was bad coding design but ok in drupal_add_css ??
Also, my use case is for excanvas/FLOT support if interested.
Comment #66
klonos...coming from #1077878: Add HTML5shiv to core
Comment #67
cosmicdreams CreditAttribution: cosmicdreams commentedLooks like what this patch needs is the implementation changes that #62 and #63 call for. I'll get to work on that.
Comment #68
KrisBulman CreditAttribution: KrisBulman commentedsubscribing
Comment #69
cosmicdreams CreditAttribution: cosmicdreams commentedEnded up not having time to work on this yesterday. Look like I will have a few hours near the end of the work day to touch on this.
Comment #70
cosmicdreams CreditAttribution: cosmicdreams commentedworking on this issue finding additional grammatical issues. Should I care?
Comment #71
cosmicdreams CreditAttribution: cosmicdreams commentedHere is a patch with all of the changes requested to comments and grammar the top two items that effulgentsia mentions have not been done. I'll actually have to read an understand what this code is doing in order to do that.
Comment #72
cosmicdreams CreditAttribution: cosmicdreams commentedComment #73
ericduran CreditAttribution: ericduran commentedLets wait till the actual code changes are made to mark it for review.
That way effulgentsia won't review the code till it's ready.
Comment #74
cosmicdreams CreditAttribution: cosmicdreams commentedSince I only marked this issue as need review in order to see if I had created the patch properly and have the testbot validated it, that should be good.
If anyone else has working knowledge about this patch and know how to implement the suggestions listed above feel free to jump in.
Comment #75
JacineTagging. Maybe this sprint is the lucky one?
Quick update: Per @ericduran This issue needs 2 small changes and will take him a couple of hours.
Comment #76
ericduran CreditAttribution: ericduran commentedThis patch fixes :
- Drupal.settings is now render last. I added a new JS_SETTINGS constant with a which gives the settings group a weight of 200.
- The $query_string added at the end of the file is now on a per file or per group basis. Which makes it work correctly even when aggregation is turned off.
- And all the comments/white spaces/line breaks are fixed.
This is now ready for some serious review.
There's one small thing I'll like to change about it, but honestly that doesn't have to go in with this issue. The one thing is forcing the browser group to render last before the settings. This should actually be happening now. But I need to review it a little more to see why thats not happening.
For anyone that wants the test, here are some things you might want to test for:
- Make sure all the expected JS is on the page ;-)
- Add some JS in a module with the browser option
- Add some JS in a theme with the browser option
Comment #77
RobLoachI like where this is going, but I still would prefer using #1033392: Script loader support in core (LABjs etc.), with checks to jQuery.browser.
Comment #78
cosmicdreams CreditAttribution: cosmicdreams commented@Rob Loach: why? I don't mean to be cheeky here. I'm interesting in your opinion.
Comment #79
RobLoach@cosmicdreams: Oh, just because it seems cleaner than adding a bunch more HTML and code ;-) . I'm happy either way though.
Comment #80
ericduran CreditAttribution: ericduran commented@Rob Loach, This really doesn't block the other issue.
This is honestly an improvement to drupal_get_js which just happens to allow the browser option but it also makes it easier to to add other improvements on top of it.
After we get this one in, we can open another issue to combine drupal_render_style, drupal_render_scripts, and drupal_group_js, drupal_group_css.
Either way, getting this one in doesn't mean we need to use the browser option for the html5 shiv. It just means drupal_get_js is better than it was before ;)
Comment #81
JacineYeah, I don't really understand why the script loading issue keeps getting referenced to this. One has nothing to do with the other. Also, every single article and book I have every read, recommends using conditional conditional comments for the shiv. Why on earth would be use a script loading solution for that? anyway, this issue is absolutely not meant to replace any true solution for dealing with polyfills. Like Eric said, it's to make it better and more consistent with drupal_add_js more consistent with drupal_add_css.
Comment #82
cosmicdreams CreditAttribution: cosmicdreams commentedI'm in the process of testing this now. I just installed the patch and setup my drupal 8 site and I see that all of the js from the system and Bartik theme are present.
I'm about to test adding a new javascript into the mix by creating a module for that job. What is the proper syntax I should use?
Comment #83
ericduran CreditAttribution: ericduran commentedSame way its used on drupal_add_css -- See http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_ad...
Comment #84
cosmicdreams CreditAttribution: cosmicdreams commentedcool, I created a module named test_patch, and put the proper test_patch.module and test_patch.info files in place. Here's the contents of my test_patch.module:
now i'm going to install that module and inspect the result.
Comment #85
cosmicdreams CreditAttribution: cosmicdreams commentedThere must be something wrong with my module because the text is simply being spat out at the top. I'll test adding it to a theme.
Added the new line of code to the theme...
Oh, I think I figured out what I did wrong with adding that line to the module. I'll test both.
Comment #86
ericduran CreditAttribution: ericduran commentedYou don't need the group, also you didn't specified it as en external js file so that won't work like that @see http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_ad...
Comment #87
cosmicdreams CreditAttribution: cosmicdreams commentedok, with your corrections I turn my attention to testing this within a theme. Here's the code I added to bartik's bartik_preprocess_html
testing...
result:
* javascript shows up in Drupal.setting's CDATA:
"\/\/html5shiv.googlecode.com\/svn\/trunk\/html5.js":1
Is that the intended result?
Comment #88
ericduran CreditAttribution: ericduran commentedI would test whats currently supported as thats all thats expected to work, so I wouldn't do //... instead pick a protocol for an external file or your a local file.
Comment #89
cosmicdreams CreditAttribution: cosmicdreams commentedwill do, but just so I know. What's the result I should be expecting? Can you post code I can compare to?
for example: should I expect that the browser specific code is wrapped in a conditional comment that only impacts a version of IE like I would expect to see for conditional css?
Comment #90
cosmicdreams CreditAttribution: cosmicdreams commentedcorrected code to :
do we all agree that the code is syntactically correct?
testing...
OK, I've got a good result this time. You see, apparently, I hadn't applied the patch yet.... sorry.
So I've got a good perspective of before patch and after patch behavior.
Before patch the line of code above would simply place the new script in the list of other included scripts without additional comments.
After the patch I get this:
Now I'm off to test this in a module...
Comment #91
cosmicdreams CreditAttribution: cosmicdreams commentedOK, previously mentioned module, I change my hook_page_alter to this:
Syntactically it looks right although excessive
testing...
works great! Here's the output:
Yea, in both cases there seems to be an extra, blank conditional comment included. Is that intentional?
Comment #92
cosmicdreams CreditAttribution: cosmicdreams commentedSo in summary, works for me. I recommend RBTC status.
Comment #93
ericduran CreditAttribution: ericduran commented@cosmicdreams try without the external flag. I'm not sure why it would be adding the extra cdata there I'll take a look.
Comment #94
ericduran CreditAttribution: ericduran commentedActually the cdata is added to all inline, settings, and external scripts.
That shouldn't be the case, good catch.
Comment #95
cosmicdreams CreditAttribution: cosmicdreams commentedstanding by for another 15 minutes or so if you want me to retest.
Comment #96
cosmicdreams CreditAttribution: cosmicdreams commentedIf there a specific lines of code you want me to test please paste them to the issue.
Comment #97
ericduran CreditAttribution: ericduran commentedAnd this should fix that extra cdata tag.
Comment #98
cosmicdreams CreditAttribution: cosmicdreams commentedtesting...
Works great!
from module
from theme
Comment #99
cosmicdreams CreditAttribution: cosmicdreams commentedIs it worth the time to test the patch if the javascript's type is 'inline' or 'file'?
Comment #100
ericduran CreditAttribution: ericduran commentedWe can test file, but that should just work like external.
I haven't tried inline but we should probably test that too. I think it'll work fine, but we should definitely testing.
Comment #101
cosmicdreams CreditAttribution: cosmicdreams commentedcool, I ran out of steam last night. I'll try to test this evening. I'll use drupal_add_js's examples as a basis for testing.
Comment #102
cosmicdreams CreditAttribution: cosmicdreams commentedOK, I have an hour before I pass out. LET'S DO THIS!
Going to attempt to test with a modified set of examples that are shown in the api doc for drupal_add_js.
Testing...
Looks like I have some interesting results. First, for my test case I added the following to Bartik's bartik_preprocess_html:
Here's what resulted in the header
and the footer
Comment #103
cosmicdreams CreditAttribution: cosmicdreams commentedthe
...is in there because of the module I previously made that puts that code there.
Does everything meet expectations?
Comment #104
cosmicdreams CreditAttribution: cosmicdreams commentedThis kind of looks fishy, are those random characters at the end expected?
Also, what's putting lose empty lines in?
Comment #105
ericduran CreditAttribution: ericduran commentedAll that looks correct. Those random characters are appending on purpose.
Can we get a couple of more people review this. As this should pretty much be wrapped up or at least extremely close.
Thanks.
Comment #106
cosmicdreams CreditAttribution: cosmicdreams commentedEven the extra lines of whitespace?
Comment #107
sunAnd now (you figured it): Each and every detail and expectation you manually tested and talked about in those last 5,000,000 comments needs to live in an automated test.
Comment #108
ericduran CreditAttribution: ericduran commentedYea, I kinda saw it coming. Currently there isn't any test for any of this. So I guess we should add it here.
Comment #109
mikeytown2 CreditAttribution: mikeytown2 commentedJust saw this issue; for those looking for something like this in D6 you can put a prefix or a suffix around JS tags with the Advanced CSS/JS Aggregation module. hook_advagg_js_extra_alter() is the hook used to set the prefix or suffix for JS (add in conditional comments).
Comment #110
cosmicdreams CreditAttribution: cosmicdreams commented@sun, I assume you mean me. I'll look into what I need to know in order to add test cases.
Comment #111
klonos...here you go: SimpleTest project, Getting Started with SimpleTest, Simpletest Tutorial (Drupal 6), SimpleTest Tutorial (Drupal 7)
...and here's QUnit (a JavaScript test suite).
Comment #112
ericduran CreditAttribution: ericduran commented@sun, We don't need to add Test core already has all this being tested in the JavaScriptTestCase in the common.inc file.
I'm going to run a re-test but this is looking correct to me. @sun, @effulgentsia wanna give it a pass?
Comment #113
ericduran CreditAttribution: ericduran commented#97: 0001-Issue-865536-by-ericduran-Added-drupal_add_js-is-mis.patch queued for re-testing.
Comment #114
JacineChanging tags per #1307612: Decide on HTML5 Initiative Sprint Tag(s).
Comment #114.0
JacineRevamped issue summary.
Comment #114.1
ericduran CreditAttribution: ericduran commentedUpdated issue summary.
Comment #114.2
ericduran CreditAttribution: ericduran commentedUpdated issue summary.
Comment #114.3
ericduran CreditAttribution: ericduran commentedUpdated issue summary.
Comment #115
ericduran CreditAttribution: ericduran commentedI'm going to assigned this issue to @effulgentsia as he's been my main reviewer on this issue.
effulgentsia, feel free to assign it back to me if you can't review it.
Thanks.
Comment #116
ericduran CreditAttribution: ericduran commentedOh, I can't do that lol. Never mind.
Comment #117
effulgentsia CreditAttribution: effulgentsia commentedI can :)
I'm trying to clear my schedule to be able to review this tomorrow. If I can't for some reason, I'll unassign myself. If anyone else wants to review though, please don't let this issue being assigned to me stop you.
Comment #118
sun1) [and elsewhere] No quotes around 'browsers'.
2) [and elsewhere] In phpDoc, "JS" is always phrased out as "JavaScript". (also note the capitalization)
3) Not sure whether the "item", or rather full sentence, ultimately makes sense.
In my entire lifetime of Drupal work, I tried to specify 'settings' as type for drupal_add_js(), only to find out that it's 'setting'.
As of now, these seem to be singular, so the constant should be JS_SETTING (singular), too.
The empty array should be applied or assumed by default.
drupal_get_js() calls into drupal_add_js()...?
That's a mind-blowing recursion.
Looking further down, it looks like this code was merely moved, so separate issue.
There should be a blank line between switch cases, unless a case falls through to the next.
-17 days to next Drupal core point release.
Comment #119
ericduran CreditAttribution: ericduran commentedA couple of replies to @sun's comments:
The 'browsers' => array(), option is applied by default. What you see me adding here is is for the edge case ''misc/drupal.js' being that the way that JS file is placed on the page is kinda hacky. The array is hard-coded in there without actually running through drupal_add_js which would actually populate all the correct settings for every other JS file. If we decided to change that I think it'll have to be on a separate issue.
Same goes for the the drupal_add_js($setting, 'setting'). This is just a move of whats there, so we can follow it up on another issue.
The other comment related stuff, and the JS_SETTING is now fixed.
Also I left the comment as "should load the JavaScript item" as thats what it says when using css, also Items is not the most descriptive word, but I think it beats "should load the Javascript file, settings array, inline JS, etc.."
Comment #120
cosmicdreams CreditAttribution: cosmicdreams commentedThis patch builds on top of ericduran's last patch. This one also includes the requested change to comments where JS should now be Javascript
Comment #121
ericduran CreditAttribution: ericduran commented@cosmicdreams, I avoided changing anything extra on purpose ;) Those other changes seem tiny but any unnecessary changes really slow down a patch.
Instead all other changes that aren't related to this patch can be made in a separate issue.
Comment #124
cosmicdreams CreditAttribution: cosmicdreams commentedI'm not so sure that the jQuery approach to solving this problem is actually better. A cursory review of available internet-based information on jQuery.support shows that it's not really meant to apply scripts per browser type. It's meant for feature detection. That's not the aim of this patch.
The other approach that jQuery offers is to use $.browsers. This option is not ideal either since it is likely that a future release will cut $.browsers out of jQuery altogether.
Using conditional comments for browser specific scripts is a valid and decent approach. It allows us to group together legacy browser specific scripts so that support for those browsers can be removed once those older browsers become marginalized.
I'm sure that there will be a strong use case for using feature detection to conditional enable specific behaviors. But that really is a different patch.
@ericduran, sorry about that. I recommend the patch in #119 as the final patch. How can we move this issue forward?
Strange, I don't see the comment that I replied to anymore. where did 122 and 123 go?
Comment #125
effulgentsia CreditAttribution: effulgentsia commentedThis takes #119 and makes some changes to drupal_pre_render_scripts() that I hope makes it more grokable. I'm gonna get some lunch now, but still want to do some manual testing with this, and think about whether any automated tests need to be added.
Comment #126
effulgentsia CreditAttribution: effulgentsia commentedThis one just cleans up some phpDoc, mostly changing JS to JavaScript as per #118. Also, this passes my manual testing. I'm still working on adding some automated tests.
The manual tests I ran was to compare D8 HEAD to this patch, for each of these 4 pages:
For each of these, I saved the html source output with HEAD, and with this patch, and diffed the two to make sure there's no difference between the order or contents of any SCRIPT tag.
Comment #127
ericduran CreditAttribution: ericduran commentedThis is definitely more grokable :)
Comment #128
effulgentsia CreditAttribution: effulgentsia commentedWith tests. The tests could use review, but otherwise, I think this is RTBC.
Comment #129
effulgentsia CreditAttribution: effulgentsia commentedThis shouldn't be a bitwise operator. While fixing that, I refactored the new function this is in, drupal_aggregate_js(), a bit.
Below is just a single patch. d.o. duplicated it for some reason on upload.
Comment #130
ericduran CreditAttribution: ericduran commentedSweet, I guess we did need more test ;-)
Comment #131
ericduran CreditAttribution: ericduran commentedI took the last patch for a spin,
- Tested as much Ajax/JS interaction in core as I could find. Also tested with the few JS devel has.
- Tested adding various JS each with different weight and group number. The order shows correctly.
- Also Tested Groups with Aggregation turn on.
- JS Settings show up last (unless I use a group weight higher than JS_SETTING, This was not possible before, but is a great feature addition)
- I can successfully pass in the 'browsers' options and have drupal_pre_render_conditional_comments render them properly,
Even thought I worked on a big chunk of this, I'm going to mark this RTBC as per @effulgentsia comment on #128 and my Review.
I'm sure this could use another pass from @sun and a couple of other people just to be sure.
Comment #132
cosmicdreams CreditAttribution: cosmicdreams commentedI concur with RTBC status based on previous tests. Will test it again tonight.
Comment #133
cosmicdreams CreditAttribution: cosmicdreams commented@effulgentsia
Maybe I'm not understand the AggregationTest well enough but it doesn't seem that it is testing the new 'browsers' argument in that test. Should it?
Comment #134
effulgentsia CreditAttribution: effulgentsia commented@cosmicdreams: I don't think it needs to. Of the three new tests in #129, only testBrowserConditionalComments() tests the new feature added in this issue. The testVersionQueryString() test tests a regression that had occurred in earlier versions of this patch (see #62.2), so that flagged that this test is needed. The testAggregation() test is probably something that should have been added in #769226: Optimize JS/CSS aggregation for front-end performance and DX, but since it wasn't there, here seems like a good place to do it, considering that the complete refactoring of drupal_get_js() that's in this patch could have caused a regression here, though I don't think such a regression was actually introduced in any of the submitted patches on this issue.
Comment #135
sunThere should be no quotes around the keys/property names.
Looks like these defaults should live in system_element_info() instead?
-19 days to next Drupal core point release.
Comment #136
ericduran CreditAttribution: ericduran commentedThis fixes the quotes around the keys/property names.
@sun, These defaults aren't the default for the actual "script" element. If just the default settings we're using for generating the
<script>
tag.Comment #137
effulgentsia CreditAttribution: effulgentsia commentedRight. This patch adds a 'scripts' element to system_element_info(), but we have no singular 'script' element type that we can add defaults for.
Comment #138
ericduran CreditAttribution: ericduran commentedI'm tempted to mark it RTBC again :-p lol
Comment #139
effulgentsia CreditAttribution: effulgentsia commentedI am too. But let's let sun do it when he's satisfied.
Comment #140
sunExcellent!
Comment #141
JacineHooray!!! :D Thanks so much everyone. You guys rock. This makes me so happy. :)
Comment #141.0
JacineUpdated issue summary.
Comment #142
ericduran CreditAttribution: ericduran commentedYay!, I edited the summary to reflect its RTBC status.
Comment #143
catch#136: 865536-136.patch queued for re-testing.
Comment #145
ericduran CreditAttribution: ericduran commentedre-rolling now.
Comment #146
ericduran CreditAttribution: ericduran commentedugh, I didn't realized the move to /core happen already :-/
Comment #147
JacineFinally something I can actually help with for this patch! Here's a re-roll for ya.
Comment #149
aspilicious CreditAttribution: aspilicious commentedLets try this one
Comment #150
ericduran CreditAttribution: ericduran commentedThe last patch seems, good. The diff looks different to me, but it might just be too early for me.
We'll wait to see what the test bot says.
Comment #151
cosmicdreams CreditAttribution: cosmicdreams commentedbot likes it. Back to RTBC
Comment #152
effulgentsia CreditAttribution: effulgentsia commentedConfirming RTBC.
Yeah. Doing a diff between the #136 patch and the #149 one, there are a bunch of differences that aren't real, but an artifact of the diff algorithm used to generate #149. Attached patch is just a
git apply
of #149 followed by agit diff
from my computer, which seems to generate a cleaner cross-diff with #136. Leaving as RTBC, because there is no difference between #149 and this one other than the diff algorithm used.Comment #154
effulgentsia CreditAttribution: effulgentsia commented#152: 865536-152.patch queued for re-testing.
Comment #156
scor CreditAttribution: scor commented#149: 865536-149.patch queued for re-testing.
#152 is just a reroll of #149. Let's keep the patch #149 since it passed the tests earlier (let's see if it passes the test again).
Comment #157
scor CreditAttribution: scor commentedreuploading #149 which is passing tests...
Comment #158
JacineHey, so can we just commit this one? It looks fine.
Comment #159
cosmicdreams CreditAttribution: cosmicdreams commented@Jacine, isn't that your call as the HTML5 initiative owner?
Comment #160
Jacine@cosmicdreams Hehe, well actually I don't have commit access. I think the patch in #157 is fine, so I marked it RTBC. Just trying to figure out what @effulgentsia is seeing and I am not, if anything. :)
Comment #161
effulgentsia CreditAttribution: effulgentsia commented#157 is great. I was responding to #150 and for the benefit of anyone else who likes to double check incremental patches by diffing them against an earlier patch, but considering there's some mysterious technical difficulties with #152, we should just go with #157. Thanks for everyone's dedication and work on this issue!
Comment #162
JacineGreat! Thank you @effulgentsia. :D
Comment #163
ericduran CreditAttribution: ericduran commentedCool.
Yay, this is a wrap :-p
Comment #164
catchCommitted/pushed to 8.x, thanks!
This needs a change notification.
Comment #165
ericduran CreditAttribution: ericduran commentedAwesome! :)
Comment #166
ericduran CreditAttribution: ericduran commentedChange notice has been created at http://drupal.org/node/1330682
I'll leave the title as is and leave it active, until I get a better description in there.
Comment #167
JacineThanks so much @ericduran. I improved up the change notification, so marking this fixed.
Comment #169
hass CreditAttribution: hass commentedAs suggested in #1471382: Add IE-conditional classes to html.tpl.php, need a backport for adding htm5shim in D7. drupal_add_html_head() places places html5shim too early.
Comment #170
effulgentsia CreditAttribution: effulgentsia commentedAs discussed in http://groups.drupal.org/node/210973, backporting new features/APIs to D7 is okay as long as we're not breaking backwards compatibility of existing APIs / data structures. I don't yet know what that means for this patch, but I suspect it's doable, if someone wants to give it a shot.
Comment #171
webchickThis isn't critical.
Comment #172
ericduran CreditAttribution: ericduran commentedOk, I'll try a re-rolled later today.
Also removed the sprint tag since this isn't related to the html5 sprints.
Comment #173
fubhy CreditAttribution: fubhy commentedI hope you didn't stay up for 30 days :P (j/k). Are you still going to do this? Otherwise I can take this over :).
Comment #174
ericduran CreditAttribution: ericduran commentedLol, I forgot about re-rolling this one. I'm going to get back into this one because I rather this gets in before #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page that way drupal_add_js and drupal_get_js can both be inline in D7 and D8.
Comment #175
ericduran CreditAttribution: ericduran commentedOh but don't let me stop you :) I think this should be a pretty clean re-rolled.
Comment #176
tim.plunkettRerolled.
Comment #178
tim.plunkettEr, did that wrong. The tests patch was just the fix instead, and there were still 'core/' bits left over.
Take two!
Comment #179
ericduran CreditAttribution: ericduran commentedThis looks good to me.
A couple of nice extra wins that we get from this patch is now having the ability to change aggregate callback in D7 ;-)
Comment #180
fubhy CreditAttribution: fubhy commented+1 Review
Looking good! Lets get this commited :P
Comment #181
AaronELBorg CreditAttribution: AaronELBorg commentedLooks good. I wish it was committed already! Now I gotta wait........
Nuts.
Comment #182
hass CreditAttribution: hass commentedGreat guys. It just works. RTBC.
I guess some others may need this, too. Copy and Paste :-)
Comment #183
Mark Trapp#182: off-topic, but for future visitors who land on this, you really shouldn't link directly to the shim.
Comment #184
JacineI'd like to see this committed to D7 as well.
Comment #185
perforator CreditAttribution: perforator commentedWhen will it be commited? Who is in charge? Many people would like to deploy YAML on D7 (http://www.yaml-fuer-drupal.de/de/changelog/4.0.1.16) without patching D7.
Comment #186
nod_It's RTBC and will get committed once a D7 core committer have the time to. We are over thresholds meaning that this kind of patch will not get committed until a few major and critical are resolved. Usually the oldest RTBC are committed first, you bumping it just made him move down this list, delaying some more it's commit.
The easiest way to know if a patch will get committed is to ask on IRC: #drupal-contribute and leave the issue alone unless you have something to contribute, if you found a RTBC patch introduced a bug for example.
You can see who is core commiter on the core project page. They are busy people, they may not be available in IRC very often.
Comment #187
David_Rothstein CreditAttribution: David_Rothstein commentedYup, I've had this patch on my radar for a little while, and although I'm not sure if it actually falls under the "don't-commit-if-we're-over-thresolds rule" (since I think it is properly classified as a task, i.e. missing API, rather than a pure feature request), it's certainly pretty close, so it has to be lower priority than other issues.
Anyway, I'm definitely supportive of adding the 'browsers' option to D7 but what concerns me a bit is that the patch is doing a lot of other things too (JS_SETTING, group and aggregate callbacks for JavaScript, etc). All of that totally made sense for Drupal 8, but it's a lot of code churn to commit to a stable branch, especially when it touches a pretty fundamental part of Drupal. I appreciate from the above comments that the patch has been very well-tested, but also appreciate that if we release a change this big to 400,000 sites it will get tested in ways we haven't thought of also :)
So I'm wondering if we can get some discussion about whether for Drupal 7, it would be possible/better to go back to some of the earlier, more limited patches in this issue, that just added the 'browsers' option and didn't try to do other refactoring? Certainly that would make this patch a lot easier (and quicker) to commit confidently to Drupal 7.
Comment #188
David_Rothstein CreditAttribution: David_Rothstein commentedJust to be clear, what I'm roughly talking about would look something like the attached (with the tests added in too, of course!).
Comment #189
hass CreditAttribution: hass commentedI understood above patch was just a easy re-role of a D8 patch that has been tested and discussed very well. I belive it's just waste of time reinventing any other wheel and maintaining different code logics. This asks for more troubles. We should commit this now as is and don't waste more valuable developers time. I have not seen any issues at all, but can think of others if we start implementing things different. There should be enough tests in D7 that make sure nothing breaks.
Comment #190
attiks CreditAttribution: attiks commentedI share @David_Rothstein concern, but I'm all in favor of the patch in 178, it is an improvement for D7 and will make sure that upgrading from D7 to D8 will be possible in the future. Another remark is that according to 170, this kind of backporting is allowed, and that remark was made 2 months ago. (OT, #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page will be even a bigger change code wise)
Do we need more testing of real life scenario's: absolutely, feel free to have a look at testswarm, it might make testing easier
Only concern: Doesn't this mean that settings are added after the javascript from the theme? Anf if so, can it break existing themes?
Comment #191
attiks CreditAttribution: attiks commentedDidn't mean to change status
Comment #192
ericduran CreditAttribution: ericduran commentedYea, I also assumed this patch wasn't going to be backport-able at the time because of all the changes.
We could roll back to a simpler patch, but that would make a bunch of other bug fixes that are happening on D8 harder to backport. Off the top of my head I know at least 2 other issues that would be simpler to fix in D7 if this patch gets it. That being said I'm not saying they can't be fixed without this patch, is just nicer to have D7 and D8 be in line so the backports are a lot simpler and cleaner.
It should be pretty easy to add the browser option and the test without all the other changes, but these changes are probably welcome changes for anyone on D7 that this would cause problems for (being that it would probably be cleaner to do what ever they were doing before). Also I imaging this *might* break some contrib such as the speedy module (But that being said it'll now be easier for a module like speedy to accomplish is task).
If we want a lighter patch then lets go that route, but in the long run it's going to be more work. :-/
Comment #193
David_Rothstein CreditAttribution: David_Rothstein commentedThis is the behavior before the patch also (the settings are hardcoded to be added last, after the theme). So the default ordering will remain the same.
As for the general issue, let me lay out for a second how I'm seeing this:
All those put together seem like good reasons not to take the risk.
I don't, off the top of my head, have a great example of how this particular change might break something... probably in most cases it will be OK. But it does change data structures, and I don't think it's worth spending lots of time trying to think about the edge cases that might break as a result of that if we don't have to. (One example, perhaps: If someone is using hook_js_alter() to do heavy alteration of the JavaScript groups, then after this patch those alterations suddenly start affecting JavaScript settings when they didn't before, you could wind up with the order messed up... e.g., with JavaScript settings added to the page before jQuery, which will break everything.)
Happy to hear more opinions, but I think we really should follow the policy here unless there's a really good reason to break it, and that means we shouldn't change existing structures especially if there's no need to.
Comment #194
David_Rothstein CreditAttribution: David_Rothstein commented@attiks (#190):
Can you clarify this concern? How will this issue affect the D7 to D8 upgrade path?
@ericduran (#192):
Out of curiosity, what are those issues? I agree that keeping the D7 and D8 codebases in sync is good when possible, but by definition of course we can't always keep them in sync; if we did there would never be a Drupal 8 :)
Comment #195
sherpadawan CreditAttribution: sherpadawan commented#35: handle_browsers_in_drupal_get_js-865536-35.patch queued for re-testing.
Comment #196
webchickThat sounds like "needs review."
Comment #197
hass CreditAttribution: hass commentedNo, its ready, you just need to commit it.
Comment #198
webchick#193 and #194 haven't been answered yet. Furthermore, until David gets the help he's requested on resolving 7.15 release blockers, I'm not planning on committing any features to 7.x.
Comment #199
hass CreditAttribution: hass commentedComment #200
webchickhass, please knock it off.
Comment #201
attiks CreditAttribution: attiks commented@David_Rothstein sorry for the late reaction, my concern is mainly about contrib modules needing to chance a lot of code, and the difficulties of backporting D8 patches to D7.
The biggest problem AFAIK is that we don't have any proper javascript testing, so I understand why you're hesitant to commit this, but the current handling of the javascript in D7 isn't 'optimal'.
I would love to see either patch backported to D7 because no browsers option is a PITA.
Comment #202
effulgentsia CreditAttribution: effulgentsia commentedIndeed. Here's #178 updated to remove the JS_SETTING part.
As per above, changing setting elements from JS_LIBRARY to JS_SETTING is indeed an API change with respect to hook_js_alter(). I don't think the addition of a 'scripts' element with group and aggregate callbacks are though: that's just an API addition / implementation change.
Comment #203
effulgentsia CreditAttribution: effulgentsia commentedI'm not sure why the interdiff is showing the above. I don't see any change between #178 and #202 with respect to that. Seems like a bug with my interdiff utility.
Comment #204
effulgentsia CreditAttribution: effulgentsia commentedThis one's better. Interdiff is relative to #178. Same extraneous lines noted in #203 still appearing in the interdiff for some mysterious reasons.
To allay David's concerns, it would be nice if someone (or several people) could confirm the manual testing done in #126, both for D7 core, and for a site with various popular contrib modules enabled, especially any contrib modules that implement hook_js_alter() or do something related to JS aggregation, such as http://drupal.org/project/advagg.
Comment #205
mikeytown2 CreditAttribution: mikeytown2 commentedcurrently downloading all modules and will check for usage of hook_js_alter()
http://drupal.org/sandbox/greggles/1481160
Will get back by tomorrow for the details.
Comment #206
David_Rothstein CreditAttribution: David_Rothstein commentedRemoving the JS_SETTING stuff definitely does make me feel better (since it addresses the direct API change issue), but as I said also, this patch still seems like a pretty big code refactoring for a stable release....
Maybe someone can help me out: Is there any previous example where we committed a patch to a stable version of Drupal core that did a similar, major refactoring of an important core subsystem, for any purpose other than fixing a serious bug?
In general, I think we refrain from doing that in stable releases. For example, #330082: Refactor drupal_get_js() to use render layer similar to drupal_get_css() (the issue that originally proposed this refactoring) was bumped to Drupal 8 in January 2011 with a comment that said "Drupal 8 territory at this point" and no one disagreed there. And now we're 18 months past that.... I'm trying to follow how things have been done in the past as much as possible, and in my experience this is not how we tend to do things, especially for a feature/task (the 'browsers' option) which can be introduced in a simpler way.
If my assumptions above are wrong, I'm happy to be shown an example of that, though.
And I do understand that people want to keep the D7->D8 API changes for module authors (and D8->D7 backport process for core contributors) as simple as possible, but this is overall a losing battle :) There are already major differences between D7 and D8 in a number of key areas, and they will continue to grow as time goes on. This issue is really a small drop in the bucket compared to all that.
Comment #207
mikeytown2 CreditAttribution: mikeytown2 commentedoutput from
grep -l -r -i "_js_alter(" ./
in the dir containing all the 7.x modules.Comment #208
attiks CreditAttribution: attiks commentedFYI ./clientside_validation/clientside_validation.module isn't going to be a problem
Comment #209
mikeytown2 CreditAttribution: mikeytown2 commentedSomeone (not me) needs to check each modules usage of hook_js_alter() to see what the side effects might be if this patch goes into D7. If it does effect that module we need agreement from said module maintainers that the new change is ok with them. Does this sound right?
Comment #210
hass CreditAttribution: hass commentedCan we get this commited now after 7.15 is out, please? The patch was already RTBC.
Comment #211
windmaomao CreditAttribution: windmaomao commentedsubscrib +1
Comment #212
bleen CreditAttribution: bleen commentedwindmaomao: there is no longer a need to leave "subscribe" comments. Please use the shiney green "Follow" button at the top of the issue instead.
Comment #213
hass CreditAttribution: hass commentedCommit?
Comment #214
Dave ReidThe patch in #204 is not currently RTBC? Why would you needlessly ping an issue when it isn't? See #209 on how you can help get this moving towards RTBC.
Comment #215
hass CreditAttribution: hass commentedJust commit #178. It is RTBC. I'm using it since months, zero issues.
Comment #216
Dave ReidAnd you're using every single module that implements hook_js_alter() listed in #207 and have no issues? Because if you are not, this is not valuable feedback, and you are just wasting your and my time, so I won't waste anymore here.
Comment #217
mikeytown2 CreditAttribution: mikeytown2 commentedThat list is fairly old; fresh list from January 31, 2013
Comment #218
hass CreditAttribution: hass commentedThe only waste of time is that the patch that works well has not committed after 1year. And you are the only who put a veto in. This wastes 1000th of theme developers time.
Comment #219
mikeytown2 CreditAttribution: mikeytown2 commentedNot tested but based off of the code in question these modules should be OK:
http://drupal.org/project/availability_calendars/ - availability_calendar.module, availability_calendar_handler_filter_availability.inc
http://drupal.org/project/blackwhite/ - blackwhite.module
http://drupal.org/project/bootstrap_api/ - bootstrap_api.module
http://drupal.org/project/closure/ - closure.module
http://drupal.org/project/cookiebar - cookiebar.module
http://drupal.org/project/cookiecontrol/ - cookie_googleanalytics.module
http://drupal.org/project/domain - domain_conf/domain_conf.module
http://drupal.org/project/drupalforkomodo
http://drupal.org/project/drupal_ruble - commands/hooks.rb
http://drupal.org/project/easy_module - data/easy_module_hooks.data.php
http://drupal.org/project/elfinder - elfinder.module
http://drupal.org/project/equalheights
http://drupal.org/project/excluded/ - excluded.module
http://drupal.org/project/ftools/ - ftools.module
http://drupal.org/project/google_webfont_loader_api - google_webfont_loader_api.module (might be able to switch to drupal_add_js with type 'setting')
http://drupal.org/project/jquery_dollar - jquery_dollar.module (might be able to switch to drupal_add_js if setting the weight)
http://drupal.org/project/jstool - jstool.module (module seems like a bad idea)
http://drupal.org/project/navigation_timing/ - navigation_timing.module
http://drupal.org/project/clientside_validation/ - clientside_validation.module (looks like hook_init would be better then hook_js_alter in this case)
http://drupal.org/project/simpletest - simpletest.module
http://drupal.org/project/speedy - speedy.module
http://drupal.org/project/tabledragextra - tabledragextra.module
http://drupal.org/project/textmate - hook_js_alter.7.php
Modules that would need testing:
http://drupal.org/project/advagg/ - I'll make sure this works with the patch before a beta version is released.
http://drupal.org/project/ais/ - ais.module (might be able to switch to drupal_add_js if setting the weight)
http://drupal.org/project/bundle_aggregation/ - bundle_aggregation.module
http://drupal.org/project/core_library - core_library.module, modules/core_library_ui/core_library_ui.module
http://drupal.org/project/dojo_toolkit - dojo_toolkit.module
http://drupal.org/project/headjs - headjs.module
http://drupal.org/project/jqmulti - jqmulti.module
http://drupal.org/project/labjs - labjs.module
http://drupal.org/project/lc_connector - lc_connector.module, litecommerce DrupalConnector
http://drupal.org/project/librejs/ - librejs.module
http://drupal.org/project/coder - coder_review_7x.inc, coder_upgrade/conversions/call.inc
http://drupal.org/project/scriptjs - scriptjs.module
Comment #220
mikeytown2 CreditAttribution: mikeytown2 commentedThemes that would need testing:
http://drupal.org/project/adaptivetheme - at_core/inc/alter.inc
http://drupal.org/project/arctica - arctica/template.php
http://drupal.org/project/mothership - mothership/functions/js.php
http://drupal.org/project/simpleclean - template.php
(advagg could make the functionality of almost all of these hooks obsolete)
Comment #221
markhalliwellJust putting my .02 in, been using the latest patch (#204) for a while and haven't seen any real issues. Not going to get into the whole debate of whether it should be committed, but in my eyes this really should be committed. I was trying to figure out for the longest time why drupal_add_js() wasn't respecting the browser conditions. It doesn't make sense why drupal_add_css() supports this but not drupal_add_js().
Comment #222
jbrown CreditAttribution: jbrown commented#204: drupal-865536-204.patch queued for re-testing.
Comment #223
mikeytown2 CreditAttribution: mikeytown2 commentedAfter getting this working 100% in AdvAgg (no core patch required if using AdvAgg) I would say these are the modules that would need to be tested. The issue with these modules is they rewrite the aggregates by grouping them together in new ways or by doing some tricks with in-lining various scripts. They would most likely ignore any browser tags. Thus it wouldn't break badly, just the browser tags would most likely go missing.
http://drupal.org/project/bundle_aggregation
http://drupal.org/project/core_library
http://drupal.org/project/headjs
http://drupal.org/project/labjs - Will be ok if using AdvAgg, AdvAgg patch was accepted.
http://drupal.org/project/librejs
http://drupal.org/project/scriptjs
http://drupal.org/project/dojo_toolkit - This might be ok.
Comment #224
RedRat CreditAttribution: RedRat commentedNo progress for three years? :-(
Comment #225
altrugon CreditAttribution: altrugon commentedShouldn't this issue be ported into D8 before it is too late?
Comment #226
mikeytown2 CreditAttribution: mikeytown2 commented@altrugon
See #164
Comment #227
altrugon CreditAttribution: altrugon commentedyeah! missed that one sorry ;)
Comment #227.0
altrugon CreditAttribution: altrugon commentedUpdated issue summary.
Comment #228
japerryDepending on which gets in first, this patch for d7 will need to be rerolled against #1664602: Allow attributes to be passed to drupal_add_[css|js] (SRI) As these two patches conflict with each other.
Comment #229
markhalliwellGiven @David_Rothstein's concerns with modifying so much underlying architecture in how JS is handled, I decided to take the approach of the patch in #20. I agree with @sun's comments in #32 100%, but this was also for HEAD (8.x). This is a backport to 7.x and I agree with @David_Rothstein here. The approach here to very minimally, yet thoroughly, support browser conditionals in JS (mainly by simply calling drupal_render() instead of theme().
I've included the tests from #204 and also fixed a few issues and added some additional scripts to test with aggregation.
Let me know if this is a more appropriate approach for the backport and/or if I may overlooked something.
PS... I'm really tempted to mark this as a "bug" (not going to, but tempted). There is a lot of javascript that should only be loaded for IE. Take for example font icons. Modern browsers can handle CSS3 content properties, IE7 cannot. Many services include a JS script that will dynamically add the character for IE7 browsers, if this script is added (without the conditional comment wrappers) then you see double icons because the script is run on a page that supports the CSS3 content property.
edit: interdiff is from patch in #204
Comment #230
markhalliwellChanging order of files? Pass should be last I think.
Comment #232
markhalliwellAssigning for review
Comment #233
David_Rothstein CreditAttribution: David_Rothstein commentedThanks! Looks like almost the exact same approach I used in #188 so I don't really have any complaints myself :)
Main difference with that patch seems to be in how the key is calculated in the 'file' case of drupal_get_js()... I am not sure off the top of my head which approach is better. But overall, this looks like a good direction to me.
Comment #234
markhalliwellHeh :) I honestly didn't scroll as far as #188 for reviewing possible options (decided to start at the beginning and see the progressions of 8.x). But it's a good sign that two people came to the same conclusion. The idea with the file bit in my patch was to help "group" similar conditionals when it's not aggregated. Granted, it's rather trivial and ultimately should only include one conditional comment instead of multiple. What really matters is that the aggregated scripts are grouped properly (as evident by the tests). I could go either way.
Comment #235
mgifford@mark Carver - your patch applies nicely. You & @David_Rothstein had the same approach to this problem (with a year's space in between). It was committed to D8 3 years ago. What's the hesitation to having this marked as RTBC?
Comment #236
markhalliwell229: 865536-D7-229-PASS.patch queued for re-testing.
Comment #237
markhalliwellI guess both @David_Rothstein and I were just waiting to hear back from someone other than ourselves to RTBC it (since we both provided a patch). There is a little difference in how the key is handled between ours. So whichever one you deem appropriate, please feel free to RTBC it.
Comment #238
mgiffordTesting #229 With LibreJS (and labjs I think) against mikeytown2's list of modules in #223, most worked fine, but LibreJS gave me this nasty error:
Fatal error: Unsupported operand types in /home/sc431931bb6ed749/www/includes/common.inc on line 4347
It works with #188 but for some reason but drupal-add-js-browsers-865536-188-do-not-test.patch was built as just a prototype I think...
Bundle aggregation, Core Library, HeadJS, ScriptJS, Dojo Toolkit, AdvAgg worked fine.
Need more testing against #219 & #220 though. Not sure if we want to make advagg mandatory for those themes.
These modules would still need to be tested: Coder, lc_connector, jQuery Multi, Adaptive Image Styles (ais)
Along with these themes: AdaptiveTheme, Arctica, mothership & Simple Clean.
Do we need to reach out to these modules/themes?
Along with testing raw:
So, sadly I can't mark it as RTBC yet.
Oh ya, then we'd need to alter:
https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_ad...
Comment #239
markhalliwellI'll add the inline test in the next few days (unless someone else beats me to it).
I'm inclined to say no, however it does/will impact quite a few contrib project. Perhaps an issue should be created in each queue (@mgifford, could you do this?) asking them to test/review this patch. It should not, however, hold up this issue (as some projects may be stale/minimally maintained). If we get feedback, great, we'll address it.
hook_js_alter()
doesn't mention anything about the properties of a JS item. The patch already contains documentation updates fordrupal_add_js
.Comment #240
mgifford@Mark Carver - Yes, I'll make an issue in each queue (for impacted themes & modules) and tie them back here.
Comment #241
mfbThe fix for LibreJS module would be a trivial one-line patch, see #2295203: Drupal 7 Core Change: drupal_add_js() is missing the 'browsers' option (librejs)
Comment #242
Jeff Burnz CreditAttribution: Jeff Burnz commentedHonestly I never saw this as a contrib issue but rather a "whats out there in the wild that we don't know about" issue, as in all the site owners who are not programmers who can't easily fix a breakage without hiring someone to do it. I'm not sure that was part of the deal, aka the whole stable api thing.
Comment #243
markhalliwellUnfortunately, I have not be able to get to this issue to address #238/#239.
This is simply an API addition, a backport... not really a change. It's just introducing a new associative key in the JS array; an array that already existed. It's very unlikely that any existing site (or contrib module) will "break" with this patch (aka: WSOD).
The only reason anything would appear to "break" is someone previously introduced their own "browser" key in the JS array and did some sort of custom rendering to account for this. I can assure you (as a themer), that this is very highly unlikely. Most developers wouldn't even attempt to customize to that extent (or care enough to know how) and simply add this conditional JS to their
page.tpl.php
template.Technically speaking, the only real "breakage" that would appear to happen is when people start using said "browser" key and realized that some contrib module wasn't aggregating it properly and that is simply a new feature request in the contrib module.
It's a little sad that only librejs has responded to the issues @mgifford created. Enough time was given to warn these contrib projects, I say we go for it.
Comment #244
mikeytown2 CreditAttribution: mikeytown2 commentedI've had ZERO complaints with the browser key being supported in AdvAgg for the last 2+ years, so I agree with Mark that the risk here is very very low.
Comment #245
sonicthoughts CreditAttribution: sonicthoughts as a volunteer commented+1 no issues. please commit so themers can do this properly.
Comment #246
cilefen CreditAttribution: cilefen commentedIn order for this issue to be fixed it must be moved by a community member to "Needs review" status. In comments #239 and forward, there is a discussion about an API break that seems to have inclined toward "we don't need to worry about it".
Next, a community member must offer a review of the patch. Thorough reviews are preferred. I hope this helps!
Comment #249
markhalliwellComment #250
joseph.olstad+1 no reported issues with #204 (on D7.39), we've been using it for quite some time in production sites with over 200 contrib modules
Comment #251
ejanus CreditAttribution: ejanus as a volunteer commentedI need this functionality in a current project. Any updates if this will be integrated into core?
Comment #252
joelpittetRe-uploading #229 rerolled (it actually just applied without git, just line fuzzed).
I've not addressed any concerns from #243/ #238/ #239.
Comment #259
joseph.olstadCool, looks like this could soon make its' way into 7.x now that 8.x has it.
We're still using patch #204.
Comment #260
dsutter CreditAttribution: dsutter as a volunteer commentedRTBC+ patch #204
Comment #261
gdaw CreditAttribution: gdaw as a volunteer commentedRTBC +1 for patch #204 , D7 needs this committed
Comment #262
khu CreditAttribution: khu at Old Moon Digital commentedRerolled #229 for the latest release of D7.
Comment #263
natew CreditAttribution: natew as a volunteer commentedThis patch does not apply to drupal core 7.69. It appears due to changes introduced in https://www.drupal.org/node/3051370 (in 7.68). I took a stab at re-rolling it and read through all this issue. However, I am not sure how to aggregate files with the same browser tag. I thought I would contribute my try, please feel free to ignore if this is wholly wrong, or provide direction on how I can help?
It is this section of code which now resides in drupal_pre_render_scripts (not drupal_get_js)...
It was
and now I think should be something like the following? or maybe pass it js_element through drupal_render?