Convert drupal_flush_all_caches() function to a Cache Clearer service

+++ b/core/rebuild.php
@@ -50,7 +50,7 @@
-  drupal_rebuild($autoloader, $request);
+  Cache::rebuild($autoloader, $request);

Not sure that related to cache - it is more like controller

Add deprecated annotation and trigger_error function

Btw while you on it - Icm sure it needs to check core commands, probably there is similar command so they could reuse same codebase

It seems like drupal_flush_all_caches() is doing most of the heavy lifting here.

It doesn't have a 'kill includes' issue, either.

How about we turn drupal_flush_all_caches() and drupal_rebuild() into a class. Maybe call it CacheRebuilder.

It seems like the replacement for drupal_flush_all_caches() would be concrete (rather than static), so we could inject services, but it just rebuilds the container so maybe not.

why not to convert the rebuild.php script to the controller

we could inject services

I see no way, rebuild.php was introduced exactly for the case when core can't bootstrap #2097189: Add a rebuild script

I see bins deleted in Rebuilder::rebuild()

   ...
  foreach (Cache::getBins() as $bin) {
     $bin->deleteAll();
  }

  // Disable recording of cached pages.
  \Drupal::service('page_cache_kill_switch')->trigger();

  self::flushAllCaches();
  ...

But Rebuild::flushAllCaches() has bins clearing code already

    foreach (Cache::getBins() as $service_id => $cache_backend) {
      $cache_backend->deleteAll();
    }

And this last bin clearer code has unused $service_id key.

So, it seems bins in Rebuilder::rebuild() were cleared twice. If this is correct then need create non-public member function binsClear() to exclude similar code in both functions. If this is incorrect then need remove bins clear code from Rebuilder::rebuild() and remove unused $service_id key from Rebuild::flushAllCaches().

That we are clearing them twice is AFAIK actually a long-standing bug and there is an issue about that, so I wouldn't bother to make a method for that.

1. +++ b/core/includes/common.inc
   @@ -1183,10 +1123,15 @@ function drupal_flush_all_caches() {
     */
    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(REQUEST_TIME, 10, 36));
   +  @trigger_error('_drupal_flush_css_js() is deprecated in drupal:8.8.0. It will be removed from drupal:9.0.0. Use \Drupal\Core\Cache\Rebuilder::flushCssJs() instead. See https://www.drupal.org/node/3014783', E_USER_DEPRECATED);
   +  Rebuilder::flushCssJs();
    }
   

   this was prefixed with an underscore, so it was considered an internal/private function. should we add a public replacement for that, are there any direct calls to it? If not we could make it protected and just keep the old code here, it's a single line anyway.

   Checking below, looks like there are actually are some calls to it. interesting..

