Remove the persist service tag

Can you please explain the problem space here? Why it is important? What are the nasty issues? Why does it exist in Symfony world and not Drupal world? Are we not going to support in D8 at all? Is it still possible for contrib to create a persist service? I hope these are not frustrating questions.
And thanks for all the awesome work in the issue queue.

Just to let me understand it correctly, the purpose of the issue is to remove the API functioanlity but rather hardcode the two services during rebuild?

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -988,6 +988,22 @@ protected function attachSynthetic(ContainerInterface $container) {
   +      while ($request = array_shift($requests)) {
   ...
   +        $container->get('router.request_context')->fromRequest($request);
   

   Don't we just need to register the last one? Just curious. We also do have RequestContext::fromRequestStack

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1071,17 +1087,16 @@ protected function compileContainer() {
   +      if ($definition->isSynthetic()) {
   

   Just curious, can't we also manually take over the synthetic services? Classloader, kernel and service container should be available from within the DrupalKernel

I think we should do that.

Keeping just the request stack sounds good to me.

Probably needs a re-roll.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -1059,6 +1028,25 @@ protected function attachSynthetic(ContainerInterface $container) {
+      while ($request = array_shift($requests)) {
+        $container->get('request_stack')->push($request);
+      }

+1 for not trying to maintain the existing object but instead start with a new one.

RTBC + 1

Code changes look good. The patch could use some comments. Thanks for providing the complete context. IS summary is not updated with api changes. This is a task so needs a beta evaluation. Also, we are removing a functionality so we can update some old change record and perhaps add a new one for this.

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1059,6 +1028,25 @@ protected function attachSynthetic(ContainerInterface $container) {
   +    // Restore request stack.
   

   I think we can explain here why we have to restore request stack?

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1059,6 +1028,25 @@ protected function attachSynthetic(ContainerInterface $container) {
   +      $original_request_stack = clone $this->container->get('request_stack');
   

   By directly looking at the code I can't tell that $this->container is an old container so can we please add a comment here as well?

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1059,6 +1028,25 @@ protected function attachSynthetic(ContainerInterface $container) {
   +      while ($request = $original_request_stack->pop()) {
   +        $requests[] = $request;
   +      }
   ...
   +      while ($request = array_shift($requests)) {
   +        $container->get('request_stack')->push($request);
   +      }
   

   This can be done in a single loop?

4. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1059,6 +1028,25 @@ protected function attachSynthetic(ContainerInterface $container) {
   +      if ($request) {
   +        $container->get('router.request_context')->fromRequest($request);
   

   I think this needs a comment as explained in #6.1

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -1059,10 +994,63 @@ protected function attachSynthetic(ContainerInterface $container) {
+        if ($request->hasSession()) {
+          $request->setSession($container->get('session'));
+        }
...
+    if ($this->container->initialized('session')) {

Would the above initialize the session and the below then being true?

Would it make sense to first check the session, then the request stack?

This is just a reroll for now.

Here's another reroll and a couple more fixes.

This should fix all the kernel tests, but the fail in TwigDebugMarkupTest is a bit beyond me..

Rebuilding a container in place is always gonna be tricky. Instead I converted to test to a kernel test

Rerolled for 9.2.x, raw interdiff attached. The messenger messages are now persisted across rebuilds, I've tried to update that code to match the new method.

Re-rolled patch #40.
Please review.

@Gauravmahlawat thanks, but #41 is missing the TwigDebugMarkupTest that was converted from a functional to a kernel test.

I opened a merge request instead of using the patch workflow, fixed the test and coding standards issues, and deleted the old test.

Unsure why the template order is slightly different in the kernel test vs the functional test - the XSS injected template name appears in a different position in the list, which is why the test assertions have changed slightly.

Related #3300306-6: Drupal ContainerBuilder allows mutation of non-synthetic services explains why it's now required

Rebased and synced some small changes. Couple deprecation and some Symfony 6 stuff conflicted.

Looking at the patch and there's no BC. That made sense when the patch was written against 8.0 beta but guess that's not true anymore. :(

I'd expect this to still not be very disruptive but seems there are a handful of actual modules using it.
https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22name%...

I bet no way to make it in to 9.5 so moving to 10.1, MR needs rebase

Added a patch for Drupal 10.1.x.

@neclimdul what do you mean with BC? We getting rid of tag, no way to provide BC

We can't provide backwards compatibility in Drupal 10, but we could potentially have a 9.5.x-only patch that triggers a deprecation when the persist tag is used.

We could also have a similar warning in Drupal 10 that just informs people the persist tag is a no-op (or throws an exception even?).

@neclimdul maybe we get rid of synthetic services in follow-up? Then BC will be easier

Sorry, I see the confusion. I meant what catch said, there isn't a version of this that says "Hey, you're using persist on your service and that's not going to do anything in 10.x" or what ever deprecation schedule this fits into.

Thanks, that's where I stuck - we can't provide BC - only a deprecation warning

I just hit this in #3278493: Make it easier for theme builders to enable Twig debugging and disable render cache. How Drupal\Core\KeyValueStore\KeyValueMemoryFactory is tried to be persisted causes tests to break because the service isn't persisted.

Debasing patch #56

Given #58 should we split this into two issues? Deprecate the persist service tag in 10.3, and then remove it in 11.0?

Rebased against 11.x in a new branch on MR!6658. Locally the functional version of TwigDebugMarkupTest passes, so I removed the conversion to a kernel test here.

Should restoreServiceState() be decoupled from the kernel? Each persistent service could be responsible for transferring its data from the old to the new one, via an intermediate service that collects persist services tags and calls each service to do the work?

Hiding patches for clarity

Added #68 to remaining tasks

Moving to NW for change record unless you are waiting for an answer to 68 first, if that's the case can put back in review. But I didn't get that vibe that was the case.

Polished a bit new code

Re#68

Looking at UpdateKernel and InstallerKernel I see both overriding initializeContainer() but calling parent, so I think it's ok to keep it in DrupalKernel

reroll after #3419987: Fix Container::reset() and provide DrupalKernel::resetContainer()