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: [PP-1] 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()

Trying to revive this. Not sure if we can remove the tag entirely yet, but MR!15404 redoes this work including combining initializeContainer() and resetContainer(), which largely do the same thing.

Created a draft CR. Needs work.

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

Bot probably picked the wrong MR

I think I might be PHPCS'ing on wrong MR? Can we hide one? Just saw the 'take-2' on the end of the name of the bottom one. Sounds like it is the 'live' one?

I think I might be PHPCS'ing on wrong MR? Can we hide one? Just saw the 'take-2' on the end of the name of the bottom one. Sounds like it is the 'live' one?

Yes. I hid the older MR. I'm not sure if that was the right call or not, but it's easy to un-hide it if necessary.

I'm setting the status to Needs Work because there are still test failures that must be corrected, as mentioned in #79.

Have removed the kernel test assertions that were failing. Not sure if that is what was the right way to fix the tests. But the kernel tests are passing.

There are 2 functional tests failing. Looking at LanguageNegotiationContentEntityTest.php that fails at line 148, there is earlier inside the test function a container rebuild at line 113. Not sure how that is fixed?