CSS/JS asset caching can easily be trashed

See below.

Quick untested patch based on the proposed resolution. Might not be too bad to fix directly.

Went for getLibrariesToLoad() - we could have the same result from getAlreadyLoadedLibraries() but have a different set of libraries still to load.

*coughs*

+++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
@@ -114,10 +114,10 @@ protected function getLibrariesToLoad(AttachedAssetsInterface $assets) {
+    $cid = 'css:' . $theme_info->getName() . ':' .  Crypt::hashBase64(serialize($this->getLibrariesToLoad($assets))) . (int) $optimize;

Nit: extra whitespace before Crypt::hashBase64

Fixing that whitespace.

@catch: The latest patch does not fix the whitespace typo.

That's a good point, it doesn't.

Minimizing duplicate calls to getLibrariesToLoad() in the same function.

This looks solid, but I can't see how this can go in without some test coverage.

I have two concerns.

1. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -331,6 +325,9 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
   +      // Attached settings override both library definitions and
   +      // hook_js_settings_build().
   

   This is wrong and a very big behavior change. hook_js_settings_alter() is what should be able to override attached JS settings.

2. I see one other problem:

   public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
     …
   
     if ($cached = $this->cache->get($cid)) {
       …
     }
     else {
       …
       $settings_have_changed = count($libraries_to_load) > 0 || count($assets->getSettings()) > 0;
     }
   
     …
   
     $settings = FALSE;
     if ($settings_required && $settings_have_changed) {
        $settings = …
     }
   
     if ($settings !== FALSE) {
       …
       $settings = NestedArray::mergeDeepArray([$settings, $assets->getSettings()], TRUE);
       …
     }
   
     …
   }
   

   In other words: this patch thinks it only need to call $assets->getSettings() if $settings !== FALSE. But the value of $settings itself depends on $assets->getSettings().

   Therefore it is not correct to simply assume the cached value is correct; because the cached value in fact also did depend on $assets->getSettings().

   Two options come to mind:

   1. Figure out how to make the non-cached-branch not depend on count($assets->getSettings()).
   2. Or just make the CID take count($assets->getSettings()) into account, which is fine because it's just a boolean.

This is wrong and a very big behavior change. hook_js_settings_alter() is what should be able to override attached JS settings.

So the order before this is:

1. libraries
2. attached
3. js_settings_build()
4. js_settings_alter()

The order in the patch is

1. libraries
2. js_settings_build()
3. attached
4. js_settings_alter()

hook_js_settings_build() says it only applies to library definitions, so it's not really clear to me what's correct here.

Option #2 of the second point seems reasonable.

Oops, #14.1 was wrong. I misread it; I thought #attached[drupalSettings] came after hook_js_settings_alter(). That's not the case.
Sorry!

I agree with #15 and #16.1.

CNW for #14.2

We should probably have a small CR for the behavior change.

Or just make the CID take count($assets->getSettings()) into account, which is fine because it's just a boolean.

Done.

Pair-programmed the unit test with @cyberwolf, @wimvds, @ppind, @sandervd and @phoenix at the http://drupaleuven.be meetup! :D

FAIL patch is the interdiff.

+++ b/core/tests/Drupal/Tests/Core/Asset/AssetResolverTest.php
@@ -0,0 +1,150 @@
+    $this->assertCount($expected_cache_item_count, $this->cache->getAllCids());
...
+    $this->assertCount($expected_cache_item_count, $this->cache->getAllCids());

Yeah it would be nice to look at the actual assets which are returned ...

#22: that requires far more mocking. The AssetResolver class indeed needs much more test coverage. But that is out of scope. This is the minimum viable test coverage IMO.

Patch ordering.

I agree with Wim on the test coverage. We've gone from absolutely zero explicit test coverage of AssetResolver to testing the actual bug here. Adding more test coverage would be great, but I'd be tempted to commit this more or less as is so it's in RC4, and then more test coverage could happen later.

Marking RTBC - the manual testing in RC4, and RC4 having a clean slate on critical issues, overrides the additional automated test coverage we can add at any point for me.

+++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
@@ -331,6 +325,9 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
 

Nit here,
remove this extra line?

Ah I thought for some reason that we don't actually test the bug introduced here, sorry for that.

