Replace drupal_flush_all_caches() with targeted invalidation#829
Conversation
Avoid rebuilding the entire service container and router when only theme CSS has changed. Instead, clear only CSS/JS aggregates, the render cache, and the library discovery cache. Fix #826
|
Review findings:
|
jjroelofs
left a comment
There was a problem hiding this comment.
Thanks for narrowing the cache clear. I found two stale-cache paths that the old full flush covered and the targeted version currently misses, so I think this needs one more pass before merge.
| // If the CSS & JS aggregation are enabled we need to clear the caches. | ||
| drupal_flush_all_caches(); | ||
| // Clear CSS/JS aggregated files. | ||
| \Drupal::service('asset.css.collection_optimizer')->deleteAll(); |
There was a problem hiding this comment.
This targeted replacement drops the CSS/JS asset query-string reset that drupal_flush_all_caches() performed (_drupal_flush_css_js() on Drupal 9/early 10, asset.query_string->reset() on newer core). When aggregation is disabled, themesettings-*.css keeps the same URL/query suffix after the file is overwritten, so browsers or edge caches can keep serving the old CSS. Please reset the asset query string as part of this helper, with a fallback for the supported Drupal 9.3/10 range.
| \Drupal::service('asset.css.collection_optimizer')->deleteAll(); | ||
| \Drupal::service('asset.js.collection_optimizer')->deleteAll(); | ||
| // Clear the render cache so pages pick up the new CSS. | ||
| \Drupal::cache('render')->invalidateAll(); |
There was a problem hiding this comment.
Invalidating only the render bin still leaves full page/dynamic page cache responses that can contain the old asset URL or old query string. The previous full cache flush cleared those persistent bins, so anonymous users may continue to get stale CSS until those entries expire. Please also invalidate the relevant response caches/tags, or ensure the generated theme-settings CSS is represented by a cache tag that reaches cached page responses.
jjroelofs
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. The asset query-string reset and page-cache invalidation concerns from the earlier review look addressed in 517dd92. I found one remaining Drupal 11 compatibility issue below.
| // Clear the discovery cache for library definitions. | ||
| \Drupal::cache('discovery')->invalidateAll(); | ||
| // Invalidate the library info cache. | ||
| \Drupal::service('library.discovery')->clearCachedDefinitions(); |
There was a problem hiding this comment.
This still calls clearCachedDefinitions(), which is deprecated in Drupal 11.1 while this theme declares ^11 support. Please use clear() when the service provides it, falling back to clearCachedDefinitions() for Drupal 9/10 compatibility, e.g. store \Drupal::service('library.discovery') in a variable and branch with method_exists($library_discovery, 'clear').
clearCachedDefinitions() is deprecated in Drupal 11.1. Use clear() when available, falling back for Drupal 9/10 compatibility.
Linked issues
Solution
Replace
drupal_flush_all_caches()with targeted cache invalidation that only clears CSS/JS aggregates, render cache, and library discovery cache. This avoids rebuilding the entire service container and router when only a CSS file has changed.Checklist