Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
With 'Aggregate JavaScript files' option turned on on performance page, JS files are not being aggregated. They are printed as if it was disabled.
I believe this bug was introduced by #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests:
- I went back in git history to the commit before that one (a8cccfc), reinstalled, and js aggregation takes place fine.
- I then checked out the commit where this got in (dd3a597), reinstalled Drupal, and js aggregation no longer takes place.
Comment | File | Size | Author |
---|---|---|---|
#24 | d8-aggregate-js-2413587-22.patch | 4.29 KB | Wim Leers |
#22 | d8-aggregate-js-2413587-22_with_bug_of_2_and_14_FAIL.patch | 4.17 KB | Wim Leers |
Comments
Comment #1
Wim LeersI will investigate this first chance I get, probably 07:00 CET in 2 days. If confirmed, this implies we have no integration test coverage for JS aggregation!
Comment #2
willzyx CreditAttribution: willzyx commentedasset.js.collection_optimizer
service is not called at all inAssetResolver::getJsAssets
.. or am i missing something?Comment #3
willzyx CreditAttribution: willzyx commentedComment #4
Manuel Garcia CreditAttribution: Manuel Garcia commentedJust applied the patch #2, seems to do the trick.
Comment #5
willzyx CreditAttribution: willzyx commentedI did some tests and patch at #2 seems to causes some issues with js settings (eg Block layout > Place blocks no longer works because of this). New patch attached
Comment #7
willzyx CreditAttribution: willzyx commentedRerolled
Comment #8
webchickThis feels like a critical regression; I can't see us shipping with this setting being broken.
Comment #9
Wim LeersConfirmed, also confirmed that #7 fixes it. Slightly improving #7, and adding tests.
Comment #11
Gábor HojtsyThe assert message explains things that are not actually asserted. Either assert those too or give a shorter explanation. I see the test ensures that there are not more things appearing in the footer, which is the point of the test.
Comment #12
Wim LeersComment #13
Gábor HojtsyThe last assert message is still saying things it does not assert. I mean it may pass even if the prior ones fail yet its pass message includes conditions that are tested on the prior one.
Comment #14
Wim LeersPerhaps like this then? :)
Comment #15
Gábor HojtsyLooks good now :)
Comment #16
willzyx CreditAttribution: willzyx commentedpatch at #14 causes the same issues described at #5
Comment #17
Wim Leers#16: confirmed.
Comment #18
Wim LeersAha, I see, you fixed this in #5, that wasn't clear to me because there were no interdiffs.
Applied the same fix as in #5. It's a sensible change, because it doesn't make sense to "aggregate settings"; there is only ever going to be one piece of settings.
This is now identical to #7, with the sole addition of tests.
Comment #19
Wim Leers(Note: I suspected #16 was caused by #784626: Default all JS to the footer, allow asset libraries to force their JS to the header, but it wasn't; the problem was that some AJAX responses would not generate a
Settings
command.)Comment #20
Gábor HojtsyLooks good. Its redoing the fix in #5. I also tested it manually on the blocks admin page.
Comment #21
alexpottGiven that this patch twice broke block admin by optimising settings it feels like we should be adding tests somewhere.
Comment #22
Wim LeersAfter first dismissing that as being not worth the time to write a test for because it'd be too tricky; I figured out a way that's not too tricky. The patch now includes a regression test to protect against the regression introduced in #2 and #14.
Comment #24
Wim LeersGAHHH, wtf is going on with testbot? Again it didn't test a patch :/
Comment #25
opdaviesI've kicked that bot and restarted the tests, and it's returned as passed.
Comment #26
Gábor Hojtsy@alexpott had a great point there. The regression test looks good to me, thanks!
Comment #27
alexpottThanks @Wim Leers - nice test.
This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 65edcac and pushed to 8.0.x. Thanks!