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.

CommentFileSizeAuthor
#176 attached-attributes-1664602-176.patch11.79 KBcboyden
#174 attached-attributes-1664602-174.patch11.82 KBmerauluka
#167 1664602-167-attached-attributes.patch10.52 KBChristianAdamski
#160 1664602.patch10.57 KBPol
#159 1664602.patch1.07 KBPol
#157 1664602.patch9.5 KBPol
#144 1664602.patch11.6 KBPol
#143 1664602.patch11.06 KBPol
#141 1664602.patch10.59 KBPol
#139 1664602.patch10.59 KBPol
#136 1664602.patch10.59 KBPol
#135 1664602.patch9.61 KBPol
#133 1664602.patch8.76 KBPol
#131 1664602.patch8.93 KBPol
#123 1664602-123.patch13.58 KBaerozeppelin
#123 interdiff-1664602--116-123.txt9.4 KBaerozeppelin
#116 interdiff.txt2.08 KBslucero
#116 js_attributes-1664602-116.patch6.41 KBslucero
patch_commit_fe01acd1a960.patch2.08 KBtherealwebguy
#1 1664602-1.patch1.38 KBWim Leers
#4 drupal8.script-tag-attributes.4.patch1.01 KBsun
1664602-1-d8.patch1.55 KBwebchick
#20 js_attributes-1664602.patch3.13 KBquicksketch
#21 js_attributes-1664602.patch5.21 KBquicksketch
#22 js_attributes-1664602.patch6.82 KBquicksketch
#40 js_attributes_1664602.patch5.71 KBcatch
#44 interdiff-1664602-40-44.txt2.96 KBDavid_Rothstein
#44 js_attributes_1664602-44.patch5.84 KBDavid_Rothstein
#54 js_attributes_1664602-54.patch5.97 KBjaperry
#59 js_attributes-1664602-59.patch5.84 KBjyee
#70 js_attributes-1664602-70.patch5.88 KBrobcolburn
#73 js_attributes-1664602-71.patch6.25 KBrobcolburn
#75 js_attributes-1664602-75.patch7.75 KBrobcolburn
#77 js_attributes-1664602-77.patch7.66 KBrobcolburn
#79 js_attributes-1664602-79.patch7.68 KBrobcolburn
#81 js_attributes-1664602-81.patch8.23 KBrobcolburn
#83 js_attributes-1664602-83.patch8.3 KBrobcolburn
#85 js_attributes-1664602-85.patch8.32 KBrobcolburn
#87 js_attributes-1664602-87.patch8.36 KBrobcolburn
#89 js_attributes-1664602-89.patch0 bytesrobcolburn
#91 js_attributes-1664602-91.patch8.46 KBrobcolburn
#93 js_attributes-1664602-93.patch8.47 KBrobcolburn
#99 js_attributes-1664602-99.patch8.5 KBrbayliss
#104 js_attributes-1664602-104-TESTS-ONLY.patch2.63 KBDavid_Rothstein
#104 js_attributes-1664602-104.patch6.09 KBDavid_Rothstein
#104 interdiff.txt3.94 KBDavid_Rothstein
#111 js_attributes-1664602-111.patch6.09 KBslucero
#111 interdiff.txt1000 bytesslucero
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Version: 7.14 » 8.x-dev
Status: Active » Needs review
FileSize
1.38 KB

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

Wim Leers’s picture

Title: drupal_add_js #attributes » drupal_add_js() #attributes

Status: Needs review » Needs work

The last submitted patch, 1664602-1.patch, failed testing.

sun’s picture

Title: drupal_add_js() #attributes » Allow to specify SCRIPT HTML element attributes through drupal_add_js()
Component: javascript » base system
Status: Needs work » Needs review
Issue tags: +JavaScript
FileSize
1.01 KB

Proper patch for D8. :)

Wim Leers’s picture

Thanks, @sun! :)

sun’s picture

Issue tags: +Needs backport to D7

1) 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? :-/

Wim Leers’s picture

1) Probably.
2) That was my thinking as well… :/

nod_’s picture

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.

nod_’s picture

Status: Needs review » Needs work
babbage’s picture

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

Wim Leers’s picture

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

effulgentsia’s picture

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

Wim Leers’s picture

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

Wim Leers’s picture

