Cache CSS/JS asset resolving

+1 on concept, this is exactly what I've been wanting to get to ever since working on #2381473: Decrease the size of the asset library cache item at DDD Montpellier.

Will review ASAP.

1. locale_js_alter() - this has to run per-language. We could probably add the current language to the cache key though. Then document that the results of the hook are cached.

I'm curious whether we could have a build time hook as well as a runtime hook, so that we can save what we need but still have the flexility, for example for system_js_settings_alter()/

I've seen damian complaining recently about too much time needed there as well.

Can't we just not cache getJsSettingsAssets()? Precisely because it is dynamic, it doesn't make a whole lot of sense to cache it.

RE: locale_js_alter(): once again, what we're missing is… cacheability metadata :) Alternate suggestion: make \Drupal\Core\Asset\AttachedAssetsInterface implement \Drupal\Core\Cache\CacheableDependencyInterface, and give it setters to indicate that it should vary by language.
Of course, that opens the door for more such dependencies. But it also opens the door towards configurable overrides (i.e. some module has Config to override a certain asset, then we need the corresponding cache tag to be associated).
Note that I'm fine with hardcoding per-language — I'm just pointing out that there is a more general solution.

Sounds good!

1. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -191,115 +215,140 @@ protected function getJsSettingsAssets(AttachedAssetsInterface $assets) {
   +    $cid = 'js:' . $theme_info->getName() . ':' . $this->languageManager->getCurrentLanguage()->getId() . ':' .  Crypt::hashBase64(serialize($assets));
   

   is there a reason why we not just call out to sha512 from the beginning?

2. +++ b/core/modules/system/system.module
   @@ -607,9 +607,47 @@ function system_page_attachments(array &$page) {
   +    // Checks that the DB is available before filling theme_token.
   +    if (!defined('MAINTENANCE_MODE')) {
   +      $settings['ajaxPageState']['theme_token'] = \Drupal::csrfToken()->get($theme_key);
   +    }
   

   I guess this needs to be dynamic, given that this even depends on the session ...

The patch looks splendid :)

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

   We should document why the theme needs to be part of the cache key.

2. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -191,115 +215,140 @@ protected function getJsSettingsAssets(AttachedAssetsInterface $assets) {
   +    $cid = 'js:' . $theme_info->getName() . ':' . $this->languageManager->getCurrentLanguage()->getId() . ':' .  Crypt::hashBase64(serialize($assets));
   

   We should document why the theme and current language need to be part of the cache key.

3. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -191,115 +215,140 @@ protected function getJsSettingsAssets(AttachedAssetsInterface $assets) {
   +      $settings_required = in_array('core/drupalSettings', $libraries_to_load) || in_array('core/drupalSettings', $this->libraryDependencyResolver->getLibrariesWithDependencies($assets->getAlreadyLoadedLibraries()));
   +      $settings_have_changed = count($libraries_to_load) > 0 || count($assets->getSettings()) > 0;
   +      $settings_needed = $settings_required && $settings_have_changed;
   ...
   +      if ($settings_have_changed && $settings_needed) {
   

   You've changed the logic a bit here, and I think you have a small mistake: $settings_needed implies $settings_have_changed, so you only need to include $settings_needed in the if-test.

4. +++ b/core/lib/Drupal/Core/Render/theme.api.php
   @@ -826,6 +826,28 @@ function hook_library_info_build() {
   +function hook_js_settings_build(array &$settings, \Drupal\Core\Asset\AttachedAssetsInterface $assets) {
   +
   +  // Manipulate settings.
   

   Weird newline.

I think we should explicitly update also hook_js_alter() / hook_css_alter() and document that this is no longer request aware.

Fixing #2. I'm 99.9% certain the MAINTENANCE_MODE check is no longer necessary, but yes good catch that depends on session. Moved back to the alter.

If I understand correctly this is about ensuring that the right theme is used for ajax requests ...

The failure in CKEditorLoadingTest points to a likely bug in drupalSettings, this is the failing line:

isset($settings['ajaxPageState']) && in_array('ckeditor/drupal.ckeditor', explode(',', $settings['ajaxPageState']['libraries'])),

The other failures seem similar.

The failures are interesting, looking at them atm.

The problem is with that patch we add the theme_token to normal requests, unless before. This itself should not be a problem, so let's fix our code against unsane checks ...
the second interdiff just fixes the problem mentioned there ...

Alright, so this patch fixes first all the failures with the approach of interdiff-1.txt and then apply interdiff-2.txt on top of it

1. +++ b/core/modules/system/system.module
   @@ -607,9 +607,43 @@ function system_page_attachments(array &$page) {
   - * Sets values for the core/drupalSettings and core/drupal.ajax libraries.
   + * Sets values for the core/drupalSettings library.
   

   Given that theme_token is only added for the drupal/ajax library, this change seems wrong?

   The majority of the work is now done in system_js_settings_builder(), but the session-dependent values are still handled here. Perhaps we should describe it that way ([…] and session-dependent values for the core/drupal.ajax library.)?

2. +++ b/core/modules/system/system.module
   @@ -647,36 +681,15 @@ function system_js_settings_alter(&$settings, AttachedAssetsInterface $assets) {
   +  // Add the theme token to ajaxPageState, ensuring the database is available
   +  // before doing so.
      /** @var \Drupal\Core\Asset\LibraryDependencyResolver $library_dependency_resolver */
      $library_dependency_resolver = \Drupal::service('library.dependency_resolver');
      if (isset($settings['ajaxPageState']) || in_array('core/drupal.ajax', $library_dependency_resolver->getLibrariesWithDependencies($assets->getAlreadyLoadedLibraries()))) {
   -    // Provide the page with information about the theme that's used, so that
   -    // a later AJAX request can be rendered using the same theme.
   -    // @see \Drupal\Core\Theme\AjaxBasePageNegotiator
   

   I think the deleted comment contains important information that we now lost?

Left it at NR for more reviews, but I think this looks at the very least close to RTBC otherwise.

NW so we can get those details fixed, and then hopefully RTBC.

Thank you for your review!

The majority of the work is now done in system_js_settings_builder(), but the session-dependent values are still handled here. Perhaps we should describe it that way ([…] and session-dependent values for the core/drupal.ajax library.)?

Tried to improve the documentation to be clear, what depends on the current request and what not.

I think the deleted comment contains important information that we now lost?

Well, those bits are now in system_js_settings_build(), aren't they?

1. +++ b/core/modules/system/system.module
   @@ -609,7 +609,8 @@ function system_page_attachments(array &$page) {
   + * active theme but no other request dependent values.
   

   Nit: s/request dependent/request-dependent/

2. +++ b/core/modules/system/system.module
   @@ -643,7 +644,8 @@ function system_js_settings_build(&$settings, AttachedAssetsInterface $assets) {
   + * as well as theme_token ajax state.
   

   and core/drupal.ajax' 'theme_token' ajaxPageState.

3. RE: "those bits are": Oops, you're right :) Sorry!

Another review round.

1. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -191,115 +217,142 @@ protected function getJsSettingsAssets(AttachedAssetsInterface $assets) {
   +      // The current list of header JS libraries are only those libraries that are
   

   80 cols.

2. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -191,115 +217,142 @@ protected function getJsSettingsAssets(AttachedAssetsInterface $assets) {
   +            // 'scope' is a calculated option, based on which libraries are marked
   

   80 cols.

3. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -191,115 +217,142 @@ protected function getJsSettingsAssets(AttachedAssetsInterface $assets) {
   +      // If the core/drupalSettings library is being loaded or is already loaded,
   

   80 cols.

4. +++ b/core/lib/Drupal/Core/Render/theme.api.php
   @@ -826,6 +826,27 @@ function hook_library_info_build() {
   + * Add to the JavaScript settings (drupalSettings).
   

   "Add"? Shouldn't it be "Modify"?

After this one, it will really be RTBC.

Thank you for review again.

"Add"? Shouldn't it be "Modify"?

Agreed.

The conclusion of #2381473, at the end of #2381473-37: Decrease the size of the asset library cache item, is precisely what this patch does: cache at a higher level. We could choose to remove the asset library cache entirely and only keep this cache. But doing that can be a follow-up, and can be what we repurpose #2381473 for, after this lands.

Great patch. Thanks!

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 5d3a761 and pushed to 8.0.x. Thanks!

Yay! This also allowed me to fix two majors: #2381473: Decrease the size of the asset library cache item and #2378737: Consider not executing hook_library_(info)_alter() *per* library, but once for all :)

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

Note that $assets includes JS settings. So it'll actually vary by user, even though CSS assets don't care about the JS settings asset.

We should also fix the JS assets caching, but that one will be trickier.

A good way to fix the JS assets caching is to split this up in two cache entries:

a) Generic libraries + settings --- shared across all users // just would need to change getJsSettingsAssets() to not merge the assets settings
b) Attached settings + hook_js_settings_build result

Unfortunately it would be a behavior change to have hook_js_settings_build() operate on the pure libraries definition without any dynamic parts - though that would be the correct fix.

Then the code would - except for changing the cid to include only $attachments->getLibrariesToLoad() instead of the whole attachments - not need changing at all.

Maybe however that is how hook_js_settings_build() was thought of and we _should_ do this even before release?

Per #40 I opened #2603138: CSS/JS asset caching can easily be trashed as critical follow-up (justification in the issue).

Thanks for filing that issue!