If a module attaches JS files in hook_page_alter() the files are not aggregated.

How to repro?

1. Download latest Google Analytics 7.x
2. Enable JS aggregation
3. JS file sites/all/modules/google_analytics/googleanalytics.js is not aggregated.

Tested with drupal-7.0-alpha7.

CommentFileSizeAuthor
#27 js-hook-page-build-929096-27.patch3.02 KBjhedstrom
#19 js-hook-page-build-929096-19.patch3.04 KBDavid_Rothstein
#6 drupal.js-page-alter.3.patch2.32 KBsun

Comments

jacine’s picture

jacine
she/her
Location New York
commented

subscribe.

jacine’s picture

jacine
she/her
Location New York
commented

Hm, I took a look at Google Analytics and it doesn't seem like proper usage of hook_page_alter(). From what I understand, the point of implementing hook_page_alter() is to actually alter the $page array. In doing so, if you wanted to add CSS or JS files, you'd use the #attached property (which definitely works with aggregation on).

It doesn't appear that Google Analytics is using anything in the $page array, and I could be wrong, but hook_js_alter() seems like a better candidate.

jacine’s picture

jacine
she/her
Location New York
commented
Title: JS files attached via hook_page_alter() are not aggregated » JS & CSS files attached via hook_page_alter() are not aggregated

Oh, and yeah, I can confirm that adding CSS and JS files via drupal_add_css() and drupal_add_js() in a hook_page_alter() implementation works great until you turn aggregation on, which is definitely a WTF at the very least.

hass’s picture

hass commented

It's written in the upgrade docs that hook_footer() has become hook_page_alter(). GA need to attach the code in this late stage... I see no way to change this. So - aggregation need to be fixed on the end of the day or many modules JS/CSS files are not aggregated.

jacine’s picture

jacine
she/her
Location New York
commented

Well, hook_footer() and $closure are now supposed to be in the $page['page_bottom'] region, which is alterable in hook_page_alter(). You can use that, but you'd still need to use #attached in the $page array, as opposed to drupal_add_js() i.e.

   $page['page_bottom']['google_analytics'] = array(
    '#attached' => array(
      'js' => $js,
    ),
  );

It prints right before the closing <body> tag in html.tpl.php, and the aggregation works.

Does that solve your problem?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: JS & CSS files attached via hook_page_alter() are not aggregated » JS files added in hook_page_alter() are not aggregated
Status: Active » Needs review
StatusFileSize
new drupal.js-page-alter.3.patch2.32 KB

Always start with a test.

jacine’s picture

jacine
she/her
Location New York
commented

CSS files are also affected in the same way.

hass’s picture

hass commented

In past D7 days the drupal_add_js() has not worked in page alter... But in #946550: Javascript should be attached to page_bottom, not footer (otherwise it won't ever be included by some themes) i was told by JacobSign and David Rothenstein to go back to drupal_add_js and jacob said WTF to the #attached code snippets I've used since Gabor pointed this out as the way how to attach JS in page alter... So this is not a WTF question only here and if something may work for me or not. I'd do not like to role back again and role forward later again. I'd like to get this fixed or a clear doc how it works. It looks like I'm not the only confused here... So I'd like to *know* and not to guess any longer...

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

well, in order to continue here, we need to figure out why this patch passed (and therefore works) and your code does not.

hass’s picture

hass commented
Title: JS files added in hook_page_alter() are not aggregated » JS files added in hook_page_alter()/hook_page_build() are not aggregated

I can try again with latest dev. Changing title.

hass’s picture

hass commented

I'm currently not able to repro this issue again with beta3. Maybe someone fixed this bug since alpha7. Before closing this case it should be repro'd again with alpha7 - only to verify. Additional it cannot be wrong to get the new test in for future.

David_Rothstein’s picture

David_Rothstein commented

