If you use drupal_add_js('window.google_analytics_uacct = ' . drupal_json_encode($id) . ';', array('type' => 'inline', 'group' => JS_LIBRARY, 'weight' => -21)); to order an inline JS code above the jquery lib it is not added above the jquery.js - no it's added below. This is incorrect.

If you use drupal_add_js('window.google_analytics_uacct = ' . drupal_json_encode($id) . ';', array('type' => 'inline', 'weight' => JS_LIBRARY - 21)); it works, but this have also worked in past and is not the way how drupal_add_js() should be used in D7.

For an example see #975504: drupal_add_js() API change that have a patch that can be applied to GA to repro the bug, please.

CommentFileSizeAuthor
#21 remove_every_page_asset_flag-977844-21.patch26.54 KBwim leers

Comments

hass’s picture

hass commented

Here is an example how the HTML code should looks like:

<script type="text/javascript"> 
<!--//--><![CDATA[//><!--
window.google_analytics_uacct = "UA-1234-1";
//--><!]]>
</script>
<script type="text/javascript" src="http://localhost/drupal7/misc/jquery.js?v=1.4.4"></script> 
<script type="text/javascript" src="http://localhost/drupal7/misc/jquery.once.js?v=1.2"></script> 
<script type="text/javascript" src="http://localhost/drupal7/misc/drupal.js?lc6z1k"></script>

And here how it looks like with the bug if 'group' is used in the API:

  <script type="text/javascript" src="http://localhost/drupal7/misc/jquery.js?v=1.4.4"></script> 
<script type="text/javascript" src="http://localhost/drupal7/misc/jquery.once.js?v=1.2"></script> 
<script type="text/javascript" src="http://localhost/drupal7/misc/drupal.js?lc6z1k"></script> 
<script type="text/javascript"> 
<!--//--><![CDATA[//><!--
window.google_analytics_uacct = "UA-1234-1";
//--><!]]>
</script>

You can see the weight is clearly wrong.

effulgentsia’s picture

effulgentsia commented
Status: Active » Closed (works as designed)

See #769226-212: Optimize JS/CSS aggregation for front-end performance and DX. I'll respond on #975504: drupal_add_js() API change as well.

hass’s picture

hass commented
Status: Closed (works as designed) » Active

Optimization have nothing to do with this bug here.

effulgentsia’s picture

effulgentsia commented
Title: drupal_add_js() 'weight' calculation with 'group' is incorrect » drupal_sort_css_js() sorting by group, then every_page, then weight is a WTF
Issue tags: +DrupalWTF

Optimization has everything to do with this bug here :)

drupal_sort_css_js() sorts the js items by 'group', then by 'every_page', then by 'weight'. This allows aggregate files to be more effectively re-used across a site visit. But, it means you can't place a JS item above one whose 'every_page' is TRUE without either setting 'every_page' of your item to TRUE, or else putting your item in an earlier group, as per #975504-4: drupal_add_js() API change.

I admit this is a WTF in some situations. Though I also think array('type' => 'inline', 'group' => JS_LIBRARY-1) makes sense for what you're trying to do, since what you're trying to do is say "place this javascript code above all JS_LIBRARY files".

hass’s picture

hass commented

drupal_sort_css_js() sorts the js items by 'group', then by 'every_page', then by 'weight'. This allows aggregate files to be more effectively re-used across a site visit. But, it means you can't place a JS item above one whose 'every_page' is TRUE without either setting 'every_page' of your item to TRUE, or else putting your item in an earlier group

This sentences are much easier to understand how the logic works than the API docs of drupal_add_js()...

hass’s picture

hass commented
Title: drupal_sort_css_js() sorting by group, then every_page, then weight is a WTF » drupal_sort_css_js() does not sort by group, then every_page, then weight if files added via hook_library()

@effulgentsia: I checked how jquery.js is added to the page and I do not see a 'every_page' => TRUE. Maybe the bug is that a JS file order from a hook_library() cannot over-weighted? As you can see above I'm only using a lower weight than misc/jquery.js what should cause the script tag to move above jquery.js, but it does not.

  // jQuery.
  $libraries['jquery'] = array(
    'title' => 'jQuery',
    'website' => 'http://jquery.com',
    'version' => '1.4.4',
    'js' => array(
      'misc/jquery.js' => array('group' => JS_LIBRARY, 'weight' => -20),
    ),
  );

So I'm NOT trying to place JS above a JS file that have 'every_page' set to TRUE. The title seems now missleading and wrong as this seems to be no documenatation or WTF issue to me.

effulgentsia’s picture

effulgentsia commented

Well, I guess you can add one more WTF to the mix. You can pass $every_page as a parameter to drupal_add_library(). And that is done for 'jquery' within drupal_add_js(). Which is understandable: in some cases, the caller of drupal_add_library() needs to decide if the library is being added to every page or not.

The documentation about all of this needs a lot of love. Some of it is just plain wrong. I'll try to help on that when I have a chance.

hass’s picture

hass commented

Hell... now I found this every page TRUE for jquery. every page is not really correct for Google Analytics - so I need to go with group -1 for now... thank your very much for the many hints!