With HEAD everyone gets the correct assets, just they get their very own special cache entry for them, so in terms of that specific bug, the test coverage here is fine. What the test coverage doesn't do is confirm that the fix here doesn't introduce any new bugs unrelated to the issue title, but that's a fault of the lack of test coverage for the class in general.

Exactly. And that lack of test coverage in this area is inherited from Drupal 7.

This patch looks great. But...

+++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
@@ -219,9 +215,9 @@ protected function getJsSettingsAssets(AttachedAssetsInterface $assets) {
-    // hook_js_alter(). Additionally add the current language to support
-    // translation of JavaScript files.
-    $cid = 'js:' . $theme_info->getName() . ':' . $this->languageManager->getCurrentLanguage()->getId() . ':' .  Crypt::hashBase64(serialize($assets)) . (int) $optimize;
+    // hook_library_info_alter().
+    $libraries_to_load = $this->getLibrariesToLoad($assets);
+    $cid = 'js:' . $theme_info->getName() . ':' . Crypt::hashBase64(serialize($libraries_to_load)) . (int) (count($assets->getSettings()) > 0) . (int) $optimize;

Why are we dropping the current language from $cid? Reading #2506369-4: Cache CSS/JS asset resolving.1, it looks to me that this was needed to support locale_js_alter(), and also looks to me like that function in HEAD still does JS file translation, so don't we still need to vary this cache by language?

Also let's upload the complete patch as the last attachment so it doesn't keep failing :)

@effulgentsia that's why. We no longer cache the alters so language isn't needed in the cache ID.

Don't credit me, I am just reuploading for testbot.

We no longer cache the alters

Are you sure? Looks to me like $this->moduleHandler->alter('js', $javascript, $assets); is still happening in the code block that's being cached. What am I missing?

#38 no you're right, we don't cache hook_js_settings_alter() but we do cache hook_js_alter(). And looking at locale tests, I can't see any coverage either.

This restores that, but confirms this shouldn't go into RC4 and we should try to get it in with some extra test coverage before 8.0.0.

Uploading interdiffs as patches like a champ.

Added coverage for the language manager stuff. Interdiff is against #36.

1. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -219,9 +215,9 @@ protected function getJsSettingsAssets(AttachedAssetsInterface $assets) {
   -    // hook_js_alter(). Additionally add the current language to support
   -    // translation of JavaScript files.
   ...
   +    // hook_library_info_alter().
   +    $libraries_to_load = $this->getLibrariesToLoad($assets);
   

   Nit: We probably want to restore this part of the comment then.

2. +++ b/core/tests/Drupal/Tests/Core/Asset/AssetResolverTest.php
   @@ -0,0 +1,167 @@
   +      ->will($this->onConsecutiveCalls($english, $english, $japanese, $japanese));
   ...
   +    $this->assetResolver->getJsAssets($assets_a, FALSE);
   +    $this->assetResolver->getJsAssets($assets_b, FALSE);
   +    $this->assertCount($expected_cache_item_count * 2, $this->cache->getAllCids());
   

   Nice!

Also, I can't believe none of us noticed the language manager disappearing from the CID generation logic until @effulgentsia noticed in #33. Wow. :(

With that now rectified, I think what I said in #23 is now actually true:

The AssetResolver class indeed needs much more test coverage. But that is out of scope. This is the minimum viable test coverage IMO.

Yes that was patch cruft - I'd originally looked at moving all of the alters out, but that would leave too much uncached, then forgot to revert the cid changes when giving up on that.

Restored the comment (not exactly the same wording).

I considered adding test coverage for $optimize = TRUE, since that should be easy to add, but the css optimizer service isn't injected:

 if ($optimize) {
        $collection_optimizer = \Drupal::service('asset.js.collection_optimizer');
        $js_assets_header = $collection_optimizer->optimize($js_assets_header);
        $js_assets_footer = $collection_optimizer->optimize($js_assets_footer);
      }

Given that, I've opened #2614936: Improve unit test coverage for AssetResolver to keep going with the tests, and added a patch there which injects those two services, but I definitely think that's out of scope here.

Adding commit credit.

+++ b/core/lib/Drupal/Core/Asset/AssetResolver.php

@@ -331,6 +326,9 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
 
 
    if ($settings !== FALSE) {

Theres still an extra line here.

We still need a CR for the change outlined in the IS...

If anything relied on using hook_js_settings_build() to change / extend settings that have been attached to the page, then hey need to change to hook_js_settings_alter(), which is the run-time hook that _should_ be used instead.

I don't agree that that's an API change, or that it needs a change notice - the existing behaviour is a bug.

However looking through existing change notices, the hook wasn't even mentioned, so I added a note about it to https://www.drupal.org/node/2383115

Removed the extra blank line.

#49:

However as settings need core/drupalSettings library, which IIRC adds some empty settings, this should never be the case that assets have settings, but no library has settings.

So this should be fine.

This is by design. You must depend on core/drupalSettings to have Drupal JS settings be loaded in the HTML. And indeed, core/drupalSettings has these defaults, which cause it to be not empty, which causes the hook to be called:

drupalSettings:
  version: VERSION
  js:
    misc/drupalSettingsLoader.js: {}
  drupalSettings:
    # These placeholder values will be set by system_js_settings_alter().
    path:
      baseUrl: null
      scriptPath: null
      pathPrefix: null
      currentPath: null
      currentPathIsAdmin: null
      isFront: null
      currentLanguage: null
    pluralDelimiter: null

I'm also working on a manual review of this issue.

The IS says:

CSS and JS assets are cached with the following CIDs:

    // JS
    $cid = 'js:' . $theme_info->getName() . ':' . $this->languageManager->getCurrentLanguage()->getId() . ':' .  Crypt::hashBase64(serialize($assets)) . (int) $optimize;

    // CSS
    $cid = 'css:' . $theme_info->getName() . ':' . Crypt::hashBase64(serialize($assets)) . (int) $optimize;

In our default javascript we have:

$settings['user']['uid'] = $user->uid;

Therefore CSS and JS settings are cached by uid by default.

This is incorrect. $assets->getSettings() does not contain user.uid === THE_USER_ID. It only has the core/drupalSettings library listed in $assets->getLibraries(). It's then system_js_settings_alter() that sets the current path (etc) and user_js_settings_alter() that sets the current user ID (etc). But system_js_settings_alter()'s result is not even cached, let alone it would affect the CID.

But, the IS also says this:

In addition to that, the following code can trash all JS / CSS asset caches easily:

  $build['#attached']['drupalSettings']['currentTime'] = time();

without you expecting that.

That part is true. But this is major at best, not critical.

+++ b/core/tests/Drupal/Tests/Core/Asset/AssetResolverTest.php
@@ -0,0 +1,167 @@
+        (new AttachedAssets())->setAlreadyLoadedLibraries([])->setLibraries(['core/drupal'])->setSettings(['user' => ['uid' => 1]]),
+        (new AttachedAssets())->setAlreadyLoadedLibraries([])->setLibraries(['core/drupal'])->setSettings(['user' => ['uid' => 2]]),

Consequently this is a vast exaggeration; and should instead use the fairly crazy example of currentTime = time().

Updating the test per #62.

Tagging for triage now it's not critical.

I was wondering how we managed to let that slip when working on the original patch, nice that we actually didn't...

I agree with @catch about this being good as an "rc target", meaning to get it in during what's left of the RC phase rather than after 8.0.0. Because in general, we should not commit behavior changes like this one to hook_js_settings_build() in patch releases, but rather should only do that in minors, so if this issue doesn't land before 8.0.0 it would be postponed to 8.1.0. But if in the in between 6 months we discover that many sites are going down because of some contrib or custom module putting something into a JS setting that is trashing the cache due to this core bug, then we'd need to choose between either letting those sites suffer or fixing this and incurring the BC break in a patch release, and both of those would be much worse outcomes than fixing it now.

Pushed to 8.0.x, but NW for change record.

I missed that @catch had already updated a CR in #56. I just now added this issue to that CR.

This may broke have something with the testbots by introducing some sort of race condition. We've seen several patches fail with the following:

fail: [Browser] Line 253 of core/modules/system/src/Tests/Update/UpdatePathTestBase.php:
GET http://localhost/checkout/update.php/start?id=2&op=do_nojs returned 0 (0 bytes)."

The very first time we see that failure is about 30 seconds after this was committed. There have been about 18 tests so far in the last 30 minutes that have failed exactly that way..

Some examples:
https://www.drupal.org/pift-ci-job/82522
https://www.drupal.org/pift-ci-job/82580
https://www.drupal.org/pift-ci-job/82628

https://www.drupal.org/pift-ci-job/82522 looks like it was from before this was committed?

Arg. sorry. 15:11 UTC and 15:11 PST are not the same thing.

causing errors running phpunit locally.

1) Drupal\Tests\Core\Asset\LibraryDiscoveryParserTest::testBuildByExtensionSimple
constant(): Couldn't find constant CSS_THEME

/home/jgilliland/public_html/d8_1/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php:141
/home/jgilliland/public_html/d8_1/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php:103

2) Drupal\Tests\Core\Asset\LibraryDiscoveryParserTest::testBuildByExtensionWithTheme
constant(): Couldn't find constant CSS_THEME

