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.

Files: 
CommentFileSizeAuthor
#116 interdiff.txt2.08 KBslucero
#116 js_attributes-1664602-116.patch6.41 KBslucero
patch_commit_fe01acd1a960.patch2.08 KBtherealwebguy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch patch_commit_fe01acd1a960.patch. Unable to apply patch. See the log in the details link for more information. View
#1 1664602-1.patch1.38 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1664602-1.patch. Unable to apply patch. See the log in the details link for more information. View
#4 drupal8.script-tag-attributes.4.patch1.01 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,293 pass(es). View
1664602-1-d8.patch1.55 KBwebchick
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1664602-1-d8.patch. Unable to apply patch. See the log in the details link for more information. View
#20 js_attributes-1664602.patch3.13 KBquicksketch
FAILED: [[SimpleTest]]: [MySQL] 46,312 pass(es), 4 fail(s), and 0 exception(s). View
#21 js_attributes-1664602.patch5.21 KBquicksketch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch js_attributes-1664602_0.patch. Unable to apply patch. See the log in the details link for more information. View
#22 js_attributes-1664602.patch6.82 KBquicksketch
PASSED: [[SimpleTest]]: [MySQL] 46,316 pass(es). View
#73 js_attributes-1664602-71.patch6.25 KBrobcolburn
PASSED: [[SimpleTest]]: [MySQL] 41,141 pass(es). View
#93 js_attributes-1664602-93.patch8.47 KBrobcolburn
PASSED: [[SimpleTest]]: [MySQL] 41,139 pass(es). View
#99 js_attributes-1664602-99.patch8.5 KBrbayliss
PASSED: [[SimpleTest]]: [MySQL] 41,527 pass(es). View
#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

Comments

Wim Leers’s picture

Version: 7.14 » 8.x-dev
Status: Active » Needs review
FileSize
1.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1664602-1.patch. Unable to apply patch. See the log in the details link for more information. View

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
PASSED: [[SimpleTest]]: [MySQL] 37,293 pass(es). View

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
FAILED: [[SimpleTest]]: [MySQL] 46,312 pass(es), 4 fail(s), and 0 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch js_attributes-1664602_0.patch. Unable to apply patch. See the log in the details link for more information. View

Updating tests.

quicksketch’s picture

FileSize
6.82 KB
PASSED: [[SimpleTest]]: [MySQL] 46,316 pass(es). View

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
PASSED: [[SimpleTest]]: [MySQL] 39,656 pass(es). View

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
FAILED: [[SimpleTest]]: [MySQL] 40,262 pass(es), 0 fail(s), and 2 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 40,447 pass(es), 0 fail(s), and 2 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 41,106 pass(es), 0 fail(s), and 2 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 41,144 pass(es), 2 fail(s), and 0 exception(s). View

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
PASSED: [[SimpleTest]]: [MySQL] 41,141 pass(es). View

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

FileSize
7.75 KB
FAILED: [[SimpleTest]]: [MySQL] 40,410 pass(es), 70 fail(s), and 35,650 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in includes/common.inc. View

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
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in modules/simpletest/tests/common.test. View

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
FAILED: [[SimpleTest]]: [MySQL] 41,138 pass(es), 2 fail(s), and 1 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 41,139 pass(es), 1 fail(s), and 1 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 41,139 pass(es), 1 fail(s), and 0 exception(s). View

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
FAILED: [[SimpleTest]]: [MySQL] 41,137 pass(es), 1 fail(s), and 0 exception(s). View

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
PASSED: [[SimpleTest]]: [MySQL] 41,131 pass(es). View

Should finally pass testing.

robcolburn’s picture

Status: Needs work » Needs review
robcolburn’s picture

FileSize
8.46 KB
FAILED: [[SimpleTest]]: [MySQL] 41,133 pass(es), 1 fail(s), and 1 exception(s). View

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
PASSED: [[SimpleTest]]: [MySQL] 41,139 pass(es). View
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

FileSize
8.5 KB
PASSED: [[SimpleTest]]: [MySQL] 41,527 pass(es). View

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

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