Remove the aggregate stale file threshold and state entry

Random failure.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rerolled the patch

Running PHPStan on *all* files.
------ ---------------------------------------------------------------------------
Line core/tests/Drupal/Tests/Core/Asset/CssCollectionOptimizerLazyUnitTest.php
------ ---------------------------------------------------------------------------
55 Class Drupal\Core\Asset\CssCollectionOptimizerLazy constructor
invoked with 11 parameters, 10 required.
115 Class Drupal\Core\Asset\CssCollectionOptimizerLazy constructor
invoked with 11 parameters, 10 required.

Updated patch #23 by addressing #24, please review it.

Thanks!

The interdiff looks good.

The last thing here is to deal with #17 and #18, I think we should probably remove the post update hook from this patch, and open a follow-up against Drupal 11 to add it there - can just have that part of the patch attached. Marking needs work for that.

catch → credited acbramley → .

catch → credited Chi → .

catch → credited kevinquillen → .

catch → credited lauriii → .

Re-rolled. Thinking about #17/#18 more I think if someone somewhere really is using the old classes, they should be responsible for cleaning up the state entries too, so leaving the update in. Crediting people from the other issue.

A workaround if you wanted this in 10.1.0 would be to set stale_file_threshold to 0.

Bumping to major since this is a DX issue for local development, shame it stalled.

Fixing cs issues.

We could also backport the ::deleteAll() changes to 10.0 without the deprecations. That would improve things for local development and deployments.

To me #32 sounds reasonable. Even if @alexpott disagrees, it seems fine to run the post update now and we could have another post update later to clean it up again if he feels strongly about this. I don't think there are any usages in contrib.