/home/jgilliland/public_html/d8_1/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php:141
/home/jgilliland/public_html/d8_1/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php:130

3) Drupal\Tests\Core\Asset\LibraryDiscoveryParserTest::testVersion
Use of undefined constant JS_LIBRARY - assumed 'JS_LIBRARY'

/home/jgilliland/public_html/d8_1/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php:161
/home/jgilliland/public_html/d8_1/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php:213

4) Drupal\Tests\Core\Asset\LibraryDiscoveryParserTest::testNonStringVersion
Use of undefined constant JS_LIBRARY - assumed 'JS_LIBRARY'

/home/jgilliland/public_html/d8_1/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php:161
/home/jgilliland/public_html/d8_1/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php:248

5) Drupal\Tests\Core\Asset\LibraryDiscoveryParserTest::testExternalLibraries
Use of undefined constant JS_LIBRARY - assumed 'JS_LIBRARY'

/home/jgilliland/public_html/d8_1/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php:161
/home/jgilliland/public_html/d8_1/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php:272

6) Drupal\Tests\Core\Asset\LibraryDiscoveryParserTest::testDefaultCssWeights
constant(): Couldn't find constant CSS_THEME

/home/jgilliland/public_html/d8_1/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php:141
/home/jgilliland/public_html/d8_1/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php:295

7) Drupal\Tests\Core\Asset\LibraryDiscoveryParserTest::testLibraryWithCssJsSetting
Use of undefined constant JS_LIBRARY - assumed 'JS_LIBRARY'

/home/jgilliland/public_html/d8_1/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php:161
/home/jgilliland/public_html/d8_1/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php:353

8) Drupal\Tests\Core\Asset\LibraryDiscoveryParserTest::testLibraryWithDataTypes
constant(): Couldn't find constant CSS_THEME

/home/jgilliland/public_html/d8_1/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php:141
/home/jgilliland/public_html/d8_1/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php:409

9) Drupal\Tests\Core\Asset\LibraryDiscoveryParserTest::testLibraryWithJavaScript
Use of undefined constant JS_LIBRARY - assumed 'JS_LIBRARY'

/home/jgilliland/public_html/d8_1/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php:161
/home/jgilliland/public_html/d8_1/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php:438

10) Drupal\Tests\Core\Asset\LibraryDiscoveryParserTest::testLibraryWithLicenses
Use of undefined constant JS_LIBRARY - assumed 'JS_LIBRARY'

/home/jgilliland/public_html/d8_1/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php:161
/home/jgilliland/public_html/d8_1/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php:481

+++ b/core/tests/Drupal/Tests/Core/Asset/AssetResolverTest.php
@@ -0,0 +1,168 @@
+if (!defined('CSS_AGGREGATE_DEFAULT')) {
+  define('CSS_AGGREGATE_DEFAULT', 0);
+}

This doesn't get along with the Library test.

if (!defined('CSS_AGGREGATE_DEFAULT')) {
  define('CSS_AGGREGATE_DEFAULT', 0);
  define('CSS_AGGREGATE_THEME', 100);
  define('CSS_BASE', -200);
  define('CSS_LAYOUT', -100);
  define('CSS_COMPONENT', 0);
  define('CSS_STATE', 100);
  define('CSS_THEME', 200);
  define('JS_SETTING', -200);
  define('JS_LIBRARY', -100);
  define('JS_DEFAULT', 0);
  define('JS_THEME', 100);
}

This probably runs fine alone and in the simpletest runner which splits processes but it broken in the full phpunit run.

Created #2616846: PHPUnit is broken again from the command line to fix the PHPUnit fails

Handling the PHPUnit fails in a separate issue.