Aggregate JavaScript files no longer working

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

asset.js.collection_optimizer service is not called at all in AssetResolver::getJsAssets .. or am i missing something?

I 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

Rerolled

This feels like a critical regression; I can't see us shipping with this setting being broken.

Confirmed, also confirmed that #7 fixes it. Slightly improving #7, and adding tests.

+++ b/core/modules/system/src/Tests/Common/AttachedAssetsTest.php
@@ -169,6 +169,21 @@ function testAggregatedAttributes() {
+    $this->assertEqual(2, count(\Drupal::service('asset.js.collection_renderer')->render($footer_js)), 'There are 2 JavaScript assets in the footer: one with drupalSettings, one with the sole aggregated JavaScript asset.');

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

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

Perhaps like this then? :)

Looks good now :)

patch at #14 causes the same issues described at #5

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

#16: confirmed.

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

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

Looks good. Its redoing the fix in #5. I also tested it manually on the blocks admin page.

Given that this patch twice broke block admin by optimising settings it feels like we should be adding tests somewhere.

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

GAHHH, wtf is going on with testbot? Again it didn't test a patch :/

I've kicked that bot and restarted the tests, and it's returned as passed.

@alexpott had a great point there. The regression test looks good to me, thanks!

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