+++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizerLazy.php
@@ -135,27 +131,15 @@ public function optimize(array $css_assets, array $libraries) {
   public function getAll() {

Went through contrib uses of this class and didn't see any calls for this. Seems fine to make this change and deprecate the method.

+1 for backporting the changes to \Drupal\Core\Asset\CssCollectionOptimizerLazy::deleteAll to 10.1.x.

Here's the 10.1 backport.

+++ b/core/lib/Drupal/Core/Asset/AssetCollectionOptimizerInterface.php
@@ -25,6 +25,11 @@ public function optimize(array $assets, array $libraries);
+   * @deprecated in drupal:10.2.0 and is removed from drupal:11.1.0. There is

+++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizerLazy.php
@@ -135,27 +131,15 @@ public function optimize(array $css_assets, array $libraries) {
+    @trigger_error(__METHOD__ . ' is deprecated in drupal:10.2.0 and will be removed in drupal:11.0.0, there is no replacement. See https:// www.drupal.org/node/3301744', E_USER_DEPRECATED);

+++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizerLazy.php
@@ -150,26 +146,15 @@ public function optimize(array $js_assets, array $libraries) {
+    @trigger_error(__METHOD__ . ' is deprecated in drupal:10.2.0 and will be removed in drupal:11.0.0, there is no replacement. See https:// www.drupal.org/node/3301744', E_USER_DEPRECATED);

Could be fixed on commit - s/11.1/11.0

Will this fix the issue in 10.1 where if a user clears cache, it doesn't refresh the aggregate CSS/JS that's pulled down?

@mherchel - I just applied this to my 10.1 and other than a warning "] rmdir(/files/css): Directory not empty FileSystem.php:267" it did appear to fix the aggregate CSS/JS on my Pantheon deploy.

@mherchel yes it should.

@mherchel yes it should.

Just tested this out, and it doesn't appear to be working.

Not sure if this blocks this issue, or if this should be reported in a followup.

Testing steps

1. Ensure aggregated CSS/JS, caching enabled etc.
2. View source. Copy filename including querystring and save it (It looks something like

   /sites/default/files/css/css_hegpbZvWLCegz1eocO6sYJ0X_AGboa15OU4bOdIZPaI.css?delta=0&language=en&theme=olivero&include=eJx9UFuSgyAQvBDqGfYkqQF6lV1kqBmI6-0XEzWVVCo_MPQDmnYsGLzUTLHPwqNA1bgN5ORwnyKT_9nRXephud4FqeCvVIoH9UC6GNKvGl21YB4sKZ7uIFfCFTeR4dhG4SHzAoHv7NrZyO5BfAPeXAMWHW5rP7OvESdfaNTzkOgaRiqBU6doeTzJepIKEjd1S_B4Z8gS5jfyRCK8PHcQNEdqHRTmaEmGfTdVIYdom_vQMuz9hVaOpEbohDxBfHPXzVql07K2MsYH8vRcIWsbO1OiEfK5-COJTizF1XJIjrMhP4d0eQneFwE-UP3ErZLX7_ZQRxlfm83YMF5yyBiO4R-XNOx7

3. Clear cache
4. View source again and compare the aggregate filename/querystring against what I checked before.
5. I expect the filename or querystring to change but it doesn't. I also manually modified Olivero's CSS (to intentionally make it out of date), but that had no affect.

@mherchel can you check if drush cr deletes the aggregate file from disk? And can you try a hard browser refresh to see if you get the new contents from it once it's recreated?

Yep. Verified its working 🙌

Testing steps

1. Verified path of CSS aggregate file
2. Ran drush cr
3. noted that entire CSS directory is now removed from sites/files
4. modified a CSS file in Olivero
5. Refreshed the path to the CSS file
6. Noted that the new file is created, and it has the updated content.

Thank you!

+++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizerLazy.php
@@ -142,20 +142,7 @@ public function getAll() {
-    $this->state->delete('drupal_css_cache_files');

Shouldn't we keep this?

@lauriii it's no longer set by anything non-deprecated in core so it's redundant code in the new classes.

Discussed #43/#44 with @lauriii - while the delete should be a no-op, in the interests of keeping changes to the absolute minimum in 10.1, adding it back.

Committed 405f8b33a7 and pushed to 11.0.x. Also committed the 10.1.x patch. Thanks!

#45, It works when I clear the cache via the admin toolbar, but not when I do it via Drush.
When I run drush cr I get the following messages:

[notice] The file assets://css was not deleted because it does not exist.
[notice] The file assets://js was not deleted because it does not exist.

CSS and JS are not removed and new files are not created.

#45, works when I run cache clear via admin toolbar, but doesn't work via Drush.
When I run drush cr command I get the following messages:

[notice] The file assets://css was not deleted because it does not exist.
[notice] The file assets://js was not deleted because it does not exist.

As a result, old CSS and JS files are not deleted, and new ones are not created.

@BadWebCoder please open a new issue. The existing code was already deleting files in that directory, just not the directory itself, so if drush isn't able to find the directory now, it probably wasn't able to find it beforehand either.

I did get the notices @BadWebCoder mentioned when I cleared cache with Drush,

$ vendor/bin/drush cr
 [notice] The file assets://css was not deleted because it does not exist.
 [notice] The file assets://js was not deleted because it does not exist.

But when I refreshed the page, my CSS changes did apply to the page.

Off-topic: I can also see that some of the forum topics I added on the site which I saw on the PC (it is a fresh install), viewing the site on a phone I can't see the most recent forum topics. Content this time and not CSS or JS.

Kindly ignore the off-topic above. It was my fault. I had wrong forum access permissions set.

If CSS aggregation is turned on, CSS changes don't show up on the site and the site remains in the past never changing, whether caches are cleared in Drush or UI.

The existing code was already deleting files in that directory, just not the directory itself

@catch, seems it is deleting the directory as well.

https://git.drupalcode.org/project/drupal/-/blob/10.1.1/core/lib/Drupal/...

Currently it's also deleted the JS directory and both are never recreated (tested on both 10.1.1 and 10.1.x-dev).

Missed a bit 📌 Remove even more of the aggregate stale file threshold and state entry Fixed .

Automatically closed - issue fixed for 2 weeks with no activity.

We are having the same issue as @FiNeX in #56.

published the change record

AFAICT this incorrectly removed system.performance:stale_file_threshold from config schema. It should have deprecated that instead? 🤔 (That is possible since #2997100: Introduce a way to deprecate config schemas → .)