Issue #2603138 by catch, Wim Leers, tim.plunkett, chx, Fabianx, effulgentsia:...

Issue #2603138 by catch, Wim Leers, tim.plunkett, chx, Fabianx, effulgentsia: CSS/JS asset caching can easily be trashed

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

@@ -116,8 +116,9 @@ protected function getLibrariesToLoad(AttachedAssetsInterface $assets) {
   public function getCssAssets(AttachedAssetsInterface $assets, $optimize) {
     $theme_info = $this->themeManager->getActiveTheme();
     // Add the theme name to the cache key since themes may implement
-    // hook_css_alter().
-    $cid = 'css:' . $theme_info->getName() . ':' . Crypt::hashBase64(serialize($assets)) . (int) $optimize;
+    // hook_library_info_alter().
+    $libraries_to_load = $this->getLibrariesToLoad($assets);
+    $cid = 'css:' . $theme_info->getName() . ':' . Crypt::hashBase64(serialize($libraries_to_load)) . (int) $optimize;
     if ($cached = $this->cache->get($cid)) {
       return $cached->data;
     }
@@ -132,7 +133,7 @@ public function getCssAssets(AttachedAssetsInterface $assets, $optimize) {
       'browsers' => [],
     ];
 
-    foreach ($this->getLibrariesToLoad($assets) as $library) {
+    foreach ($libraries_to_load as $library) {
       list($extension, $name) = explode('/', $library, 2);
       $definition = $this->libraryDiscovery->getLibraryByName($extension, $name);
       if (isset($definition['css'])) {
@@ -187,9 +188,7 @@ public function getCssAssets(AttachedAssetsInterface $assets, $optimize) {
    * Returns the JavaScript settings assets for this response's libraries.
    *
    * Gathers all drupalSettings from all libraries in the attached assets
-   * collection and merges them, then it merges individual attached settings,
-   * and finally invokes hook_js_settings_alter() to allow alterations of
-   * JavaScript settings by modules and themes.
+   * collection and merges them.
    *
    * @param \Drupal\Core\Asset\AttachedAssetsInterface $assets
    *   The assets attached to the current response.
@@ -207,9 +206,6 @@ protected function getJsSettingsAssets(AttachedAssetsInterface $assets) {
       }
     }
 
-    // Attached settings win over settings in libraries.
-    $settings = NestedArray::mergeDeepArray([$settings, $assets->getSettings()], TRUE);
-
     return $settings;
   }
 
@@ -219,9 +215,10 @@ protected function getJsSettingsAssets(AttachedAssetsInterface $assets) {
   public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
     $theme_info = $this->themeManager->getActiveTheme();
     // Add the theme name to the cache key since themes may implement
-    // 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(). Additionally add the current language to
+    // support translation of JavaScript files via hook_js_alter().
+    $libraries_to_load = $this->getLibrariesToLoad($assets);
+    $cid = 'js:' . $theme_info->getName() . ':' .  $this->languageManager->getCurrentLanguage()->getId() . ':' . Crypt::hashBase64(serialize($libraries_to_load)) . (int) (count($assets->getSettings()) > 0) . (int) $optimize;
 
     if ($cached = $this->cache->get($cid)) {
       list($js_assets_header, $js_assets_footer, $settings, $settings_in_header) = $cached->data;
@@ -239,8 +236,6 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
         'browsers' => [],
       ];
 
-      $libraries_to_load = $this->getLibrariesToLoad($assets);
-
       // Collect all libraries that contain JS assets and are in the header.
       $header_js_libraries = [];
       foreach ($libraries_to_load as $library) {
@@ -329,8 +324,10 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
       $this->cache->set($cid, [$js_assets_header, $js_assets_footer, $settings, $settings_in_header], CacheBackendInterface::CACHE_PERMANENT, ['library_info']);
     }
 
-
     if ($settings !== FALSE) {
+      // Attached settings override both library definitions and
+      // hook_js_settings_build().
+      $settings = NestedArray::mergeDeepArray([$settings, $assets->getSettings()], TRUE);
       // Allow modules and themes to alter the JavaScript settings.
       $this->moduleHandler->alter('js_settings', $settings, $assets);
       $this->themeManager->alter('js_settings', $settings, $assets);

core/tests/Drupal/Tests/Core/Asset/AssetResolverTest.php

+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Tests\Core\Asset\AssetResolverTest.
+ */
+
+namespace Drupal\Tests\Core\Asset;
+
+use Drupal\Core\Asset\AssetResolver;
+use Drupal\Core\Asset\AttachedAssets;
+use Drupal\Core\Asset\AttachedAssetsInterface;
+use Drupal\Core\Cache\MemoryBackend;
+use Drupal\Tests\UnitTestCase;
+
+/**
+ * @coversDefaultClass \Drupal\Core\Asset\AssetResolver
+ * @group Asset
+ */
+class AssetResolverTest extends UnitTestCase {
+
+  /**
+   * The tested asset resolver service.
+   *
+   * @var \Drupal\Core\Asset\AssetResolver
+   */
+  protected $assetResolver;
+
+  /**
+   * The mocked library discovery service.
+   *
+   * @var \Drupal\Core\Asset\LibraryDiscoveryInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $libraryDiscovery;
+
+  /**
+   * The mocked library dependency resolver.
+   *
+   * @var \Drupal\Core\Asset\LibraryDependencyResolverInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $libraryDependencyResolver;
+
+  /**
+   * The mocked module handler.
+   *
+   * @var \Drupal\Core\Extension\ModuleHandlerInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $moduleHandler;
+
+  /**
+   * The mocked theme manager.
+   *
+   * @var \Drupal\Core\Theme\ThemeManagerInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $themeManager;
+
+  /**
+   * The mocked language manager.
+   *
+   * @var \Drupal\Core\Language\LanguageManagerInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $languageManager;
+
+  /**
+   * The cache backend to use.
+   *
+   * @var \Drupal\Core\Cache\CacheBackendInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $cache;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+
+    $this->libraryDiscovery = $this->getMockBuilder('Drupal\Core\Asset\LibraryDiscovery')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $this->libraryDependencyResolver = $this->getMock('\Drupal\Core\Asset\LibraryDependencyResolverInterface');
+    $this->libraryDependencyResolver->expects($this->any())
+      ->method('getLibrariesWithDependencies')
+      ->willReturnArgument(0);
+    $this->moduleHandler = $this->getMock('\Drupal\Core\Extension\ModuleHandlerInterface');
+    $this->themeManager = $this->getMock('\Drupal\Core\Theme\ThemeManagerInterface');
+    $active_theme = $this->getMockBuilder('\Drupal\Core\Theme\ActiveTheme')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $active_theme->expects($this->any())
+      ->method('getName')
+      ->willReturn('bartik');
+    $this->themeManager->expects($this->any())
+      ->method('getActiveTheme')
+      ->willReturn($active_theme);
+
+    $this->languageManager = $this->getMock('\Drupal\Core\Language\LanguageManagerInterface');
+    $english = $this->getMock('\Drupal\Core\Language\LanguageInterface');
+    $english->expects($this->any())
+      ->method('getId')
+      ->willReturn('en');
+    $japanese = $this->getMock('\Drupal\Core\Language\LanguageInterface');
+    $japanese->expects($this->any())
+      ->method('getId')
+      ->willReturn('jp');
+    $this->languageManager = $this->getMock('\Drupal\Core\Language\LanguageManagerInterface');
+    $this->languageManager->expects($this->any())
+      ->method('getCurrentLanguage')
+      ->will($this->onConsecutiveCalls($english, $english, $japanese, $japanese));
+    $this->cache = new TestMemoryBackend('llama');
+
+    $this->assetResolver = new AssetResolver($this->libraryDiscovery, $this->libraryDependencyResolver, $this->moduleHandler, $this->themeManager, $this->languageManager, $this->cache);
+  }
+
+  /**
+   * @covers ::getCssAssets
+   * @dataProvider providerAttachedAssets
+   */
+  public function testGetCssAssets(AttachedAssetsInterface $assets_a, AttachedAssetsInterface $assets_b, $expected_cache_item_count) {
+    $this->assetResolver->getCssAssets($assets_a, FALSE);
+    $this->assetResolver->getCssAssets($assets_b, FALSE);
+    $this->assertCount($expected_cache_item_count, $this->cache->getAllCids());
+  }
+
+  /**
+   * @covers ::getJsAssets
+   * @dataProvider providerAttachedAssets
+   */
+  public function testGetJsAssets(AttachedAssetsInterface $assets_a, AttachedAssetsInterface $assets_b, $expected_cache_item_count) {
+    $this->assetResolver->getJsAssets($assets_a, FALSE);
+    $this->assetResolver->getJsAssets($assets_b, FALSE);
+    $this->assertCount($expected_cache_item_count, $this->cache->getAllCids());
+
+    $this->assetResolver->getJsAssets($assets_a, FALSE);
+    $this->assetResolver->getJsAssets($assets_b, FALSE);
+    $this->assertCount($expected_cache_item_count * 2, $this->cache->getAllCids());
+  }
+
+  public function providerAttachedAssets() {
+    $time = time();
+    return [
+      'same libraries, different timestamps' => [
+        (new AttachedAssets())->setAlreadyLoadedLibraries([])->setLibraries(['core/drupal'])->setSettings(['currentTime' => $time]),
+        (new AttachedAssets())->setAlreadyLoadedLibraries([])->setLibraries(['core/drupal'])->setSettings(['currentTime' => $time + 100]),
+        1
+      ],
+      'different libraries, same timestamps' => [
+        (new AttachedAssets())->setAlreadyLoadedLibraries([])->setLibraries(['core/drupal'])->setSettings(['currenttime' => $time]),
+        (new AttachedAssets())->setAlreadyLoadedLibraries([])->setLibraries(['core/drupal', 'core/jquery'])->setSettings(['currentTime' => $time]),
+        2
+      ],
+    ];
+  }
+
+}
+
+if (!defined('CSS_AGGREGATE_DEFAULT')) {
+  define('CSS_AGGREGATE_DEFAULT', 0);
+}
+
+if (!defined('JS_DEFAULT')) {
+  define('JS_DEFAULT', 0);
+}
+
+class TestMemoryBackend extends MemoryBackend {
+  public function getAllCids() {
+    return array_keys($this->cache);
+  }
+}