quicksketch’s picture

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

        if (!$item['preprocess'] || !$preprocess_js) {
          if ($item['defer']) {
            $js_element['#attributes']['defer'] = 'defer';
          }
          $query_string_separator = (strpos($item['data'], '?') !== FALSE) ? '&' : '?';
          $js_element['#attributes']['src'] = file_create_url($item['data']) . $query_string_separator . ($item['cache'] ? $query_string : REQUEST_TIME);
          $processed[$index++] = theme('html_tag', array('element' => $js_element));
        }

Adding arbitrary attributes is no different than what we're doing already for the defer attribute.

quicksketch’s picture

My example in #16 is from D7. In D8 the relevant code has been moved to drupal_pre_render_scripts().

        // The defer and async attributes must not be specified if the src
        // attribute is not present.
        if (!empty($element['#attributes']['src'])) {
          // Both may be specified for legacy browser fallback purposes.
          if (!empty($item['async'])) {
            $element['#attributes']['async'] = 'async';
          }
          if (!empty($item['defer'])) {
            $element['#attributes']['defer'] = 'defer';
          }
        }

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.

sun’s picture

@quicksketch: The entirety of that code is located in an else control structure, whereas the corresponding if to that is:

  foreach ($elements['#groups'] as $group) {
    // If a group of files has been aggregated into a single file,
    // $group['data'] contains the URI of the aggregate file. Add a single
    // script element for this file.
    if ($group['type'] == 'file' && isset($group['data'])) {
      $element = $element_defaults;
      $element['#attributes']['src'] = file_create_url($group['data']);
      $element['#browsers'] = $group['browsers'];
      $elements[] = $element;
    }
    // For non-file types, and non-aggregated files, add a script element per
    // item.
    else {
sun’s picture

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

quicksketch’s picture

Title: Allow to specify SCRIPT HTML element attributes through drupal_add_js() » drupal_add_js() defer and async options break when aggregation is enabled (Replace with universal attributes array)
Category: feature » bug
Status: Needs work » Needs review
FileSize
3.13 KB

Heh, 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:

drupal_add_js('myscript.js', array('defer' => TRUE));

After:

drupal_add_js('myscript.js', array('attributes' => array('defer' => 'defer')));

If you add any custom attributes, aggregation is forced to be disabled for that file, as it would if you set $options['cache'] = FALSE.

quicksketch’s picture

FileSize
5.21 KB

Updating tests.

quicksketch’s picture

FileSize
6.82 KB

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

quicksketch’s picture

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

barraponto’s picture

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

sun’s picture

@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) with array('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.

quicksketch’s picture

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

drupal_add_js('example.js', array('attributes' => array('id' => 'entry-template', 'type' => 'text/x-handlebars-template')));
quicksketch’s picture

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.

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

quicksketch’s picture

@sun: P.S. @jenlampton *really* wants to give you a free plane ticket to BADCamp. It's *next weekend*! :)

Wim Leers’s picture

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

RobLoach’s picture

Indirect benefit of this is that it might work with Drupal\Core\Template\Attribute too. Haven't tested though...

use Drupal\Core\Template\Attribute;

$attributes = new Attribute(array(
  'defer',
  'async',
));
drupal_add_js('example.js', array('attributes' => $attributes));
nod_’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

catch’s picture

Status: Reviewed & tested by the community » Needs review

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

quicksketch’s picture

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

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

sun’s picture

Status: Needs review » Reviewed & tested by the community

I agree with #33.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

#33 makes loads of sense. Committed/pushed to 8.x.

Wim Leers’s picture

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

quicksketch’s picture

Status: Patch (to be ported) » Fixed

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

Status: Fixed » Closed (fixed)

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

catch’s picture

Status: Closed (fixed) » Patch (to be ported)

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

catch’s picture

Status: Patch (to be ported) » Needs review
FileSize
5.71 KB

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

effulgentsia’s picture

Status: Needs review » Patch (to be ported)

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

effulgentsia’s picture

Status: Patch (to be ported) » Needs review

x-post.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

I've tried the patch in #40 and it works great. I'm able to add the right script tag for require.js.

<?php
drupal_add_js($theme_path . '/path/to/require.js', array(
      'type' => 'file',
      'scope' => 'footer',
      'weight' => 5,
      'attributes' => array(
        'data-main' => $theme_path . '/path/to/main.js',
      ),
    )
  );
?>

Produces:

<script data-main="/path/to/main.js" src="/path/to/require.js"></script>

It would be really nice to get this into core.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.96 KB
5.84 KB

So this is the part that looks like an API change:

-        if (!$item['preprocess'] || !$preprocess_js) {
+        if (!$item['preprocess'] || !$preprocess_js || $item['defer'] || $item['attributes']) {

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

quicksketch’s picture

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?

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

Does that mean that although it's extra work for the developer, it should be left alone for Drupal 7?

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.

mikeytown2’s picture

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

scottrouse’s picture

Just wanted to throw in a note that I've also tested the patch in #44 with drupal-7.x-dev, and it worked well.

David_Rothstein’s picture

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

nod_’s picture

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

dropfen’s picture

Status: Needs review » Reviewed & tested by the community

Thanks 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

robdubparker’s picture

Any chance of this being rolled into 7.x core?

dropfen’s picture

put #44 into core ;)

japerry’s picture

44: js_attributes_1664602-44.patch queued for re-testing.

japerry’s picture

Issue summary: View changes
FileSize
5.97 KB

Rerolled #44 against 7.23.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 44: js_attributes_1664602-44.patch, failed testing.

japerry’s picture

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

jvsteiner’s picture

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

mikeytown2’s picture

advagg supports async and defer. If it doesn't open an issue in the advagg issue queue :)

drupal_add_js("http://s.skimresources.com/js/{$publisher_id}.skimlinks.js", array(
  'type' => 'external', 
  'scope' => 'footer', 
  'group' => JS_DEFAULT,
  'async' => TRUE, // This should work.
  'defer' => TRUE, // This should work.
));
jyee’s picture

FileSize
5.84 KB

reroll of #44 for Drupal 7.28

mikeytown2’s picture

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

mgifford’s picture

Status: Needs work » Needs review

This wasn't run by the bot.

Status: Needs review » Needs work

The last submitted patch, 59: js_attributes-1664602-59.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 59: js_attributes-1664602-59.patch, failed testing.

samhassell’s picture

Just 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 requires can 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>

mikeytown2’s picture

@samhassell
Using inline code seems like a more standard way to accomplish that.
https://github.com/jpillora/xdomain#xdomainslavesslaves

samhassell’s picture

@mikeytown2, yeah I'm using it the alternate way.

NRCP queued 21: js_attributes-1664602.patch for re-testing.

The last submitted patch, 21: js_attributes-1664602.patch, failed testing.

robcolburn’s picture

Status: Needs work » Needs review
FileSize
5.88 KB

Re-rolling @jyee's #59 aka @David_Rothstein's #44.

Adding `type=>external` to tests in hopes it will make Testbot pass.

Status: Needs review » Needs work

The last submitted patch, 70: js_attributes-1664602-70.patch, failed testing.

The last submitted patch, 70: js_attributes-1664602-70.patch, failed testing.

robcolburn’s picture

FileSize
6.25 KB

Re-rolling @jyee's #59 aka @David_Rothstein's #44.

In hopes it will make Testbot pass.

robcolburn’s picture

Status: Needs work » Needs review
robcolburn’s picture

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

Status: Needs review » Needs work

The last submitted patch, 75: js_attributes-1664602-75.patch, failed testing.

robcolburn’s picture

Status: Needs work » Needs review
FileSize
7.66 KB

One more time TestBot

Status: Needs review » Needs work

The last submitted patch, 77: js_attributes-1664602-77.patch, failed testing.

robcolburn’s picture

Status: Needs work » Needs review
FileSize
7.68 KB

Status: Needs review » Needs work

The last submitted patch, 79: js_attributes-1664602-79.patch, failed testing.

robcolburn’s picture

Status: Needs work » Needs review
FileSize
8.23 KB

Status: Needs review » Needs work

The last submitted patch, 81: js_attributes-1664602-81.patch, failed testing.

robcolburn’s picture

Status: Needs work » Needs review
FileSize
8.3 KB

Status: Needs review » Needs work

The last submitted patch, 83: js_attributes-1664602-83.patch, failed testing.

robcolburn’s picture

Status: Needs work » Needs review
FileSize
8.32 KB

Status: Needs review » Needs work

The last submitted patch, 85: js_attributes-1664602-85.patch, failed testing.

robcolburn’s picture

Status: Needs work » Needs review
FileSize
8.36 KB

Alright, including some debug code, because TestBot.

Status: Needs review » Needs work

The last submitted patch, 87: js_attributes-1664602-87.patch, failed testing.

robcolburn’s picture

FileSize
0 bytes

Should finally pass testing.

robcolburn’s picture

Status: Needs work » Needs review
robcolburn’s picture

FileSize
8.46 KB

Status: Needs review » Needs work

The last submitted patch, 91: js_attributes-1664602-91.patch, failed testing.

robcolburn’s picture

Status: Needs work » Needs review
FileSize
8.47 KB
Elijah Lynn’s picture

Issue summary: View changes
JordanMagnuson’s picture

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

hass’s picture

Can we commit this soon, please? I need async support for reCAPTCHA module.

mikeytown2’s picture

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

hass’s picture

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

rbayliss’s picture

Rerolled after changes in 7.38.

Valentine94’s picture

#99 looks great! +1

ohthehugemanatee’s picture

Status: Needs review » Reviewed & tested by the community

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

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
  1. @@ -4458,17 +4469,19 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
               // leading to better front-end performance of a website as a whole.
               // See drupal_add_js() for details.
               $key = 'aggregate_' . $item['group'] . '_' . $item['every_page'] . '_' . $index;
    +          if (!empty($item['defer'])) {
    +            $key = 'defer_' . $key;
    +          }
    +          if (!empty($item['async'])) {
    +            $key = 'async_' . $key;
    +          }
               $processed[$key] = '';
    

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

  2. I am not sure about adding 'async' as a new option here. This means we're adding two methods to add the same attribute ($options['async'] and $options['attributes']['async']) in the same patch... I would think based on the decision to close #1140356: Add async, onload property to script tags in favor of this issue we would want to go the other way and encourage everyone to just use 'attributes' (i.e. maybe even mark the existing 'defer' option deprecated)... again, more like the earlier patches.
  3. +    // Attributes break out of aggregation for safety / extensibility
    +    $js = 'misc/collapse.js';
    +    drupal_add_js($js, array('attributes' => array('async' => 'async', 'defer' => 'defer')));
    +    $javascript = drupal_get_js();
    +    preg_match('#<script.+src="' . preg_quote(file_create_url($js)) . '[^>]*>#', $javascript, $match);
    

    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.

  4. This issue looks like it will probably need a change notice, so I'm tagging the issue accordingly.
lokapujya’s picture

Issue summary: View changes
David_Rothstein’s picture

Title: drupal_add_js() defer and async options break when aggregation is enabled (Replace with universal attributes array) » Allow an attributes array to be passed in to drupal_add_js() for setting attributes such as "async" and "defer"
Category: Bug report » Task
Status: Needs work » Needs review
FileSize
2.63 KB
6.09 KB
3.94 KB

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

The last submitted patch, 104: js_attributes-1664602-104-TESTS-ONLY.patch, failed testing.

jonasdk’s picture

David_Rothstein wrote:

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.

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.

mikeytown2’s picture

@jonasdk
Check out the advagg module https://www.drupal.org/project/advagg

wesruv’s picture

This doesn't seem to be working in D7, is there an issue/patch/something?

David_Rothstein’s picture

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

wesruv’s picture

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

slucero’s picture

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

The last submitted patch, 111: js_attributes-1664602-111.patch, failed testing.

David_Rothstein’s picture

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.

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

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.

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.

wesruv’s picture

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

slucero’s picture

@David_Rothstein:

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.

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.

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.

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.

slucero’s picture

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

stefan.r’s picture

Issue tags: +Drupal 7.50 target
Fabianx’s picture

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

Fabianx’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

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

Fabianx’s picture

Issue tags: -Drupal 7.50 target +Drupal 7.60 target

  • catch committed 12eefe5 on 8.3.x
    Issue #1664602 by quicksketch, Wim Leers, sun, webchick, nrussell: Fixed...

  • catch committed 12eefe5 on 8.3.x
    Issue #1664602 by quicksketch, Wim Leers, sun, webchick, nrussell: Fixed...

Status: Needs review » Needs work

The last submitted patch, 123: 1664602-123.patch, failed testing.

The last submitted patch, 123: 1664602-123.patch, failed testing.

  • catch committed 12eefe5 on 8.4.x
    Issue #1664602 by quicksketch, Wim Leers, sun, webchick, nrussell: Fixed...

  • catch committed 12eefe5 on 8.4.x
    Issue #1664602 by quicksketch, Wim Leers, sun, webchick, nrussell: Fixed...
hass’s picture

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

katannshaw’s picture

Is 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

Pol’s picture

Status: Needs work » Needs review

Hello,

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

It allows you to pass extra attributes:

/**
 * Implements hook_init().
 */
function custom_init() {
  drupal_add_js(
    'https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.3.3/leaflet.js',
    array(
      '#attributes' => array(
        'id' => 'coin'
      )
    )
  );
}

Let me know what you think, I'm waiting for feedback.

Pol’s picture

Oops, forgot the patch.

Status: Needs review » Needs work

The last submitted patch, 131: 1664602.patch, failed testing. View results

Pol’s picture

Status: Needs work » Needs review
FileSize
8.76 KB

Patch updated.

Pol’s picture

This 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 callback drupal_pre_render_scripts, just like the element "styles" and drupal_pre_render_styles for CSS files.

By doing this, we unify the way those Javascript tags are rendered. drupal_get_js() and drupal_get_css() have the same code structure, they are both returning the result of a drupal_render() call.

The other advantage is that now that scripts is an element type, it can be modified by third-party code easily through hook_element_info_alter().

The other very good thing is that there is no call to the theme('html_tag', ...) function anymore.

Pol’s picture

FileSize
9.61 KB

Updating patch documentation and small bug when JS is aggregated.

Pol’s picture

FileSize
10.59 KB

Updating the documentation in the patch and adding a new test.

Pol’s picture

Issue tags: -Needs tests

Status: Needs review » Needs work

The last submitted patch, 136: 1664602.patch, failed testing. View results

Pol’s picture

Status: Needs work » Needs review
FileSize
10.59 KB

Updating the patch.

To pass attributes to a Javascript item:

/**
 * Implements hook_init().
 */
function custom_init() {
  drupal_add_js(
    'https://cdnjs.cloudflare.com/ajax/libs/leaflet/1.3.3/leaflet.js',
    array(
      'attributes' => array(
        'id' => 'coin'
      )
    )
  );
}

Status: Needs review » Needs work

The last submitted patch, 139: 1664602.patch, failed testing. View results

Pol’s picture

Status: Needs work » Needs review
FileSize
10.59 KB

Add more tests.

Status: Needs review » Needs work

The last submitted patch, 141: 1664602.patch, failed testing. View results

Pol’s picture

Status: Needs work » Needs review
FileSize
11.06 KB

Adding more tests.

Pol’s picture

Issue tags: -Needs backport to D7
FileSize
11.6 KB

Updating the patch, this is the last and final version.

Fabianx’s picture

Status: Needs review » Needs work

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

Pol’s picture

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

$variables['styles'] = $this->cssCollectionRenderer->render($this->assetResolver->getCssAssets($assets, $optimize_css));
$variables['scripts'] = $this->jsCollectionRenderer->render($js_assets_header);
$variables['scripts_bottom'] = $this->jsCollectionRenderer->render($js_assets_footer);
Pol’s picture

The new issue is available at #2990123: Ensure CSS and JS are rendered using the same workflow and it fixes this issue as well.

joseph.olstad’s picture

Issue tags: -Drupal 7.60 target +Drupal 7.61 target

Bumping to 7.61. This didn't make it into 7.60.

joseph.olstad’s picture

joseph.olstad’s picture

markhalliwell’s picture

Title: Allow an attributes array to be passed in to drupal_add_js() for setting attributes such as "async" and "defer" » Allow an attributes array to be passed in to drupal_add_js() for setting attributes such as "async", "defer" and SRI validation
Priority: Normal » Major

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

markhalliwell’s picture

Title: Allow an attributes array to be passed in to drupal_add_js() for setting attributes such as "async", "defer" and SRI validation » Allow attributes to be passed to drupal_add_[css|js] for setting attributes (SRI)

I just re-reviewed the patch and realized this is only targeting JS. This needs to work with CSS as well (for SRI).

markhalliwell’s picture

Title: Allow attributes to be passed to drupal_add_[css|js] for setting attributes (SRI) » Allow attributes to be passed to drupal_add_[css|js] (SRI)
Pol’s picture

Pol’s picture

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

  1. Create a separate issue (#3051370: Create "scripts" element to align rendering workflow to how "styles" are handled) and add scripts as a element with a #pre_render callback, just like styles.
  2. Once the previous issue is done, create a separate issue to enable #attributes handling in scripts and styles eventually. (this will then fix this issue)
  3. Once the previous issue is done, then create a separate issue to remove theme() calls.
markhalliwell’s picture

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

Pol’s picture

Status: Needs review » Needs work

The last submitted patch, 157: 1664602.patch, failed testing. View results

Pol’s picture

Status: Needs work » Needs review
FileSize
1.07 KB

Fix the AJAX tests.

Pol’s picture

FileSize
10.57 KB

Upload the good patch file.

The last submitted patch, 159: 1664602.patch, failed testing. View results

Valentine94’s picture

+1 for #160 patch. Thanks

markhalliwell’s picture

  1. +++ b/includes/common.inc
    @@ -2950,6 +2950,34 @@ function drupal_add_html_head_link($attributes, $header = FALSE) {
    +function drupal_styles_defaults($data = NULL) {
    
    +++ b/modules/simpletest/tests/ajax.test
    @@ -127,17 +127,8 @@ class AJAXFrameworkTestCase extends AJAXTestCase {
    -    // @todo D8: Add a drupal_css_defaults() helper function.
    

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

  2. @@ -4532,17 +4563,18 @@ function drupal_pre_render_scripts(array $elements) {
    +    $js_element['#attributes'] = $item['attributes'] + array(
    +        'type' => 'text/javascript',
    +      );
    

    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:

    +++ b/includes/common.inc
    @@ -4335,7 +4363,7 @@ function drupal_add_js($data = NULL, $options = NULL) {
    +        ) + drupal_js_defaults(),
    
Pol’s picture

Yes 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!

scott.whittaker’s picture

I'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

ron_s’s picture

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

ChristianAdamski’s picture

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

MustangGB’s picture

Issue tags: -Drupal 7.68 target +Drupal 7.69 target
ron_s’s picture

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

MustangGB’s picture

Issue tags: -Drupal 7.69 target +Drupal 7.70 target
thursday_bw’s picture

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

Nena15’s picture

Hello, 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

Collins405’s picture

Issue tags: -Drupal 7.70 target +Drupal 7.73 target
merauluka’s picture

I have rerolled this against 7.72.

kevster’s picture

Applying #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/

marcelovani’s picture

Thank you for that. Looks like automated tests all pass.

wylbur’s picture

Issue tags: -Drupal 7.73 target +Drupal 7.79 target

Adding issue tag for next Drupal 7 release.

Rishi Kulshreshtha’s picture

Status: Needs review » Reviewed & tested by the community

Neat. The patch provided at #176 works like charm. I've tried adding the id attribute to the JS:

$block['content'] = [
  '#markup' => NULL,
  '#attached' => [
    'js' => [
      'https://maps.googleapis.com/maps/api/js?key=YOUR_API_KEY&callback=initMap' => [
        'type' => 'external',
        'async' => TRUE,
        'attributes' => [
          'id' => 'googleapis-maps',
        ],
      ],
    ],
  ],
];

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&amp;callback=initMap"></script>

sjerdo’s picture

Status: Reviewed & tested by the community » Needs work

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

/**
 * Implements hook_library_alter().
 *
 * Set data-attribute for jQuery library.
 */
function MODULE_library_alter(&$libraries, $module) {
  // Immediately return if not modifying the system libraries.
  if ($module !== 'system') {
    return;
  }

  $libraries['jquery']['js']['misc/jquery.js']['attributes'] = [
    'data-CUSTOM' => 'VALUE',
  ];
}

Patch #116 does support JS preprocessing and works like a charm..

klonos’s picture

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

DrupalGideon’s picture

I'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 -

function TEMPLATE_preprocess_html(&$variables) {
 $element = array(
    '#type' => 'html_tag',
    '#tag' => 'script',
    '#value' => '',
    '#weight' => 9999,
    '#attributes' => array(
      'type' => 'text/javascript',
      'src' => 'https://www.javascript.org/myfile.js',
      'data-my-attribute' => 'somevalue',
      'data-another-attribute' => 'anothervalue',
    ),
  );
  drupal_add_html_head($element, 'mykey');
}