Deprecate _drupal_flush_css_js() and create a new AssetQueryString cache service

Working on this

Sounds related, intrigued)

QueryString seems a bit generic. How about AssetQueryString?

Added few comments, overall looks great

- strict types - needs removal
- BC for removed state property may fire in child classes, could need DeprecatedServicePropertyTrait
- default value from state is now '0' so extra processing of NULL is gone and I think it's fine

I find it ready but not sure allowed in current state https://www.drupal.org/about/core/policies/core-change-policies/allowed-... →

Usage in contrib http://codcontrib.hank.vps-private.net/search?text=_drupal_flush_css_js%...

Usage in contrib http://codcontrib.hank.vps-private.net/search?text=_drupal_flush_css_js%...

Funny to see that what we consider a private function (because of the leading underscore) gets used like a public function anyway. 😆

larowlan → credited Wim Leers → .

Wim's comment appears unresolved

Resolved thread.

Seems all threads have been resolved but change record still needs work.

Example of before/after is always useful.

Updated the CR and added before and after. This is meant to be an internal method, so doubtful anyone would be calling this directly.

CR looks good!

claro_page_attachments_alter() still calls state directly, this needs converting to use the service:

function claro_page_attachments_alter(array &$attachments) {
  $theme_path = \Drupal::request()->getBasePath() . '/' . \Drupal::service('extension.list.theme')->getPath('claro');
  $query_string = \Drupal::state()->get('system.css_js_query_string') ?: '0';

Fixed claro_page_attachments_alter() .

Do we have a way of deprecating state variables?

We could check the state key when setting and getting, but that would be triggered by AssetQueryString which is using the same state key.

However, people shouldn't be bypassing _drupal_flush_css_js() and accessing state directly. It's like querying the database directly.

The issue isn't with _drupal_flush_css_js(), modules mostly want to read the value - we never provided an API for that, reading state directly was the only way.

Ok. An alternative approach is we switch the state key to something new in AssetQueryString, then check for the old one in \Drupal\Core\State\State::get()/::set() and throw a deprecation message?

Capturing the slack conversation with @catch and @ongwave. We will:

1. Create a new key for AsssetQueryString
2. Add deprecation trigger in State::set/get() for the old value and use the new value
3. Add a test for that
4. Use Settings::$deprecatedSettings for inspiration

I think we only need to get/set the new key; the deprecation handling should take care of that transparently to the caller.

Makes more sense. I've used the 'replacement' to swap out the deprecated key in get/set methods.

Added suggestion for test docs, otherwise RTBC++

All threads appear to be resolved and CR has before/after examples.

Overall this looks great, but I think we need a post update to remove the old state entry?

Added post-update hook.

Just a post update hook, so I think it's safe for me to put this back to RTBC.

LGTM if extra eyes are required.

Committed and pushed b3e943e926 to 11.x (10.2.x). Thanks!

Also published the change record.

I realised we forgot to add BC for the new argument here:

+  public function __construct($root, KeyValueExpirableFactoryInterface $key_value_expirable_factory, CacheBackendInterface $cache, StateInterface $state, ModuleHandlerInterface $module_handler, AccountInterface $account, BareHtmlPageRendererInterface $bare_html_page_renderer, UpdateRegistry $post_update_registry, protected AssetQueryStringInterface $assetQueryString) {

Will open a followup.

📌 Add BC for asset.query_string service to DbUpdateController::__construct Fixed

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