effulgentsia’s picture

effulgentsia commented
Title: drupal_sort_css_js() does not sort by group, then every_page, then weight if files added via hook_library() » The impact of 'every_page' within drupal_add_(css|js|library)() is confusing
ohnobinki’s picture

ohnobinki commented

+

It would be nice to understand more clearly what effect every_page has on Drupal. I.e., does drupal assume that if it has already cached all `every_page' JS files it doesn't need to confirm that the cache is correct? That is, would I need to call drupal_clear_js_cache() after changing a setting in my module which changes my drupal_add_js() calls in hook_init()? I think that the answer to my question is `no' and that doesn't make all that much sense to me :-p.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 7.x-dev » 8.x-dev
robloach’s picture

robloach
he/him
commented

There is absolutely no guarentee any given CSS/JS file should be presented on every page request. Especially now that jQuery is not required to be displayed on every page, the JavaScript/CSS aggregation sorting system completely breaks. What would happen if we simply removed the flag? With the likely move to an AMD architecture, this seems less relevant.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue summary: View changes
Parent issue: » #1805452: [meta] Asset library declarations, registry, usage, DX, compatibility, documentation
Related issues: +#1996238: Replace hook_library_info() by *.libraries.yml file, +#1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering, +#1805552: Remove custom weights from library definitions, +#1762204: Introduce Assetic compatibility layer for core's internal handling of assets
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: The impact of 'every_page' within drupal_add_(css|js|library)() is confusing » The impact of 'every_page' within drupal_add_(css|js|library)() is confusing, even damaging
Component: base system » asset library system
Priority: Normal » Major
Issue tags: +front-end performance, +DX (Developer Experience)

Indeed, the every_page heuristic is pretty broken… by design. It depends on the site which assets are loaded on every page. Hence declaring an asset is (probably) going to be loaded on every page is at best an educated guess; but it's definitely site-specific.

However, on to the good news: every single asset in Drupal 8 is now loaded through asset libraries. That also means we know about all dependencies. Which means we can build a dependency tree, and thus we can calculate the every_page heuristic automatically!

Therefore I think we should remove the every_page heuristic from Drupal 8, remove it from the docs, and either implement the above dependency tree heuristic before release (if we think it's that important), or add it in 8.1, where it won't be breaking any APIs.

In any case, in the current implementation, it is:

  1. very confusing (and not only in Drupal 8, since this issue was filed against Drupal 7, more than 4 years ago)
  2. not widely used (in Drupal 8, because the inherent problems in its design, see above)
  3. therefore not getting us the front-end performance benefit it's designed to give us
  4. and hence causes more damage than it provides value
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: The impact of 'every_page' within drupal_add_(css|js|library)() is confusing, even damaging » The impact of the 'every_page' option for CSS/JS assets is confusing, even damaging
catch’s picture

catch
he/him
Primary language English
commented

Yeah I've never seen this used correctly, in either direction, so just dropping it seems good. And we won't really be breaking any APIs either dropping it or figuring out the same information via dependencies.

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +beta target

Do we think we can remove this after 8.0.0? This implication would be that setting it is a no-op, which might be fine if it gets handled better anyway.

Adding 'beta target' though in case that's not true.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

This implication would be that setting it is a no-op, which might be fine if it gets handled better anyway.

Yes, see #15 for a proposed plan.

Keeping the tag until you remove it again, i.e. if you're convinced that that is a plausible plan.

catch’s picture

catch
he/him
Primary language English
commented

The plan in #15 looked to be removing it before 8.0.0 then adding it back at a later date. I meant if we don't do anything before 8.0.0, could we still ditch it even then. IMO the answer is 'yes' but good to validate that.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new remove_every_page_asset_flag-977844-21.patch26.54 KB

Oh, hm, good point. Rather than analyzing whether it's possible, I think it's easier to just do it.

Here's a patch to remove every_page in this issue, before 8.0.0.

catch’s picture

catch
he/him
Primary language English
commented

Double checked this removes every reference. RTBC for me once green.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2554051: Use the asset library dependency tree to improve CssCollectionGrouper & JsCollectionGrouper

Heh, okay. Filed a feature request for 8.1 to do what I described in #15: #2554051: Use the asset library dependency tree to improve CssCollectionGrouper & JsCollectionGrouper.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: The impact of the 'every_page' option for CSS/JS assets is confusing, even damaging » Remove the 'every_page' option for CSS/JS assets: it is confusing, even damaging

More accurate title.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Wim said it in #15, now that we have libraries this is obsolete.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

  • catch committed 7a6602c on 8.0.x 
    Issue #977844 by Wim Leers: Remove the 'every_page' option for CSS/JS...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

We can add the optimization back in #2554051: Use the asset library dependency tree to improve CssCollectionGrouper & JsCollectionGrouper properly.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

Nice. I didn't know there was an open issue to get rid of every_page. I found it confusing when I first encountered, knowing the flexibility we have with libraries. I'm glad this simplifies things.

Status: Fixed » Closed (fixed)

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