Problem/Motivation

The current flushJsCss() method in admin_toolbar_tools only updates the system.css_js_query_string state value, which changes the version query string for assets. However, it does not actually invalidate cached asset data, nor does it trigger regeneration of aggregated CSS and JS files. This leads to confusion and false expectations, especially when source files change but aggregates remain outdated.

Currently, the method flushJsCss() looks like this:

public function flushJsCss() {
  $this->state()
    ->set('system.css_js_query_string', base_convert($this->time->getCurrentTime(), 10, 36));
  $this->messenger()->addMessage($this->t('CSS and JavaScript cache cleared.'));
  return new RedirectResponse($this->reloadPage());
}

This only updates the query string (?v=) used in asset URLs, which may invalidate browser-side caches, but it does not actually clear or rebuild Drupal's internal asset caches.

In many cases, changes to CSS/JS files (especially in libraries defined via *.libraries.yml) do not take effect until either:

A full drush cr is run,
Or cache tags like library_info, and asset.css/asset.js are manually invalidated.
This is misleading, as the UI implies that all JS/CSS caches are being cleared.

Steps to reproduce

1. Modify a JS or CSS file registered in a library.
2. Click the “Flush CSS and JavaScript” link in the toolbar.
3. Reload the page — aggregated files remain unchanged.
4. Run drush cr — now they update.

Proposed resolution

Enhance flushJsCss() so that it actually clears asset-related caches, similar to what happens during drupal_flush_all_caches() — but without fully flushing everything.

Issue fork admin_toolbar-3514574

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ankondrat4 created an issue. See original summary.

ankondrat4 opened merge request !129

ankondrat4’s picture

ankondrat4 commented

I’ve pushed a proposed improvement to the issue fork.
The patch enhances the flushJsCss() method to invalidate asset-related cache tags and trigger regeneration of CSS/JS aggregates.

MR !129 is ready for review and feedback — thanks!

ankondrat4’s picture

ankondrat4 commented
Assigned: ankondrat4 » Unassigned
Status: Active » Needs review
dydave’s picture

dydave commented
Version: 3.5.3 » 3.x-dev
Priority: Major » Normal
Status: Needs review » Needs work

Thanks Andrii (@ankondrat4) for reporting this issue and the very detailed issue summary, it's greatly appreciated!

It looks like a valid request and I "think" I encountered this issue before, that's probably why when I develop locally in assets (JS in particular), I usually just use drush cr 😅

No problem for trying to improve this feature 👌

OK, I've taken a very quick look at the merge request and there are problems, see here:
https://git.drupalcode.org/project/admin_toolbar/-/merge_requests/129/di...

1 Code Quality finding detected
major: 
Function _drupal_flush_css_js not found

Resulting in a lot of failing PHPSTAN jobs in the build, see:

 
It looks like this is due to the fact the function _drupal_flush_css_js was removed in D11 😅
https://api.drupal.org/api/drupal/core%21includes%21common.inc/function/...
(D11 is the current supported version tested in the build pipelines)

Deprecated
in drupal:10.2.0 and is removed from drupal:11.0.0. Use Use \Drupal\Core\Asset\AssetQueryStringInterface::reset() instead.

Therefore, we're probably going to need to add a check if (version_compare(\Drupal::VERSION, '11.0.0', '>=')) { in your code to support the different versions of the function + "maybe" ignore the old function in PHPSTAN if it keeps complaining (// phpstan-ignore), see:
https://phpstan.org/user-guide/ignoring-errors

Lastly, if possible, maybe we could think of how this change could be added to the automated tests of the module?
(I could help giving some advice on that 🙂)

I hope I was able to answer your request and give you an initial review of the merge request, but feel free to let us know if you have any questions or concerns on any aspect of this comment, we would surely be glad to help.
Thanks in advance!

dydave’s picture

dydave commented
Status: Needs work » Needs review

@ankondrat4:

Took a bit of time to fix the issues mentioned above at #5, mostly:

1 - Fixed PHPSTAN errors by injecting a dependency to service 'asset.query_string' conditionally, for versions greater than 10.2.0 to call \Drupal\Core\Asset\AssetQueryStringInterface::reset() and ignored PHPSTAN validation for deprecated function _drupal_flush_css_js().

2 - Added very basic Functional Tests coverage for the ToolbarController class:
Created a specific ticket to #3514774: PHPUnit: Improve Tests coverage for admin_toolbar_tools menu links and 'ToolbarController' class.
 

