Convert drupal_rebuild(), drupal_flush_all_caches() and _drupal_flush_css_js() functions into cache Rebuilder class

Created a new child issue 📌 Deprecate _drupal_flush_css_js() and create a new AssetQueryString cache service Fixed for #83

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

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.

📌 Deprecate _drupal_flush_css_js() and create a new AssetQueryString cache service Fixed is in so un-postponing.

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

kim.pepper → closed merge request !4431

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 📌 Deprecate _drupal_flush_css_js() and create a new AssetQueryString cache service Fixed 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 📌 Add a cache prewarm API and use it to distribute cache rebuids after cache clears / during stampedes Needs work .

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.

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 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 📌 Add a cache prewarm API and use it to distribute cache rebuids after cache clears / during stampedes Needs work ? Is there any overlap?

Doesn't need to be postponed on 📌 Add a cache prewarm API and use it to distribute cache rebuids after cache clears / during stampedes Needs work IMO, any overlap won't be too hard to deal with.

Removed mention of _drupal_flush_css_js() function as it is already deprecated.

Seems to have test failures.

gapple → changed the visibility of the branch 3014752-convert-functions-into-cache-rebuilder-class to hidden.

gapple → changed the visibility of the branch 3014752-convert-functions-into-cache-rebuilder-class to active.

MR 4430 has significantly different code, but no explanation for the changes.

I've rebased MR 9 from 9.4.x to 11.x

gapple → changed the visibility of the branch 3014752-convert-functions-into-cache-rebuilder-class to hidden.

gapple → changed the visibility of the branch 3014752-cache-rebuilder to hidden.

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