I looked into this and I think there are no bugs here, just API changes:

  1. On July 29, a patch was committed to make drupal_add_js() and drupal_add_css() not aggregate by default (http://drupal.org/node/769226#comment-3267510).
  2. On September 15, alpha7 was released (http://drupal.org/node/913030).
  3. On October 5, a followup patch was committed to make drupal_add_js() and drupal_add_css() go back to aggregating by default (http://drupal.org/node/769226#comment-3534204).
  4. On November 13, beta3 was released (http://drupal.org/node/971088).

So no bug here, just that alpha7 did not aggregate any JS or CSS files except when specifically requested by the caller, and then later that behavior was reverted.

David_Rothstein’s picture

David_Rothstein commented
Title: JS files added in hook_page_alter()/hook_page_build() are not aggregated » Test that JS files added in hook_page_alter()/hook_page_build() are aggregated
Category: bug » task

The patch looks pretty reasonable, though - I guess it doesn't hurt to test :)

I think it is a little fragile to rely on the specific content of filter.admin.js like that, though? Maybe it would be better to just have a specific test JS file that is used for the test.

hass’s picture

hass commented

Let's go with the test only. As david said, this was an alpha7 bug.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

What's the status in 8.x for this?

Sounds like it was not actually a bug hence might not be worth porting. Is that a wrong assumption?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Version: 8.x-dev » 7.x-dev
Status: Needs work » Closed (works as designed)

hook_page_alter() is going away in D8 and hook_page_build() is the recommended way to attach CSS/JS that is "global" to the page. CSS/JS aggregation for CSS/JS added in hook_page_build() definitely works.

Therefor, we must only look at Drupal 7. As per #14 and earlier, this was an alpha7 bug. Which means the only thing left to be done in this issue is adding test coverage for D7, which I think is rather unlikely to happen. So: closing.

David_Rothstein’s picture

David_Rothstein commented
Version: 7.x-dev » 8.x-dev
Status: Closed (works as designed) » Needs work

Why wouldn't the tests be written for Drupal 8 also? Especially since it's now the recommended way to add global CSS/JS to the page, but there's no test for this at all that I can see.

Agreed it's more important to test hook_page_build() than hook_page_alter(), though.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

EditLoadingTest.php + edit.module/edit_page_build() test this. It's likely it's also tested in other places.

I don't see why we need test coverage for this, it's guaranteed to work as long as the theme system/render layer does not mess things up, and *they* should have test coverage to guarantee that. It's rather silly to explicitly test that adding CSS/JS works within this hook; then we should also provide test coverage for any other hook where they may be added.

Or am I missing something here? I'm just getting wary of the boatloads of pointless tests in Drupal 8…

David_Rothstein’s picture

David_Rothstein commented
Status: Needs work » Needs review
StatusFileSize
new js-hook-page-build-929096-19.patch3.04 KB

It's not something I'd spend my time on writing a test from scratch, but given that the code is already written and it takes very little effort to update it, I think it's worth doing.

It's probably less about testing aggregation specifically, than testing that the whole system of using #attached to add JavaScript to the page array actually gets the JavaScript onto the page. I can imagine that breaking, if JavaScript is ever accidentally compiled and rendered in the page's header earlier in the page request than drupal_render() runs on the $page itself.

Here's a reroll for D8 which also adds a specific test JavaScript file (rather than using Filter module's).

Status: Needs review » Needs work

The last submitted patch, js-hook-page-build-929096-19.patch, failed testing.

hass’s picture

hass commented

Why is inline, external, etc. not tested?

David_Rothstein’s picture

David_Rothstein commented
Status: Needs work » Needs review

#19: js-hook-page-build-929096-19.patch queued for re-testing.

David_Rothstein’s picture

David_Rothstein commented

The test failure looks unrelated, so I triggered a retest.

@hass, there are JavaScript tests for inline, external, etc., already (although not "full-stack" tests like this one, testing the use of #attached in hook_page_build()). It could certainly be added, but I figured having one test that starts with the basics is good to begin with.

hass’s picture

hass commented

I need inline in hook_page_alter. If this hook no longer exists I need to know the migration path really works or Google Analytics may be broken. Please make sure this is all fully tested and examples exist.

David_Rothstein’s picture

David_Rothstein commented

hook_page_alter() definitely still exists in Drupal 8.

I'm not sure offhand which issue it is that's proposing its removal, but whichever one it is, that's probably the issue where you should leave the above comment about Google Analytics.

hass’s picture

hass commented

See comment #16...

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Issue summary: View changes
Issue tags: +Needs reroll
StatusFileSize
new js-hook-page-build-929096-27.patch3.02 KB

Reroll of #19.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Issue tags: -Needs reroll

Removing tag.

Status: Needs review » Needs work

The last submitted patch, 27: js-hook-page-build-929096-27.patch, failed testing.

hass’s picture

hass commented

#2362987: Remove hook_page_build() and hook_page_alter()

hass’s picture

hass commented
Parent issue: » #2350949: Add hook_page_attachments(_alter)() and deprecate hook_page_build/alter()
Related issues: +#2362987: Remove hook_page_build() and hook_page_alter()

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Closed (outdated)

I'm not sure this issue is still relevant. Feel free to reopen if that is the case.