Improve flushJsCss() to actually invalidate JS/CSS caches and regenerate aggregates

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!

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:

• Build: https://git.drupalcode.org/issue/admin_toolbar-3514574/-/pipelines/453795
• PHPSTAN: https://git.drupalcode.org/issue/admin_toolbar-3514574/-/jobs/4733653#L50

 
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, '10.2.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!

@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:
At least with these Tests, all the methods and code in the ToolbarController class are executed, which should allow us to detect deprecation messages, various errors, crashes, etc...

The tests don't actually check whether each action actually "worked", in the sense: each task is not tested specifically.
But instead, they're executing each method's code and checking they completed without error.

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!

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!

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

Unfortunately:

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

Could you please revert your changes?
We are going to need different cases for 10.2 and the use of the reset method for more recent versions.

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

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

Thanks in advance!

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

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));

Exactly what I had in mind 🤩

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

Thanks a lot Andrii 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! 🥳

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:

• 📌 GitlabCI: Fix ESLINT validation errors Active
• 🐛 Some mandatory parameters are missing ("source") to generate a URL for route Active

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

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