Problem/Motivation

Splitting off a child issue from #3014752: Convert drupal_flush_all_caches() function to a Cache Clearer service where @alexpott suggested in comment #83:

I think this issue could do with coming in 2 parts. One that deals with the new CSS / JS query string service and converting usages of _drupal_flush_css_js() and anything that accesses the 'system.css_js_query_string' state directly. And another that deals with drupal_flush_all_caches() and drupal_rebuild().

This issue is for the first part of that.

Steps to reproduce

Proposed resolution

  • Deprecate _drupal_flush_css_js()
  • Create a new AssetQueryString cache service
  • Convert usages

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3358336

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

kim.pepper created an issue. See original summary.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Assigned: Unassigned » kim.pepper

Working on this

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#2575105: Use cache collector for state

Sounds related, intrigued)

kim.pepper opened merge request !3933

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Active » Needs review
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

QueryString seems a bit generic. How about AssetQueryString?

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Title: Deprecate _drupal_flush_css_js() and create a new QueryString cache service » Deprecate _drupal_flush_css_js() and create a new AssetQueryString cache service
Issue summary: View changes
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work

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

rpayanm made their first commit to this issue’s fork.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

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

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

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. 😆

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

larowlan credited Wim Leers.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review

Wim's comment appears unresolved

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Resolved thread.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs change record updates

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

Example of before/after is always useful.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review
Issue tags: -Needs change record updates

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

CR looks good!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

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

Do we have a way of deprecating state variables? How can we detect and communicate this to contrib? A contrib search shows quite a few uses of state directly reading system.css_js_query_string, but if someone swaps the service to do something else, this won't work any more.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review

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.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

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.

See http://grep.xnddx.ru/search?text=css_js_query_string&filename= for lots of examples.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

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?

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

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
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

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

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

Added suggestion for test docs, otherwise RTBC++

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

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

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

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

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Added post-update hook.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs review » Reviewed & tested by the community

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

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

LGTM if extra eyes are required.

  • longwave committed b3e943e9 on 11.x 
    Issue #3358336 by kim.pepper, rpayanm, andypost, longwave, smustgrave,...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

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

Also tweaked and published the change record.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

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.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

#3375477: Add BC for asset.query_string service to DbUpdateController::__construct

Status: Fixed » Closed (fixed)

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