2. +++ b/core/lib/Drupal/Core/Cache/Rebuilder.php
   @@ -0,0 +1,208 @@
   +  /**
   +   * Rebuilds all caches even when Drupal itself does not work.
   +   *
   +   * @param mixed $class_loader
   +   *   The class loader. Normally Composer's ClassLoader, as included by the
   +   *   front controller, but may also be decorated; e.g.,
   +   *   \Symfony\Component\ClassLoader\ApcClassLoader,
   +   *   \Symfony\Component\ClassLoader\WinCacheClassLoader,
   +   *   or \Symfony\Component\ClassLoader\XcacheClassLoader.
   +   * @param \Symfony\Component\HttpFoundation\Request $request
   +   *   The current request.
   +   *
   +   * @see rebuild.php
   +   */
   +  public static function rebuild($class_loader, Request $request) {
   

   Naming...

   I like putting all this stuff into a Rebuilder class.

   But the thing is that flushAllCaches() obviously does so much more than that (to the point that I'm not sure that this should be in the cache component, it merely uses that, maybe Site or Utility instead?). It really is the rebuild part in this whole thing.

   What the current rebuild() method really does is a safe bootstrap and then rebuild.

   What about:

   ::rebuild() (or rebuildAll()?) instead of flushAllCaches()
   ::safeBootstrapAndRebuild() or something like that for the current rebuild()?

3. +++ b/core/lib/Drupal/Core/Cache/Rebuilder.php
   @@ -0,0 +1,208 @@
   +   * Flushes all persistent caches, resets all variables, rebuilds all data
   +   * structures.
   +   * At times, it is necessary to re-initialize the entire system to account for
   +   * changed or new code. This function:
   +   * - Clears all persistent caches:
   +   *   - The bootstrap cache bin containing base system, module system, and
   +   *     theme system information.
   +   *   - The common 'default' cache bin containing arbitrary caches.
   +   *   - The page cache.
   +   *   - The URL alias path cache.
   +   * - Resets all static variables that have been defined via drupal_static().
   +   * - Clears asset (JS/CSS) file caches.
   +   * - Updates the system with latest information about extensions (modules and
   +   *   themes).
   +   * - Updates the bootstrap flag for modules implementing bootstrap_hooks().
   +   * - Rebuilds the full database schema information (invoking hook_schema()).
   +   * - Rebuilds data structures of all modules (invoking hook_rebuild()). In
   +   *   core this means
   +   *   - blocks, node types, date formats and actions are synchronized with the
   +   *     database
   +   *   - The 'active' status of fields is refreshed.
   +   * - Rebuilds the menu router.
   

   I'm always torn between doing 1:1 conversions to methods and actually improving things.

   I'm pretty sure that list of persistent caches is way outdated, URL alias for example isn't its own cache bin anymore and doesn't seem worth mentioning here. Honestly, I'd just drop that ist and just say that all registered cache bins are cleared?

   The part below rebuild is also outdated, the only thing that still implements that hook is block.module and the only thing that it does is disable blocks that are in invalid regions.

   On the other side, we could maybe mention new things that are happening that are more important, like invalidating twig caches, and that kernel rebuild/service container invalidation?

Re-roll + fix for \Drupal\system\Controller\DbUpdateController::triggerBatch()

Remove ununsed use statements from patch and coding fixes.

Patch no longer applies. Needs a reroll.

Re-roll patch #36.
please review

Fixing test case failure issues in #39, please review.

Fixed test case failure in #41, please review.

Hi @voleger
Updated patch please take a look.

The function needs be kept, NW for that

+++ b/core/includes/common.inc
@@ -512,89 +512,14 @@ function show(&$element) {
-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(REQUEST_TIME, 10, 36));

+++ b/core/lib/Drupal/Core/Cache/Rebuilder.php
@@ -0,0 +1,201 @@
+  public static function flushCssJs() {
+    // The timestamp is converted to base 36 in order to make it more compact.
+    \Drupal::state()->set('system.css_js_query_string', base_convert(REQUEST_TIME, 10, 36));

This function is used in contrib http://grep.xnddx.ru/search?text=_drupal_flush_css_js

SO should be deprecated to not break it

@voleger Thank you, it looks mostly ready, just few places needs some work

1. +++ b/core/lib/Drupal/Core/Cache/Rebuilder.php
   @@ -0,0 +1,201 @@
   +   * @param mixed $class_loader
   +   *   The class loader. Normally Composer's ClassLoader, as included by the
   +   *   front controller, but may also be decorated; e.g.,
   +   *   \Symfony\Component\ClassLoader\ApcClassLoader,
   +   *   \Symfony\Component\ClassLoader\WinCacheClassLoader,
   +   *   or \Symfony\Component\ClassLoader\XcacheClassLoader.
   

   There's no such classloaders in 9.x https://www.drupal.org/node/3116384

   Looks we should \Composer\Autoload\ClassLoader as \Drupal\Core\DrupalKernel::__construct() defines

2. +++ b/core/lib/Drupal/Core/Cache/Rebuilder.php
   @@ -0,0 +1,201 @@
   +   * @todo Try to clear page/JS/CSS caches last, so cached pages can still be
   +   *   served during this possibly long-running operation. (Conflict on
   +   *   bootstrap cache though.)
   +   * @todo Add a global lock to ensure that caches are not primed in concurrent
   +   *   requests.
   

   both todo needs issues and links

3. +++ b/core/lib/Drupal/Core/Cache/Rebuilder.php
   @@ -0,0 +1,201 @@
   +  public static function rebuildAll() {
   +    $module_handler = \Drupal::moduleHandler();
   +    // Flush all persistent caches.
   +    // This is executed based on old/previously known information, which is
   +    // sufficient, since new extensions cannot have any primed caches yet.
   +    $module_handler->invokeAll('cache_flush');
   

   no reason for the variable as it used only once and redefined later

4. +++ b/core/lib/Drupal/Core/Cache/Rebuilder.php
   @@ -0,0 +1,201 @@
   +  public static function flushCssJs() {
   +    // The timestamp is converted to base 36 in order to make it more compact.
   +    \Drupal::state()->set('system.css_js_query_string', base_convert(REQUEST_TIME, 10, 36));
   

   not sure REQUEST_TIME should be used in new code

It still needs to update summary, and then it looks ready for commiter's eyes

Looks good to go! Thank you @voleger

1. +++ b/core/lib/Drupal/Core/Cache/Rebuilder.php
   @@ -0,0 +1,198 @@
   +  /**
   +   * Rebuilds all caches even when Drupal itself does not work.
   +   *
   +   * @param mixed $class_loader
   +   *   The class loader. Normally \Composer\Autoload\ClassLoader, as included by
   +   *   the front controller, but may also be decorated.
   +   * @param \Symfony\Component\HttpFoundation\Request $request
   +   *   The current request.
   +   *
   +   * @see rebuild.php
   +   */
   +  public static function safeBootstrap($class_loader, Request $request) {
   

   I'm not convinced this should be here. The only place that utility.inc is loaded from is from rebuild.php. This code is rescue code and for me does not belong in a class that is autoloadable by regular Drupal. If we want to remove it from utlity.inc then I think we should consider moving it to rebuild.php. There are usages in contrib but one has to wonder why drupal_flush_all_caches() was not the correct thing for them to use - http://grep.xnddx.ru/search?text=drupal_rebuild%28&filename=

2. +++ b/core/lib/Drupal/Core/Cache/Rebuilder.php
   @@ -0,0 +1,198 @@
   +  /**
   +   * 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.
   +   *
   +   * @internal
   +   */
   +  public static function flushCssJs() {
   +    // 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));
   +  }
   
   +++ b/core/modules/system/src/Controller/DbUpdateController.php
   @@ -206,7 +207,7 @@ public function handle($op, Request $request) {
   +    Rebuilder::flushCssJs();
   

   We're changing something that is internal - ie. marked with an _ to a public static function. But there is a tonne of usage in contrib - http://grep.xnddx.ru/search?text=_drupal_flush_css_js&filename= . I think we should be asking ourselves if this is the right place to put this. Or is there a core service that already interacts with this state that should be responsible for doing this. Or should we be adding a little service to do this.

Sadly merge requests provide no link in mail to check interdiff)

The MR needs to be rebased to 9.2.x.

All the deprecations need to be changed from 9.1.0 to 9.2.0.

At lot of testbot failures because of: "$state parameter is deprecated since drupal:9.2.0 and removed from drupal:10.0.0" and "$query_string parameter is added since drupal:9.2.0 and is required from drupal:10.0.0".

The testbot is not happy.

@voleger I think you need to rebase your branch.
I ran the tests and following is the results

Tests: 7258, Assertions: 14538, Errors: 5, Failures: 3, Warnings: 1, Skipped: 3, Incomplete: 1

Much less than that of the testbot.

I think deprecation of drupal_rebuild() could use separate issue as it used only in front controller and few places in contrib http://grep.xnddx.ru/search?text=drupal_rebuild

then we can get rid of core/includes/utility.inc in D10

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().

Also the change here needs to be made in light of changes to drupal_flush_all_caches(). It some ways it might be best to start a fresh - or at least copying the entire current contents of drupal_flush_all_caches() again - it now accepts a $kernel argument. And that needs to be copied over.

Fixed drupal CS issue of #81, still needs work for comments #83, #84.

as a feature

Created a new child issue #3358336: Deprecate _drupal_flush_css_js() and create a new AssetQueryString cache service for #83 Part 1.

I'd say this issue is postponed on #3358336: Deprecate _drupal_flush_css_js() and create a new AssetQueryString cache service

Why not introduce events for this?

One event for "container rebuilt".
Another event for "flush all caches". This could be triggered by "container rebuilt" so subscribers only need to subscribe to one of them, not both.

In DrupalKernel::initializeContainer(), we add this:

     // If there is no container and no cached container definition, build a new
     // one from scratch.
     if (!isset($container) && !isset($container_definition)) {
       $container = $this->compileContainer();
 
       // Only dump the container if dumping is allowed. This is useful for
       // KernelTestBase, which never wants to use the real container, but always
       // the container builder.
       if ($this->allowDumping) {
         $dumper = new $this->phpArrayDumperClass($container);
         $container_definition = $dumper->getArray();
       }
+
+      /** @var \Symfony\Contracts\EventDispatcher\EventDispatcherInterface $event_dispatcher */
+      $event_dispatcher = $container->get('event_dispatcher');
+      $event_dispatcher->dispatch(new ResetEvent(), self::EVENT_CONTAINER_REBUILT);
     }

Or maybe there is an even better place where this event should be fired.

----

Yes, I saw CONTAINER_INITIALIZE_SUBREQUEST_FINISHED, but I am not sure I understand what it does.
It looks like it has a different purpose, and does not fire on cache rebuild exclusively.

#3358336: Deprecate _drupal_flush_css_js() and create a new AssetQueryString cache service is in so un-postponing.

New issue to explore the events idea: #3375976: Use events to flush all caches on container rebuild
I don't think it is mutually exclusive with this one, but let's see where it goes.

I took the approach of tagged services. I think it works quite nicely. Would be interested in some early feedback before I go much further.

I took the approach of tagged services. I think it works quite nicely. Would be interested in some early feedback before I go much further.

Interesting. This is a valid alternative to events.

Can we make a good argument why cache tags + interface is preferable over events?
I don't have a clear opinion one way or the other, but I am curious, and I think it is good if we can describe why we do it this way.

Currently, event subscribers are more verbose, but I think we should move towards using the #[AsEventSubscriber] attribute from symfony.
Service tags + interface means that the service always needs to implement the interface, and the method always needs to have the same name.

I think in isolation it is hard to determine which is better.

Btw, at some point we could change drupal_rebuild() and DrupalKernel so that we call cache rebuild from the kernel instead of from drupal_rebuild(). But at that point we can also question other parts of the process.

Seems a number of failures in MR 4430

Hoping for some more feedback before going much further.

Gotcha will put back to review then.

Have posted in #contribute to try and get some eyes but no luck so far.

This needs an IS and CR update since #3358336: Deprecate _drupal_flush_css_js() and create a new AssetQueryString cache service went in. Might also be worth adding the options discussed here: i.e. tagged services vs events.

For me on tagged services vs. events I like the simplicity of https://git.drupalcode.org/project/drupal/-/merge_requests/4430/diffs#c8...

I also went for tagged services in #3257725: Add a cache prewarm API.

Part of this is a long-standing dislike for the Symfony Event API though, for here specifically my main reason is the Event class. There's no state to pass around here, there's no need to be able to stop propagation etc. we just want to run a method with no arguments and no return value.

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

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Rebased on 11.x

Sorry @catch Do you mind pasting the code you are referring to in #108? I think the link got broken in the rebase.

diff --git a/core/lib/Drupal/Core/Asset/AssetQueryString.php b/core/lib/Drupal/Core/Asset/AssetQueryString.php
index fa0d6c20805b4a423b99bca10247a2144a53a2b3..b49fa322369bad5b4ee5daea31fbe2925f70bdea 100644
--- a/core/lib/Drupal/Core/Asset/AssetQueryString.php
+++ b/core/lib/Drupal/Core/Asset/AssetQueryString.php
@@ -5,6 +5,7 @@
 namespace Drupal\Core\Asset;
 
 use Drupal\Component\Datetime\TimeInterface;
+use Drupal\Core\Cache\CacheClearerInterface;
 use Drupal\Core\State\StateInterface;
 
 /**
@@ -13,7 +14,7 @@
  * The string changes on every update or full cache flush, forcing browsers to
  * load a new copy of the files, as the URL changed.
  */
-class AssetQueryString implements AssetQueryStringInterface {
+class AssetQueryString implements AssetQueryStringInterface, CacheClearerInterface {
 
   /**
    * The key used for state.
@@ -48,4 +49,11 @@ public function get(): string {
     return $this->state->get(self::STATE_KEY, '0');
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function clearCache(): void {
+    $this->reset();
+  }
+
 }

This is what I meant by the simplicity of tagged services, it very much suits having neither context to pass in nor anything to return.

Thanks @catch. Moving to NW for that and issue tags.

That's not a needs work issue, it's supporting the current implementation over using an event.

New symfony events with attribute subscription and no event class are a bit better than how core currently uses events, but don't think we fully support that yet.

Merge in 11.x

This something that should be aiming for 10.2

Didn't mean to change status.

I'm not sure whether we should wait for #3257725: Add a cache prewarm API ? Is there any overlap?

Doesn't need to be postponed on #3257725: Add a cache prewarm API IMO, any overlap won't be too hard to deal with.

Seems to have test failures.

MR 4430 has significantly different code, but no explanation for the changes.
I've rebased MR 9 from 9.4.x to 11.x

I skipped over too many comments and missed context.

I was confused by the title & summary still referencing drupal_rebuild, which is no longer present in the latest MR (and potentially covered by prewarming in the future instead?).

Updated title & summary to reflect current changes only applying drupal_flush_all_caches