Problem/Motivation
Currently there is no way to pass #attributes to the drupal_add_js function. This is not a feature that is widely used, however, there are unique cases where the <script>
tag generated needs to have available an id="" or other unique attributes to that tag.
Proposed resolution
By initializing the #attributes key of the $js_element array we can easily pass specific #attribute elements in our drupal_add_js calls, and sense theme('html_tag', array('element' => $js_element)); handles #attributes passed to it, all additional support is inherited. Here is an example of the new attribute code working in the case 'external' type of drupal_add_js is invoked. The patch of course accounts for all cases of 'type':
case 'external':
$js_element = $element;
// Attach custom attributes to the element.
$js_element['#attributes'] = !empty($item['attributes']) ? $item['attributes'] : array();
// Preprocessing for external JavaScript files is ignored.
if ($item['defer']) {
$js_element['#attributes']['defer'] = 'defer';
}
$js_element['#attributes']['src'] = $item['data'];
$processed[$index++] = theme('html_tag', array('element' => $js_element));
break;
Attached to this feature request is the patch containing this new fix.
Comment | File | Size | Author |
---|---|---|---|
#176 | attached-attributes-1664602-176.patch | 11.79 KB | cboyden |
#174 | attached-attributes-1664602-174.patch | 11.82 KB | merauluka |
#167 | 1664602-167-attached-attributes.patch | 10.52 KB | ChristianAdamski |
#160 | 1664602.patch | 10.57 KB | Pol |
Comments
Comment #1
Wim LeersAnother example is using Aloha Editor, where you specify which plug-ins to load through a
data-aloha-plugins
attribute on the<script>
tag: http://aloha-editor.org/guides/using_aloha.html#using-plugins.Rerolled the patch for D7: now it actually applies and it has been cleaned up. Feature requests are against D8. Should also apply to D8.
Comment #2
Wim LeersComment #4
sunProper patch for D8. :)
Comment #5
Wim LeersThanks, @sun! :)
Comment #6
sun1) Does this require tests?
2) What happens if JS aggregation is enabled?
In light of 2), is this a good idea to do, or isn't the idea flawed in the first place? :-/
Comment #7
Wim Leers1) Probably.
2) That was my thinking as well… :/
Comment #8
nod_Needs to be taken into consideration while working on #1490312: [META] Improving CSS and JS preprocessing.
I'd be ok with saying attributes are fine on single scripts and gets removed on aggregation, once we have aggregation groups (can't seem to find the issue talking about it though) we might be able to add attributes to them as well.
Though if aggregation groups get aggregated, we're boned.
Comment #9
nod_Comment #10
babbage CreditAttribution: babbage commentedI applied the patch in #1 to a Drupal 7 install in order to get the Edit module and Aloha Editor working, as per #1702520: When using compiled version of Aloha Editor: "no method 'deferInit'". I found that this patch corrected the problem when js aggregation was switched off, but not when it was switched on. As far as I could tell, this wasn't simply a problem of my not clearing a cache or similar. Not sure if this is an issue with the approach taken in this patch, or just an interaction with the Aloha Editor bug, but post this here to ensure this aspect of the patch is scrutinized. :)
Comment #11
Wim Leers#10: thanks :) That's indeed important to also tackle.
Though one could of course argue that it makes no sense to keep these extra attributes when aggregated: imagine that 5 different JS files set the same attribute to different values. Which one should then be used on the aggregated script tag?
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedIf the attributes are important, then I would expect different attribute values to result in different aggregates. Currently, drupal_group_js() adds several things to $group_keys to force new aggregates. Might make sense to add attributes to that list. Also note that #865536-204: drupal_add_js() is missing the 'browsers' option backports the drupal_group_js() architecture to D7, though there's still discussion on that issue as to whether there's enough justification for it: this issue may help provide that justification, but even if not, D7's drupal_get_js() has an equivalent
$key = 'aggregate_' ...
line to support this too.Comment #13
Wim Leers#12: thanks, that's very valuable feedback!
Overall, I think we should just *not* have attributes on script tags, but it's not always up to us ("us" being Drupal) to decide that…
Comment #14
Wim Leers#10: easily solved by setting
preprocess = FALSE
:) See e.g. #1787940: Allow for JS aggregation yet prevent the data-aloha-defer-init attribute on aloha.js' script tag from being removed.Comment #16
quicksketchThough this patch is a bit strange, it's not unprecedented. The "defer" attribute we've allowed to be added to scripts for as long as we've had drupal_add_js(). When the JS aggregator went in, we made it so that the "defer" attribute only works when the individual file has aggregation turned off:
From drupal_get_js():
Adding arbitrary attributes is no different than what we're doing already for the defer attribute.
Comment #17
quicksketchMy example in #16 is from D7. In D8 the relevant code has been moved to drupal_pre_render_scripts().
So overall this suggestion makes no unusual changes. However it needs to be rerolled so that the attributes list is moved into this same IF statement so that has effect ONLY when aggregate is set to FALSE. We should update the documentation to reflect this requirement too. It wouldn't hurt to do the same thing for async and defer, since they have the same requirement already, but it's not mentioned anywhere.
Comment #18
sun@quicksketch: The entirety of that code is located in an
else
control structure, whereas the corresponding if to that is:Comment #19
sunAdditionally, I think this issue is semi-obsolete — AE folks have been notified of the problem and are working on a completely revamped library loading already:
https://github.com/alohaeditor/Aloha-Editor/issues/737
I don't think it is worth to pursue this issue, but I'll leave it to @Wim to won't fix it.
Comment #20
quicksketchHeh, looks like in D8 the "defer" and "async" options are entirely lost if you enable aggregation, so this moves to a bug report.
To fix this, I propose we kill two birds with one stone. We replace the "defer" and "async" options with a single "attributes" option. If you want defer or async, you use the attributes array. If you want custom attributes like Aloha requires, just add it. I think use of async and defer are rare enough that we can easily change this behavior without upsetting anyone.
Before:
After:
If you add any custom attributes, aggregation is forced to be disabled for that file, as it would if you set $options['cache'] = FALSE.
Comment #21
quicksketchUpdating tests.
Comment #22
quicksketchThis version includes an additional test to ensure that attributes are kept when JS aggregation is enabled. As mentioned above, the script is not aggregated with other files when attributes are added.
Comment #23
quicksketch@sun: Sorry I didn't see your responses up above. Honestly I'm not sure what the requirement is for Aloha currently (I'm just trying to get set up to test it). I think this approach makes sense from an implementation standpoint all-around, plus it fixes a bug. But if it turns out that Aloha fixes this on their side then that's great. The new approach allows additional flexibility and increases consistency with normal Drupal paradigms, but I would consider it an unnecessary change on the whole if Aloha isn't going to need it. Then again, you never know when you'll need something like this and it's got its upsides. In the grand scheme of things I'd say this would be insignificant in affecting the D7->D8 porting and learning curve.
Comment #24
barraponto CreditAttribution: barraponto commentedIt is common to add ids and custom types to script tags when you're using js libraires to do the templating client-side. See http://handlebarsjs.com/ for example.
Comment #25
sun@quicksketch:
I didn't expect substantial changes here, but I've to say, I really like your API function argument simplification here, a lot.
First things first, resolving this issue will "resolve the bug", on the cost of performance (in case aggregation is enabled). That is OK-ish and could indeed be expected. Specifying custom attributes is edge-case either way, so 0.001% will be affected.
Replacing two custom array-of-doom options parameters with a single, standard 'attributes' parameter makes a lot of sense. (definitely not backportable though)
I'm not up to speed with regard to cross-HTML4 and HTML5 browser engine interpretations of the 'defer' and 'async' attributes though -- do they need any values, and if so, do the values have to be exactly "defer" and "async" respectively?
If the answer to both is no/anything, then we're good to go with replacing
array('defer' => TRUE)
witharray('attributes' => array('defer' => TRUE))
.Otherwise, we could still technically decide that "developers should know that they need specify certain values" and do no special-casing. Alternatively, we could as well ensure proper values built-in.
Aside from that, this patch looks pretty good.
Comment #26
quicksketchThanks @barraponto, that's an absolutely excellent example! Well seems like we should push forward with this then.
I tried the exact example that HandlebarsJS would require and found that it worked perfectly with the patch in #22. I had thought that changing the "type" attribute would have been a problem, but since D8 doesn't output a type attribute by default, setting type to something entirely custom like "text/x-handlebars-template" worked fine.
Comment #27
quicksketchI think that's probably the best way to go. Let's not get in the way of developers. If they want something like array('defer' => 'mypants') then we should let them without any kind of correction.
Comment #28
quicksketch@sun: P.S. @jenlampton *really* wants to give you a free plane ticket to BADCamp. It's *next weekend*! :)
Comment #29
Wim Leers1) Aloha currently still needs it, but I'm working with them to get it removed. Their loading mechanism makes sense given Aloha's history, but if they're aiming to be the best, most widespread, most integrable WYSIWYG editor, then they should get rid of their "aloha-defer-init" attribute. They acknowledge this, and they're working on improving loading/building.
2) Aloha currently needs it, and it may only be post-feature freeze that they'll get rid of it. So, from that POV, it's not ideal to not fix this issue.
3) @barraponto pointed out valid use cases. It's also better for DX if developers can integrate random JS libs without having to curse core.
4) @quicksketch clearly argumented this is in fact related to an hitherto unreported bug.
5) @quicksketch's patch cleans up the API.
I think a follow-up issue could/should be an update to the aggregation logic so that if there's >1 script with the defer and/or async attributes (or more generally, any time there's >=2 JS files with the same set of attributes), they can be aggregated, while maintaining their attributes. However, it doesn't make sense to implement this right away IMO, it's more sane to defer (pun intended) the implementation of that to the grouper/bundler that is yet to be implemented over at #352951: Make JS & CSS Preprocessing Pluggable.
I'm very much in favor of the patch in #22 being committed. It looks RTBC to me. I pinged @nod_ and @Rob Loach for reviewing this issue: https://twitter.com/wimleers/status/262908598810730497. Hopefully one of them can set it to RTBC.
Comment #30
RobLoachIndirect benefit of this is that it might work with Drupal\Core\Template\Attribute too. Haven't tested though...
Comment #31
nod_Looks good to me.
Comment #32
catchSo this is disabling aggregation when there's attributes. Are we sure we don't want to make an aggregate group for async and/or defer instead?
Comment #33
quicksketchThis patch just restores the same behavior that we have in Drupal 7. We weren't grouping together defer/async files then either. I don't think we want to get into custom aggregation groups for a couple reasons. It's uncommon enough that we have *any* files that use defer or async to begin with and we don't have any idea what attributes people are going to add to their scripts since they can be things other than async/defer. In some situations (similar to the Handlebars.js example), grouping files together by common attributes could actually break the functionality of the script.
Comment #34
sunI agree with #33.
Comment #35
catch#33 makes loads of sense. Committed/pushed to 8.x.
Comment #36
Wim Leers#32: that's what I said in #29 :) It's a new feature, and it can be a follow-up. Glad to see this has been committed :)
Comment #37
quicksketchAs an API change, I don't think this needs backporting. Aloha has solved this problem through a hook_preprocess_html() (though it's not pretty) in D7.
Comment #39
catchI just ran into this trying to use require.js in Drupal 7, there's no way to add arbitrary attributes with drupal_add_js() and while it does introduce a feature here there's no API breakage, fixes a valid bug, so feels backportable.
Moving back to 'to be ported'.
Comment #40
catchHere's a backport. Leaves the 'defer' option, adds 'attributes'.
I checked what aloha does and it's using drupal_add_html_head(). That puts the JavaScript before stylesheets get rendered, which is poor for front end performance since it blocks rendering of the page and therefore delays the stylesheets getting downloaded.
The two new test methods passed locally.
Comment #41
effulgentsia CreditAttribution: effulgentsia commentedThis can be backported without #865536: drupal_add_js() is missing the 'browsers' option, but if we want to make it easier to keep this flow consistent between D7 and D8, then I suggest getting that issue in too.
Comment #42
effulgentsia CreditAttribution: effulgentsia commentedx-post.
Comment #43
Anonymous (not verified) CreditAttribution: Anonymous commentedI've tried the patch in #40 and it works great. I'm able to add the right script tag for require.js.
Produces:
It would be really nice to get this into core.
Comment #44
David_Rothstein CreditAttribution: David_Rothstein commentedSo this is the part that looks like an API change:
(In particular the check for $item['defer'], since that's an existing property rather than new one - this will turn off JS aggregation for files that were previously getting it.)
That change seems to make sense and fix a bug; however in #16 @quicksketch sort of suggested that the current behavior (where you have to specify 'preprocess' FALSE manually on the file if you want your 'defer' to take effect) was intentional? Does that mean that although it's extra work for the developer, it should be left alone for Drupal 7?
Either way, if we are going to change that behavior it should be documented, so I've added it to the attached patch. And also fixed a minor issue in the tests (it was adding a library that doesn't exist) while I was at it.
Comment #45
quicksketchI don't think this behavior was meant to be intentional, my guess it was overlooked when we added the JS aggregation. My preference (and the way it now works in D8) is that if there any attributes then aggregation is disabled for that file.
Its rare that anyone uses defer anyway, but in those situations, their code has to already be working with the defer attribute when JS aggregation is turned off. Unless they explicitly only have their code working when the JS aggregation is turned on (thus removing the defer attribute to make it work), this isn't going to affect anything negatively.
So I think the current patch is fine. It only changes the behavior of the JS aggregator for consistency. It may end up removing files from aggregation (thus causing more HTTP requests), but it shouldn't be able to break script functionality because those scripts should already be working fine when JS aggregation isn't enabled.
Comment #46
mikeytown2 CreditAttribution: mikeytown2 commentedFYI: AdvAgg 7.x-2.x supports defer, async, onload when aggregation is enabled & has an option to defer all scripts in the advagg mod sub module.
Comment #47
scottrouse CreditAttribution: scottrouse commentedJust wanted to throw in a note that I've also tested the patch in #44 with drupal-7.x-dev, and it worked well.
Comment #48
David_Rothstein CreditAttribution: David_Rothstein commentedSo #1140356: Add async, onload property to script tags was marked RTBC, and it's basically trying to enable some of the exact same things as this patch, but in a different way.
As I commented there, we can only really commit one of them, so I think the people working on each issue should combine efforts and figure out which approach is best...
(For what it's worth, the patch in that issue does not make any effort to force the file to not be aggregated when one of these attributes is used.)
Comment #49
nod_I'm for the attribute array: It's what we're doing in D8 and it'll help people who wants to add data attributes to script tags (you'd need that for require JS, maybe templating).
Comment #50
dropfen CreditAttribution: dropfen commentedThanks for the patch! Works very well.
I reviewed the code and since there are no possible security Issues applied the patch to a project where I needed the contentflow library with AddOns. The AddOns needs to be declared in the 'load' Attribute of the script tag.
So, with the patch #44 I was able to do things the Drupal Way instead of hacking it in another place, thx
Comment #51
robdubparker CreditAttribution: robdubparker commentedAny chance of this being rolled into 7.x core?
Comment #52
dropfen CreditAttribution: dropfen commentedput #44 into core ;)
Comment #53
japerry44: js_attributes_1664602-44.patch queued for re-testing.
Comment #54
japerryRerolled #44 against 7.23.
Comment #56
japerryDepending on which gets in first, this patch will need to be rerolled against #865536: drupal_add_js() is missing the 'browsers' option As these two patches conflict with each other.
Comment #57
jvsteiner CreditAttribution: jvsteiner commentedUsing this in conjunction with the skimlinks module, and the -54 works great however, only when advagg is disabled: not ideal. I saw some talk upthread that seemed to show that it should work, but it hasn't for me. On my site, i am loading an external script, and need to add the "async" attribute. I am suing 7.26 also, which I believe this patch has not been rolled against, so maybe that is the problem - there was one rejection when I tried to patch, however the .rej file only had some comment lines in it. Any ideas?
Comment #58
mikeytown2 CreditAttribution: mikeytown2 commentedadvagg supports async and defer. If it doesn't open an issue in the advagg issue queue :)
Comment #59
jyee CreditAttribution: jyee commentedreroll of #44 for Drupal 7.28
Comment #60
mikeytown2 CreditAttribution: mikeytown2 commentedI just committed a patch so that advagg works with the attributes array now #2267519: Support `'attributes' => array('defer' => 'defer')` style . Noted that aggregation works with this as well; 1 file aggregate, but if 2 defer's are next to each other then you'll get a 2 file aggregate.
Comment #61
mgiffordThis wasn't run by the bot.
Comment #65
samhassell CreditAttribution: samhassell commentedJust ran across another instance of this. xdomain (https://github.com/jpillora/xdomain) is a useful development library for getting around annoying CORs restrictions quickly. It
requirescan optionally use the following tag/attribute:<script src="//cdn.rawgit.com/jpillora/xdomain/0.6.15/dist/0.6/xdomain.min.js" slave="http://xyz.example.com/proxy.html"></script>
Comment #66
mikeytown2 CreditAttribution: mikeytown2 commented@samhassell
Using inline code seems like a more standard way to accomplish that.
https://github.com/jpillora/xdomain#xdomainslavesslaves
Comment #67
samhassell CreditAttribution: samhassell commented@mikeytown2, yeah I'm using it the alternate way.
Comment #70
robcolburn CreditAttribution: robcolburn commentedRe-rolling @jyee's #59 aka @David_Rothstein's #44.
Adding `type=>external` to tests in hopes it will make Testbot pass.
Comment #73
robcolburn CreditAttribution: robcolburn commentedRe-rolling @jyee's #59 aka @David_Rothstein's #44.
In hopes it will make Testbot pass.
Comment #74
robcolburn CreditAttribution: robcolburn commentedComment #75
robcolburn CreditAttribution: robcolburn commentedI agree with @mikeytown2, it should be possible to preserve aggregation (though it should be segregated), and "async" should be first-class property rather than requiring the "attributes" prefix. New patch should address those concerns.
Comment #77
robcolburn CreditAttribution: robcolburn commentedOne more time TestBot
Comment #79
robcolburn CreditAttribution: robcolburn commentedComment #81
robcolburn CreditAttribution: robcolburn commentedComment #83
robcolburn CreditAttribution: robcolburn commentedComment #85
robcolburn CreditAttribution: robcolburn commentedComment #87
robcolburn CreditAttribution: robcolburn commentedAlright, including some debug code, because TestBot.
Comment #89
robcolburn CreditAttribution: robcolburn commentedShould finally pass testing.
Comment #90
robcolburn CreditAttribution: robcolburn commentedComment #91
robcolburn CreditAttribution: robcolburn commentedComment #93
robcolburn CreditAttribution: robcolburn commentedComment #94
Elijah LynnComment #95
JordanMagnuson CreditAttribution: JordanMagnuson commentedIs there any way, without this patch, to add custom attributes to a given script tag?
For example, in order for Drupal to play nicely with CloudFlare's asynchronous javascript loader (Rocket Loader), some scripts need to have their script tags updated with cf-async="false", like so:
<script type="text/javascript" cf-async="false">
This definitely needs to be done for the Drupal settings script, as well as certain other scripts, but adding custom attributes doesn't seem to be possible via hook_js_alter(). So my question is, is there any way at all to do what I want without hacking core? Or is this patch necessary for that kind of script tag alteration?
Comment #96
hass CreditAttribution: hass commentedCan we commit this soon, please? I need
async
support for reCAPTCHA module.Comment #97
mikeytown2 CreditAttribution: mikeytown2 commented@hass
You can put the asynchronous tag in place and if they're patched or running with AdvAgg then it would work. Also if you've tested the latest patch and things work, mark this RTBC :)
I don't think anything bad happens if the async attribute is set on vanilla core.
Comment #98
hass CreditAttribution: hass commentedI decided later that I'm going with drupal_add_html_head for now as I need to make sure it really use async. That suxxx, but it works. May test the patch later.
Comment #99
rbayliss CreditAttribution: rbayliss at Last Call Media commentedRerolled after changes in 7.38.
Comment #100
Valentine94#99 looks great! +1
Comment #101
ohthehugemanatee CreditAttribution: ohthehugemanatee at Forum One commentedPatch still applies cleanly on 7.41 . Some version of this has been in widespread use in production on Drupal Commons for a long time. Marking this RTBC in 7.x .
Comment #102
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis does not look right to me, and when I tested it, it appeared to affect the order that JavaScript gets added to the page. I think it's because other functions, like drupal_sort_css_js() and drupal_group_css(), are unaware of the changes to these keys.
Also this seems to make the $options['defer'] input to drupal_add_js() have a different effect than $options['attributes']['defer']? That would be confusing.
Overall, while it would be nice to have the 'defer' option preserve aggregation, it doesn't feel like a requirement to get this issue done and may wind up complicating the patch considerably, so perhaps it is worth reconsidering the earlier patches that didn't do that?
Ideally these would use xpath rather than regular expressions to search for the desired HTML pattern.
Also (minor) the code comments should be full sentences.
Comment #103
lokapujyaComment #104
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedGiven the above, here's an attempt to go back to the approach from #73. This is a reroll of that, with the tests changed to use xpath based on my review above.
The interdiff is from #73.
I'm also changing the issue title here, since the title was for the Drupal 8 version of the issue and isn't applicable to Drupal 7.
Comment #106
jonasdk CreditAttribution: jonasdk as a volunteer commentedDavid_Rothstein wrote:
When I started following this issue I thought it was for Drupal 8 - since it is also and issue there. Anybody know if there is a ticket for the Drupal 8 issue? To get Google Page speeds in the green area we need to be able to fix this issue in a core like manner - and not have to build a module ourself just for getting javascripts loaded async.
Comment #107
mikeytown2 CreditAttribution: mikeytown2 commented@jonasdk
Check out the advagg module https://www.drupal.org/project/advagg
Comment #108
wesruv CreditAttribution: wesruv at Lullabot for Cisco Systems commentedThis doesn't seem to be working in D7, is there an issue/patch/something?
Comment #109
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented@jonasdk: This issue was fixed for Drupal 8 already above (see #35).
@wesruv: The patch to review for Drupal 7 is in comment #104 above.
Comment #110
wesruv CreditAttribution: wesruv at Lullabot for Cisco Systems commented@David_Rothstein thanks for the link, I skimmed right past it.
I tried this on the current site I'm working on and chunks of aggregated JS were being included, but the files were not being created, so they're causing 404's. Tried figuring out what was happening, but wasn't able to get to the bottom of it.
Admittedly, this project repo is possessed, but the problem went away as soon as I reverted the patch's update.
Comment #111
sluceroThe patch from #104 has worked great for my testing and use. The only issues I spotted with it were a couple of notices in the logs about the `attributes` key being undefined. I've attached a new patch to include checking for this.
Comment #113
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThat sounds like something that could be caused by a file/directory permissions issue. Are you sure there's no way to experience that on the site in question without this patch applied? (For example, after clearing caches on the site, or enabling a module that adds some JavaScript of its own - i.e. something that will cause the site's aggregated JS to change.)
That looks OK but you only added the check in a couple places, not others (e.g. further down in the 'file' case in drupal_get_js()). If we need to do that it should be done consistently.
However, I'm also curious why it's necessary - what actually triggered the notice? The patch already goes out of its way to ensure the 'attributes' key is always set (e.g. by adding it to drupal_js_defaults()) so it seems like something may not be working as expected and we should get to the bottom of that. The only thing I can think of offhand is that JavaScript added by a module in hook_js_alter() may not have the 'attributes' key set... Is that what's causing it on your site? Either way, that does seem worth protecting against.
Comment #114
wesruv CreditAttribution: wesruv at Lullabot for Cisco Systems commented@David_Rothstein
Tried directory perms, definitely not that.
But I found major issues with my local today, so I'm going to try to give the patch another try in a test environment.
Comment #115
slucero@David_Rothstein:
I had quickly added the checking to address the specific errors I was seeing logged, but you make a good point. I'll try to run back through it today and post an updated patch with the additional checking.
I expect this is the case. The site I'm working uses a great deal of JavaScript added from a variety of places. I agree that for this type of case and backward-compatibility with existing sites in a similar scenario this should be checked for.
Comment #116
sluceroAttached is a new patch that adds some more checking for keys to exists throughout. This should help to avoid some of the notices originally reported in #111.
Comment #117
stefan.r CreditAttribution: stefan.r commentedComment #118
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedWhile I am not opposed per se, I am also not totally convinced:
As far as I remember defer and async work fine when the JS is added as a library. (Correct me if I am wrong).
As libraries are generally favorable for many reasons, I am not sure we need to fix this for the drupal_add_js() directly.
If however libraries do not support this, then I am +1.
Comment #119
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI now understood the patch and approach and I like it.
However:
The preprocess (and therefore aggregation) is not disabled for the new custom attributes, but only for defer.
Therefore the tests need work.
We need to test a lot more cases of combinations. (e.g. only defer, only async, async + custom, only custom, defer + custom).
I am leaving it as a 7.50 target, though I am not sure it can still make it as the deadline for 7.50 is today.
I also would suggest to merge defer and async into the attributes at the beginning of the function, then purely check attributes to avoid this.
Comment #120
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedComment #123
aerozeppelin CreditAttribution: aerozeppelin at California State University San Bernardino commentedTests for #116.
Comment #128
hass CreditAttribution: hass commentedWhat is holding this back? Currently we cannot add
integrity
key to JS and CSS what is a serious security issue if it comes to external files.Comment #129
katannshaw CreditAttribution: katannshaw at Promet Source commentedIs there any update on this? I want to add integrity and crossorigin attributes to my external JS but cannot do so. As @hass, what's holding this up? I'd appreciate some feedback.
Kat
Comment #130
PolHello,
I worked on a new patch. This is a completely different approach and patch from what was proposed here before.
It's based on how the
styles
are rendered. It doesn't use anymore the functiontheme()
.It allows you to pass extra attributes:
Let me know what you think, I'm waiting for feedback.
Comment #131
PolOops, forgot the patch.
Comment #133
PolPatch updated.
Comment #134
PolThis patch modifies the way Javascript
<script>
tags are generated.It defines a new element type "
scripts
", and this new element has a new#pre_render
callbackdrupal_pre_render_scripts
, just like the element "styles
" anddrupal_pre_render_styles
for CSS files.By doing this, we unify the way those Javascript tags are rendered.
drupal_get_js()
anddrupal_get_css()
have the same code structure, they are both returning the result of adrupal_render()
call.The other advantage is that now that
scripts
is an element type, it can be modified by third-party code easily throughhook_element_info_alter()
.The other very good thing is that there is no call to the
theme('html_tag', ...)
function anymore.Comment #135
PolUpdating patch documentation and small bug when JS is aggregated.
Comment #136
PolUpdating the documentation in the patch and adding a new test.
Comment #137
PolComment #139
PolUpdating the patch.
To pass attributes to a Javascript item:
Comment #141
PolAdd more tests.
Comment #143
PolAdding more tests.
Comment #144
PolUpdating the patch, this is the last and final version.
Comment #145
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI'd rather get #116 in, because while the re-factoring is nice it does not really belong in this issue. We could carefully think about doing that in another issue though and likely it would need to be changed in D8 first, too - if that is not yet the case.
#116 itself has the problem that the documentation says that preprocess will be disabled for attributes, but that is not the case.
The D7 behavior for 'defer' is and should be for attributes as well:
- If you want to specify attributes like 'defer' you need to yourself disable 'preprocess'. Else it will just be ignored.
Comment #146
PolHi Fabian,
Thanks for your feedback and fair enough, I will open a new issue.
I checked and It's already done in Drupal 8, see: https://github.com/drupal/drupal/blob/ba42166da282f6c727d15869d0b3702dd3...
Comment #147
PolThe new issue is available at #2990123: Ensure CSS and JS are rendered using the same workflow and it fixes this issue as well.
Comment #148
joseph.olstadBumping to 7.61. This didn't make it into 7.60.
Comment #149
joseph.olstadComment #150
joseph.olstadComment #151
markhalliwellThis is kind of a big deal as we need to be able to, natively, add SRI's for external resources added dynamically via core's existing APIs #2807477: [PP-1] Add Subresource Integrity (SRI) validation.
Comment #152
markhalliwellI just re-reviewed the patch and realized this is only targeting JS. This needs to work with CSS as well (for SRI).
Comment #153
markhalliwellComment #154
PolComment #155
PolHi all,
I reviewed the original patch here, and to be honest, I don't like so much the limitations that it adds, I think we can be more flexible than that while staying completely BC.
The patch I did in #2990123: Ensure CSS and JS are rendered using the same workflow would fix this issue and other stuff, so in order to make everyone happy and in synchronization with Fabian, I propose is to split the issue.
scripts
as a element with a#pre_render
callback, just likestyles
.#attributes
handling inscripts
andstyles
eventually. (this will then fix this issue)theme()
calls.Comment #156
markhalliwell#3051370: Create "scripts" element to align rendering workflow to how "styles" are handled has been committed.
This issue can now focus on just adding/allowing attributes to be passed to both JS and CSS items.
Comment #157
PolHere's my first try at it.
I'm also hiding previous patches as they are no more applicable since #3051370: Create "scripts" element to align rendering workflow to how "styles" are handled went in last week.
Comment #159
PolFix the AJAX tests.
Comment #160
PolUpload the good patch file.
Comment #162
Valentine94+1 for #160 patch. Thanks
Comment #163
markhalliwellI know this was added to mimic
drupal_js_defaults
, but can we really justify adding another function at this point? It seems to have just been added for the benefit of a test.What does this really gain us? It isn't like these functions are hookable/alterable. How much does calling another function cost performance?
I don't think this is necessary anymore. Shouldn't the item's attributes already have this when
drupal_js_defaults()
is non-destructively assigned:Comment #164
PolYes it has been added to mimic how it's done for JS and I do agree that adding that function might reduce performance, but we are talking about micro-micro-optimization here.
I personally liked it because it's mimicking the way JS is built and easier to see what are the default options of a CSS when someone wants to know.
I will probably update the patch in the following days as soon as I get more feedback.
Thanks!
Comment #165
scott.whittaker CreditAttribution: scott.whittaker as a volunteer commentedI've already applied the patch at https://www.drupal.org/project/drupal/issues/3051370 but this patch #160 borks my site (Drupal 7.67):
Fatal error: Uncaught Error: Unsupported operand types in /var/www/html/web/includes/common.inc:3654 Stack trace: #0 /var/www/html/web/includes/common.inc(6128): drupal_pre_render_styles(Array) #1 /var/www/html/web/includes/common.inc(3236): drupal_render(Array) #2 /var/www/html/web/includes/theme.inc(2918): drupal_get_css() #3 /var/www/html/web/includes/theme.inc(1125): template_process_maintenance_page(Array, 'maintenance_pag...') #4 /var/www/html/web/includes/errors.inc(254): theme('maintenance_pag...', Array) #5 /var/www/html/web/includes/bootstrap.inc(2622): _drupal_log_error(Array, true) #6 [internal function]: _drupal_exception_handler(Object(Error)) #7 {main} thrown in /var/www/html/web/includes/common.inc on line 3654
Comment #166
ron_s CreditAttribution: ron_s commented@scott.whittaker, I'm not seeing any error such as that, and it also looks like it is unrelated to this patch. Your error message references a template error:
template_process_maintenance_page(Array, 'maintenance_pag...')
Comment #167
ChristianAdamski CreditAttribution: ChristianAdamski commentedI think this needs a re-roll. He're a test-version.Does not apply to 7.67, but looks fine for -dev. So ignore this.
Comment #168
MustangGB CreditAttribution: MustangGB commentedComment #169
ron_s CreditAttribution: ron_s commentedOne thing to consider is this patch does something very similar to what the AdvAgg module does when enabled. Should make AdvAgg aware of what's happening here since it will probably impact a lot of installations.
Comment #170
MustangGB CreditAttribution: MustangGB commentedComment #171
thursday_bw CreditAttribution: thursday_bw as a volunteer commentedThe comment about making AdvAgg aware has been made before. Are they aware?
mikeytown2 made a comment about this at #42 https://www.drupal.org/project/drupal/issues/1664602#comment-7323174
He's the maintainer of AdvAgg, so they're aware. We can move on on that point.
Comment #172
Nena15 CreditAttribution: Nena15 commentedHello, I'm new to working with core stuff from Drupal. I was wondering where should the patches go, at least in this case, which file, which directory? I'm sorry if this question is too basic? I just don't know where to place the code, I tried at template.php in my custom theme and the site crashed, so it might not be there... Thanks
Comment #173
Collins405 CreditAttribution: Collins405 commentedComment #174
merauluka CreditAttribution: merauluka at Forum One commentedI have rerolled this against 7.72.
Comment #175
kevster CreditAttribution: kevster commentedApplying #174 to 7.73 gives me this error:
Fatal error: Uncaught Error: Unsupported operand types in /xxx/public_html/includes/common.inc:3570 Stack trace: #0 /xxx/public_html/includes/common.inc(6114): drupal_pre_render_styles(Array) #1 /xxx/public_html/includes/common.inc(3214): drupal_render(Array) #2 /xxx/public_html/includes/theme.inc(2955): drupal_get_css() #3 /xxx/public_html/includes/theme.inc(1125): template_process_maintenance_page(Array, 'maintenance_pag...') #4 /xxx/public_html/includes/errors.inc(254): theme('maintenance_pag...', Array) #5 /xxx/public_html/includes/bootstrap.inc(2621): _drupal_log_error(Array, true) #6 [internal function]: _drupal_exception_handler(Object(Error)) #7 {main} thrown in /data02/c2187243/public_html/includes/common.inc on line 3570
I noticed also that hte patch has the wrong root path - web/includes/ instead of includes/
Comment #176
cboyden CreditAttribution: cboyden at UC Berkeley Web Platform Services commentedUpdated reroll against latest D7 dev with correct root path.
Comment #177
marcelovaniThank you for that. Looks like automated tests all pass.
Comment #178
wylbur CreditAttribution: wylbur as a volunteer commentedAdding issue tag for next Drupal 7 release.
Comment #179
Rishi KulshreshthaNeat. The patch provided at #176 works like charm. I've tried adding the
id
attribute to the JS:The output was as expected:
<script type="text/javascript" async="async" id="googleapis-maps" src="https://maps.googleapis.com/maps/api/js?key=YOUR_API_KEY&callback=initMap"></script>
Comment #180
sjerdoThis needs work. Patch #176 doesn't seem to support preprocessing of JS files.
Using the following code, the custom data attribute is only added tot jQuery when preprocess_js is off.
Patch #116 does support JS preprocessing and works like a charm..
Comment #181
klonosNot saying that I'm in favor of the patch in #176, but in line 3651
$element['#attributes'] = $group['attributes'] + $group['attributes'];
should actually be$element['#attributes'] = $group['attributes'] + $element['#attributes'];
instead. Right?Comment #182
DrupalGideonI'm very much in favour of this being added. However, I had this problem on a site and I was able to add a script tag with attributes the following way -