I have tested with the exact steps described in the issue summary and the changes seem to work fine, as expected:
The aggregated JS file seemed to have changed after I changed a line in one of the module's JS files.

Automated tests all seem to be passing as well 🟢: Back to Needs review

@ankondrat4: Could you please help reviewing, testing the merge request again and report back?

Otherwise, any help testing, reviewing or comments would be greatly appreciated.
Thanks in advance!

ankondrat4’s picture

ankondrat4 commented

Hello @dydave,

I have reviewed your changes and refactored the code. We don't need to run twice reseting the cache query string added to all CSS and JavaScript URLs. Function _drupal_flush_css_js() equals the same as we have in module: $this->state()->set('system.css_js_query_string', base_convert($this->time->getCurrentTime(), 10, 36)); So I propose to stay it without depending to core. functions.

/**
 * Changes the dummy query string added to all CSS and JavaScript files.
 *
 * Changing the dummy query string appended to CSS and JavaScript files forces
 * all browsers to reload fresh files.
 */
function _drupal_flush_css_js() {
  // The timestamp is converted to base 36 in order to make it more compact.
  Drupal::state()->set('system.css_js_query_string', base_convert(\Drupal::time()->getRequestTime(), 10, 36));
}

Please review and feedback — thanks!

dydave’s picture

dydave commented
Status: Needs review » Needs work

Thanks Andrii (@ankondrat4) for the feedback and the changes.

I also wish we could skip having different cases...

But unfortunately, we're still seeing deprecation errors:

Remaining self deprecation notices (1)

  1x: The 'system.css_js_query_string' state is deprecated in drupal:10.2.0. Use \Drupal\Core\Asset\AssetQueryStringInterface::get() and ::reset() instead. See https://www.drupal.org/node/3358337.
    1x in AdminToolbarToolsToolbarControllerTest::testAdminToolbarToolsToolbarController from Drupal\Tests\admin_toolbar_tools\Functional

Looks like we might need different cases for 10.2 and the use of the reset method for more recent versions (D11).

As of 10.2, "The 'system.css_js_query_string' state is deprecated" as well as the function _drupal_flush_css_js.

If we really need this call, looks like it's either _drupal_flush_css_js/system.css_js_query_string for versions before 10.2 and \Drupal\Core\Asset\AssetQueryStringInterface::reset()/get() for versions above.

Could you please try reverting your changes and test again see if the previous version could fix the issue?

It looks like we should not be using the system.css_js_query_string state variable anymore 😅

Thanks in advance!

ankondrat4’s picture

ankondrat4 commented
Status: Needs work » Reviewed & tested by the community

Tested with the last changes. All works as expected from my side.
+1 RTBC.

dydave’s picture

dydave commented

Great thanks Andrii (@ankondrat4)!!!

It looks great! You even removed the unneeded line:

$this->state()
      ->set('system.css_js_query_string', base_convert($this->time->getCurrentTime(), 10, 36));

which seems to be a duplicate from what the function _drupal_flush_css_js does.

Exactly what I had in mind 🤩

If everything is working for you, let's get this merged in 👍

  • dydave committed da170d62 on 3.x authored by ankondrat4 
    Issue #3514574 by ankondrat4, dydave: Improved Admin Toolbar Tools '...
dydave’s picture

dydave commented
Status: Reviewed & tested by the community » Fixed

Thanks a lot Andrii (@ankondrat4) for your help reporting this issue, contributing the changes and your patience to work through the compatibility issues or PHPSTAN errors, it's greatly appreciated! 🙏

Following your last changes, I "sneaked in" a few minor changes with the Doc comment blocks of the protected static $modules property in all of module's Tests classes to inherit the doc {@inheritdoc}.

Since the MR came back green 🟢 for all Tests, I went ahead and merged the changes above at #11.

At this point, we should be able to consider this issue should be Fixed.

Feel free to let us know if you have any questions or concerns on any of the latest changes to the module, this issue or the project in general, we would surely be glad to help.
Thanks again Andrii for the great work and help! 🥳

dydave’s picture

dydave commented

PS. Andrii (@ankondrat4): If you still have a bit of time and would like to help contributing to the module, we would greatly appreciate some help testing and reviewing the following issues in priority:

 
Any help, comments and testing/reporting feedback would be greatly appreciated.
Thanks in advance! 🙏

Status: Fixed » Closed (fixed)

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