Requests are pushed onto the request stack twice, popped once

Let's see what happens.

Ha, wrong project, wrong patch.

The symmetry here makes sense, although I'm unclear why it's failing for a simple patch... The log suggests that some code is relying on the request still being there after we've popped it, which suggests a deeper structural/ordering issue.

+1 to everything in #8.

I simply believe that there is something accessing the route match / request stack outside of the prehandle, for example during the terminate event. I could not find an exanple but \Drupal\Core\Path\AliasManager::writeCache could have been one (thought it uses the globals)

By tracing the execution path, I have found one failure mode for an anonymous user visiting the front page for the first time.

In Drupal\page_cache\StackMiddleware\PageCache->fetch(), after the response has been created by httpKernel->handle(), the request stack has been popped twice, so is empty.
Then line 247 calls responsePolicy->check() = Drupal\Core\PageCache\ChainResponsePolicy->check()
The first rule it checks is Drupal\node\PageCache\DenyNodePreview->check()
This calls routeMatch->getRouteName() = Drupal\Core\Routing\CurrentRouteMatch->getRouteName()
This calls requestStack->getCurrentRequest(), which is empty
Then getRouteMatch(Request $request) throws an error because it's expecting a Request object and is passed null

I've taken a closer look at the classes that implement HttpKernelInterface and their sequence in the StackMiddleware chain, plus the interaction with HttpKernel. The way KernelPreHandle calls DrupalKernel to push / pop the RequestStack appears to conflict with the push / pop in HttpKernel. It seems to me that only one of these classes should be in control of pushing and popping from the RequestStack.

If we decide that DrupalKernel should be in control, then we could create a class to extend HttpKernel with versions of handleRaw and finishRequest that simply comment out the RequestStack push and pop. After that I think we have a better chance of fixing this issue and avoiding future issues with subrequests.

Here is an alternative approach. It just seems to be not possible to have a symetrie, so we rather have to drop as late as possible.

• I don't understand why we can't have symmetry.
• I also don't understand why DrupalKernel::preHandle() also needs to push $request onto the stack. If that weren't necessary, removing it would solve all this at once?
• I also don't understand why #13 has even more failures.

Super confusing issue.

Just to be clear, this issue is about research, not about complaining or anything else. Its research to understand and to solve things the proper way at the end.

I also don't understand why DrupalKernel::preHandle() also needs to push $request onto the stack. If that weren't necessary, removing it would solve all this at once?

There is code executed which needs a request before the HTTP kernel jumps in. One example is \Drupal\Core\Session\SessionManager called from within \Drupal\Core\StackMiddleware\Session::handle

I don't understand why we can't have symmetry.

So we have a onion of middlewares:

negotation(reverse_proxy(page_cache(pre_handle(session(HttpKernel)))))

When we would remove the request from the stack nothing outside of pre_handle could access the request on the stack again. This is not only directly accessing of
the request, but ALSO the current route match, outside of the scope of pre_handle. There seems to be code relying on it, implicitly.
One example:

Drupal\Core\PageCache\ChainResponsePolicy->check(Object, Object)
Drupal\Core\ProxyClass\PageCache\ChainResponsePolicy->check(Object, Object)
Drupal\page_cache\StackMiddleware\PageCache->fetch(Object, 1, 1)
Drupal\page_cache\StackMiddleware\PageCache->lookup(Object, 1, 1)

So here is an alternative approach. Let's see how this helps

I also don't understand why #13 has even more failures.

More failures as? Patch number 1 which was the wrong patch? Or do you compare with #6 and use some more advanced math?

I was not complaining. I was just stating that I don't understand most of what's been going on here so far :)

Just like Crell said in #8: he doesn't understand why it's failing for a simple patch. I also don't understand that failure. But I also don't understand most of what was posted after that.

I'm hoping somebody will be able to come up with a good explanation that will make everybody understand. That is all :)

I'm hoping somebody will be able to come up with a good explanation that will make everybody understand. That is all :)

Well, its so simple. People relied on that behaviour over time.

As per #16, this patch is not intended as a final solution, but just to test what I said in #12

I also locally tested another way to do the same thing as #20, which is to have a version of Symfony\Component\HttpFoundation\RequestStack with an optional boolean parameter on the push/pop, so that it ignores the push/pop from HttpKernel. This has the same effect as #20 but leads to a lot more changed files, mostly in tests that use the request stack. It has the advantage that it doesn't mess with HttpKernel itself.

So where next ? If you ignore that the approach is not ideal, does the fact that #20 passes mean it "fixes" the original issue ?

@cpj
Well, no, the fact that it passes just means we have no test coverage. This now still doesn't solve the issue. It now just pushes once in DrupalKernel but never pops.

+++ b/core/lib/Drupal/Core/StackMiddleware/HttpKernelWrapper.php
@@ -0,0 +1,237 @@
+
+  private function varToString($var)
+  {
+    if (is_object($var)) {
+      return sprintf('Object(%s)', get_class($var));
+    }

GRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRR I love symfony's idea of sharing code

@dawehner,

OK, understood. I will look again at the number and sequence of pushes/pops. Yes, the private functions in HttpKernel are a real pain. Some of the other oldest parts of Symfony2 are written in a similar style.

It's not yet clear to me if we really do need to disable the push/pop from HttpKernel, but assuming we do, I wasn't sure if it was preferable to essentially replacing most of the HttpKernel class with our own version or to replace the request-stack service. Replacing HttpKernel is simpler but might be harder to keep in sync with Symfony's HttpKernel class, replacing the request-stack service with an optional parameter will leave HttpKernel untouched, but requires lots of one-line changes, mostly in tests. Any preference ?

Mh, we could change the request stack to check whether we the current top entry is the same as we already try to it. This way the calls in HttpKernel would simply not do anything.

Nevertheless, we need to add some popping in DrupalKernel,

So we do we agree that this is the behaviour we want?

This is another attempt to simplify this issue and again is not intended as a final solution. I tried various approaches to control the push/pop processes but the way HttpKernel and RequestStack are written makes it very difficult to find a clean solution. This version uses separate request stack services for DrupalKernel and HttpKernel. The intention of RequestStackPush MIddleware is to move the push/pop outside of DrupalKernel, but in this test it is disable because right now the installer calls DrupalKernel->preHandle directly, which needs to set up the RequestStack for the install to work. Lets see where it fails.

Finally had time to look at this again. This completes the concept started in 28. I have successfully tested it with a dummy module that creates multiple sub-requests, so lets see if it works here. The patch adds a second request stack service and a middleware service called RequestStackMiddleware.

Symfony HttpKernel normally manages the request stack, but Drupal needs the request stack before Symfony HttpKernel::handleRaw() pushes the current request to the stack and after HttpKernel::finishRequest() pops it. There is no clean way to modify the behaviour of HttpKernel, hence we need two request stacks. 'request_stack' is used by Drupal and 'symfony_request_stack' is used by Symfony.

The RequestStackMiddleware service manages the Drupal request stack. It has the highest middleware priority in core.services.yml so that the current request is pushed / popped from the request stack before / after any other Middleware service is called.

The request stack push is removed from DrupalKernel::preHandle(). A push to the request stack is added to install.core.inc install_begin_request() because the stack middleware is not active during the install process and the installer uses the request stack.

@cpj Any ideas about the remaining failures?

30 works fine for me against 8.1.x, so am resubmitting for testing

@dawehner - looks like the errors in #35, where they refer to null being passed to line #99 of core/lib/Drupal/Core/Routing/CurrentRouteMatch.php, are because after these changes, the tests don't initialise the request stack any more, probably because of the change to DrupalKernel.php. Where does this initialisation take place for the tests ? Sorry, I'm not expert enough in Drupal tests to work it out for myself.

Added a push/pop to DrupalKernel.php terminate(request, response) because this also references the request stack, which is already popped when terminate is called. Ran simpletest locally on Drupal\block\Tests\BlockHtmlTest, which now passes. So lets see what happens now...

really nice progress!

So this may fix one tiny failure in RequestStackKernelTest, which needs to call the request stack middleware service rather than http_kernel.

Another attempt to fix one failure in RequestStackKernelTest. This time we let RequestStackMiddleware handle the request to push/pop correctly

After debugging with big-pipe enabled, the problem is obvious. Anything that refers to the request stack in $response-send() is going to fail. Typically, this will be because modules re-open the session, which needs the current request. The reason big-pipe etc work in head is that in the current implementation we push twice & pop once, so there is still a request object on the stack in $response-send() and $response->terminate(). I will investigate solution options.

This patch adds send(Request $request, Response $reponse) to DrupalKernal. This wraps the HttpFoundation $response->send() with a push/pop to request stack. This works locally with big-pipe enabled, so lets see if more tests pass...

A new approach which attempts to solve the following issues:

• Push the request to the request stack as early as possible
• Pop the request from the request stack as late as possible
• Stop HttpKernel from pushing to the request stack.

The existing call to DrupalKernel::preHandle() from the KernelPreHandle StackMiddleWare, pushes to the request stack early enough, so this is unchanged.

The remaining issues are hopefully resolved with following new classes

• RequestStackWrapper is a wrapped request stack, that prevents HttpKernel from pushing/popping from the real request stack.
• HttpKernelWrapper injects RequestStackWrapper into HttpKernel via a setter.
• StackMiddlewareSubscriber pops the stack right at the end of execution, when HttpKernel issues KernelEvents::TERMINATE

The key new learning is that StackMiddleWare alone cannot solve this problem. HttpKernel issues KernelEvents::TERMINATE after the StackMiddleWare classes have terminated. There are subscribers to KernelEvents::TERMINATE that require the current request, so the pop from the request stack must occur after these have run. A controlled way to do this is with a new subscriber to the KernelEvents::TERMINATE event that has a priority that means it runs last of all. Only __destruct() functions run after KernelEvents::TERMINATE completes.

Lets see what happens.

This time with a modified version of the unit test from #26, which now tests the request stack is empty after $http_kernel->terminate($request, $response).

Haha, so the unit test breaks?

I think moving the request stack handling out of http kernel is the right thing to do, given that the middleware layer is the place we interact with incoming/fake http requests.

1. +++ b/core/core.services.yml
   @@ -715,10 +720,15 @@ services:
      http_middleware.cors:
   -     class: Asm89\Stack\Cors
   -     arguments: ['%cors.config%']
   -     tags:
   -       - { name: http_middleware }
   +    class: Asm89\Stack\Cors
   +    arguments: ['%cors.config%']
   +    tags:
   +      - { name: http_middleware }
   

   That seems to be an unrelated change.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/StackMiddlewareSubscriber.php
   @@ -0,0 +1,57 @@
   +
   +/**
   + * Terminates the environment.
   + */
   +class StackMiddlewareSubscriber implements EventSubscriberInterface   {
   

   For documentation purposes I think we should try to answer what terminating the environment means. What is terminated ...

3. +++ b/core/lib/Drupal/Core/StackMiddleware/HttpKernelWrapper.php
   @@ -0,0 +1,27 @@
   +/**
   + * HttpKernel notifies events to convert a Request object to a Response one.
   + *
   + * This version wraps HttpKernel to add a setter function to allow requestStack
   + * to be overwritten with a RequestStackWrapper. RequestStackWrapped prevents
   + * HttpKernel from pushing / popping to the normal request stack.
   + *
   + * This setter is necessary because Symfony does not provide an interface for
   + * RequestStack, so it is not possible to inject an alternative implementation
   + * of RequestStack into the constructor of HttpKernel.
   + */
   

   Nice documentation! I don't get the last point though. We extend the existing class, so from just the type system, it should be allowed to pass it in. Maybe I'm missing something here :)

4. +++ b/core/lib/Drupal/Core/StackMiddleware/RequestStackWrapper.php
   @@ -0,0 +1,108 @@
   + * Request stack that controls the lifecycle of requests.
   + *
   + * This version wraps the RequestStack push() and pop() functions to prevent the
   + * caller from pushing / popping to the normal request stack. The other
   + * functions of RequestStack work as normal.
   + *
   + * RequestStackWrapper is injected into Symfony's HttpKernel via a setter
   + * function in the HttpKernelWrapper service.
   + *
   + * This is necessary because Drupal needs the request stack to be populated
   + * before HttpKernel::handleRaw() calls push() and after
   + * HttpKernel::finishRequest() calls pop(). This also prevents both Drupal and
   + * Symfony pushing the same request to RequestStack twice and only popping the
   + * request once.
   + *
   + * Drupal pushes to RequestStack in DrupalKernel::preHandle(), called from
   + * KernelPreHandle::handle(). Drupal pops from RequestStack in
   + * StackMiddleWareSubscriber, the very last subscriber to
   + * KernelEvents::TERMINATE, called right before end of the execution cycle.
   + */
   +class RequestStackWrapper Extends RequestStack {
   

   Nice documentation!

5. +++ b/core/lib/Drupal/Core/StackMiddleware/RequestStackWrapper.php
   @@ -0,0 +1,108 @@
   +  public function getMasterRequest()
   +  {
   +   return $this->wrappedRequestStack->getMasterRequest();
   +  }
   

   Just a quick comment, this patch doesn't comply yet with our coding standard, see https://www.drupal.org/docs/develop/standards/coding-standards Using phpcs with the rules defined in https://www.drupal.org/project/coder make your life much easier!

6. +++ b/core/tests/Drupal/Tests/Core/StackMiddleware/RequestStackMiddlewareTest.php
   @@ -0,0 +1,49 @@
   +
   +      $request = Request::create('/');
   +      $response = $http_kernel->handle($request);
   +      $http_kernel->terminate($request, $response);
   +
   +      $this->assertNull($request_stack->getCurrentRequest());
   +      $this->assertNull($request_stack->getMasterRequest());
   

   It is great to have test coverage here

Hi @dawehner,

Thanks for the comments in #54, which I will work though.

Yes, I just cannot make unit test work, although I'm not sure you can call it "unit test" when it goes through the entire request / response cycle. Locally I have built a much more detailed set of unit tests, but so far it doesn't produce the results I expect. Any hints or links on how to debug unit tests with Xdebug in PhpStorm ?

Here is the meta issue #2218651: [meta] Make Drupal compatible with persistent app servers like ReactPHP, PHP-PM, PHPFastCGI, FrankenPHP, Swoole

With some help from @dawehner, I think I finally have a set of tests that should work - at least they pass phpunit locally

Same patch after phpcbf

Removed redundant code from test.
Updated documentation as per #54
I think I'm done here.

Great work! Thank you @cpj!

1. +++ b/core/lib/Drupal/Core/EventSubscriber/StackMiddlewareSubscriber.php
   @@ -0,0 +1,60 @@
   +/**
   + * KernelEvents::TERMINATE subscriber, to pop
   + * the current Request object from the request stack at the end
   + * of the execution cycle.
   + */
   +class StackMiddlewareSubscriber implements EventSubscriberInterface {
   

   We should explain more why we are doing things here.

2. +++ b/core/lib/Drupal/Core/StackMiddleware/HttpKernelWrapper.php
   @@ -0,0 +1,24 @@
   +
   +/**
   + * HttpKernel notifies events to convert a Request object to a Response one.
   + *
   + * This version wraps HttpKernel to add a setter function to allow requestStack
   + * to be overwritten with a RequestStackWrapper. RequestStackWrapped prevents
   + * HttpKernel from pushing / popping to the normal request stack.
   + */
   +class HttpKernelWrapper extends HttpKernel {
   +
   +  /**
   +   * @param \Drupal\Core\StackMiddleware\RequestStackWrapper $requestStack
   +   *   The wrapped request stack.
   +   */
   +  public function setRequestStack(RequestStackWrapper $requestStack) {
   +    $this->requestStack = $requestStack;
   +  }
   +
   +}
   

   Why can't we pass in the request stack as normal parameter in the constructor?

Hi @dawehner,

I added more documentation to StackMiddlewareSubscriber. See what you think.

As suggested, I've added HttpKernelWrapper::__construct() to override HttpKernel::__construct() and to inject RequestStackWrapper that way. I removed the redundant setter for RequestStackWrapper from HttpKernelWrapper.

Nice to see this patch green finally. Great job @cpj! I have some comments / questions:

1. Seeing as the approach no more uses stack middleware, I think the RequestStackWrapper and HttpKernelWrapper should be moved to more appropriate namespaces - maybe \Drupal\Core where the DrupalKernel also resides.

2. Do we really need the RequestStackWrapper as it seems to just replicate the Symfony request stack. The class doc says "This also prevents both Drupal and Symfony pushing the same request to RequestStack twice and only popping the request once.". Maybe I'm not seeing everything, but the code doesn't seem to do that. Are we planning to do uniqueness checks? The unit test would need to cover that.

3. A more appropriate name for StackMiddlewareSubscriber may be RequestStackSubscriber or something similar since this is no more stack middleware.

4. Great test coverage!

Hi @almaudoh,

1. This work is intimately linked with StackMiddleware, so I see no reason to move it to a different namespace.

2. RequestStackWrapper is absolutely critical to this fix and is not the same as RequestStack. It is used by HttpKernel only. It prevents HttpKernel from push/pop to/from the RequestStack but still gives HttpKernel access to the RequestStack.

3. See 1.

#63.2 Ok, thanks for clarifying.

The doc in RequestStackWrapper needs to be updated:

+++ b/core/lib/Drupal/Core/StackMiddleware/RequestStackWrapper.php
@@ -0,0 +1,105 @@
+ * RequestStackWrapper is injected into Symfony's HttpKernel via a setter
+ * function in the HttpKernelWrapper service.

This should be adjusted to reflect the constructor injection now.

Thanks @almaudoh - I've updated the doc in RequestStackWrapper.

+++ b/core/lib/Drupal/Core/StackMiddleware/HttpKernelWrapper.php
@@ -0,0 +1,35 @@
+/**
+ * HttpKernel notifies events to convert a Request object to a Response one.
+ *
+ * This version uses RequestStackWrapper rather than RequestStack.
+ * RequestStackWrapper prevents HttpKernel from pushing / popping to
+ * the normal request stack.
+ */
+class HttpKernelWrapper extends HttpKernel {

I still don't understand this class. The request stack wrapper is a valid request stack, so you should be able to simply pass it into the normal http kernel.

Hi @dawehner - I don't think you can pass HttpKernelWrapper directly to HttpKernel because the constructor of HttpKernel expects RequestStack. Perhaps I'm missing something ?

EDIT: I mean "I don't think you can pass RequestStackWrapper directly to HttpKernel because the constructor of HttpKernel expects RequestStack"

Hi @dawehner,

my mistake, of course, you're correct. RequestStackWrapper is an instance of RequestStack so can be passed to HttpKernel. ( I'm clearly spending way too much time working in C#, with it's stricter parameter type checking).

I've removed the HttpKernelWrapper class, and this version of core.services.yml injects RequestStackWrapper into HttpKernel.

+++ b/core/tests/Drupal/Tests/Core/StackMiddleware/RequestStackMiddlewareTest.php
@@ -0,0 +1,94 @@
+    $this->assertSame($master_1, $master_2);
+    $this->assertSame($master_1, $master_3);
+    $this->assertSame($master_1, $master_4);
+    $this->assertSame($master_1, $master_5);
+
+    $this->assertSame($current_1, $request_1);
+    $this->assertSame($current_2, $request_2);
+    $this->assertSame($current_3, $request_1);
+    $this->assertSame($current_4, $request_3);
+    $this->assertSame($current_5, $request_1);
+
+    $this->assertSame($parent_2, $request_1);
+    $this->assertSame($parent_4, $request_1);

I wonder whether it would be easier to understand if this checking is done more near the getting of the request objects. What do you think?

OK @dawehner - is this version of RequestStackMiddlewareTest easier to understand ? I rearranged the code and added more comments.

I personally think so. Thank you @cpj

Hi @dawehner - OK, thanks. Are we done here ?

@cpj
Can you add an issue summary to describe the possible solutions and why we went with the one in the patch?

I ran into this issue while working on the GraphQL module... So I ended up doing this: https://github.com/fubhy/graphql-drupal/pull/100/files

Bug fixes are committed to the current stable release, which is 8.4.

The current state can be seen in this issue: nobody is working on this. It’d be great if you could help move this issue forward! :)

Needed a reroll.

Docs clean-up, btw why not add this "poping" in \Drupal\Core\EventSubscriber\KernelDestructionSubscriber::getSubscribedEvents() this one also supposed to run last

1. +++ b/core/lib/Drupal/Core/EventSubscriber/StackMiddlewareSubscriber.php
   @@ -0,0 +1,72 @@
   +   * Pop the current Request object from the request stack.
   

   s/Pop/Pops

2. +++ b/core/lib/Drupal/Core/EventSubscriber/StackMiddlewareSubscriber.php
   @@ -0,0 +1,72 @@
   +  /**
   +   *
   +   */
   +  public static function getSubscribedEvents() {
   

   needs doc block

3. +++ b/core/lib/Drupal/Core/StackMiddleware/RequestStackWrapper.php
   @@ -0,0 +1,105 @@
   +   * Call getCurrentRequest() on the wrapped request stack.
   ...
   +   * Call getMasterRequest() on the wrapped request stack.
   ...
   +   * Call getMasterRequest() on the wrapped request stack.
   

   should be inherited

The smallest of changes

array() becomes [].

It just needs to update summay why another subscriber used and that is what not clear to me in #82 about KernelDestructionSubscriber

It just needs to update summay why another subscriber used and that is what not clear to me in #82 about KernelDestructionSubscriber

This sums it up. It's destroying requests objects, so it needs to be _the last thing_, even after the core destruct event.

The destruct subscriber has a priority of -100.

+++ b/core/lib/Drupal/Core/EventSubscriber/StackMiddlewareSubscriber.php
@@ -0,0 +1,75 @@
+ * HttpKernel::terminate() issues KernelEvents::TERMINATE. There are
+ * other subscribers to KernelEvents::TERMINATE that require the
+ * current Request object, so the pop from the request stack must
+ * occur after these have run. StackMiddlewareSubscriber has the
+ * lowest priority so that it runs last of all. Only a few object
+ * __destruct() methods run after KernelEvents::TERMINATE completes
+ * and these should not require the current Request object.
...
+    // To be called last, this must be the lowest numbered TERMINATE subscriber.
+    // The lowest numbered Symfony TERMINATE subscriber is ProfilerListener,
+    // with priority -1024.
+    return [
+      KernelEvents::TERMINATE => ['onKernelTerminate', -4096],
+    ];

So we need to communicate that into the issue summary, then?

Reroll for 8.9

--- /core8/2613044-83.patch
+++ /core8/2613044-83-89x.patch
@@ -1,8 +1,8 @@
 diff --git a/core/core.services.yml b/core/core.services.yml
-index 3f68677200..84e43b6e8b 100644
+index 93e57d0fb4..ca4d6191cc 100644
 --- a/core/core.services.yml
 +++ b/core/core.services.yml
-@@ -675,6 +675,9 @@ services:
+@@ -698,6 +698,9 @@ services:
      class: Symfony\Component\HttpFoundation\RequestStack
      tags:
        - { name: persist }
@@ -12,16 +12,16 @@
    current_route_match:
       class: Drupal\Core\Routing\CurrentRouteMatch
       arguments: ['@request_stack']
-@@ -713,7 +716,7 @@ services:
+@@ -736,7 +739,7 @@ services:
      class: Stack\StackedHttpKernel
    http_kernel.basic:
      class: Symfony\Component\HttpKernel\HttpKernel
--    arguments: ['@event_dispatcher', '@controller_resolver', '@request_stack']
-+    arguments: ['@event_dispatcher', '@controller_resolver', '@wrapped_request_stack']
-   http_middleware.negotiation:
-     class: Drupal\Core\StackMiddleware\NegotiationMiddleware
-     tags:
-@@ -739,6 +742,11 @@ services:
+-    arguments: ['@event_dispatcher', '@controller_resolver', '@request_stack', '@http_kernel.controller.argument_resolver']
++    arguments: ['@event_dispatcher', '@controller_resolver', '@wrapped_request_stack', '@http_kernel.controller.argument_resolver']
+   http_kernel.controller.argument_resolver:
+     class: Symfony\Component\HttpKernel\Controller\ArgumentResolver
+     arguments: ['@http_kernel.controller.argument_metadata_factory', ['@argument_resolver.request_attribute', '@argument_resolver.request', '@argument_resolver.psr7_request', '@argument_resolver.route_match', '@argument_resolver.default']]
+@@ -784,6 +787,11 @@ services:
       arguments: ['%cors.config%']
       tags:
         - { name: http_middleware, priority: 250 }

So this one and related the only blocker remaining

Updated issue summary a bit

Patch fixes test but failures in views looks valid, the last one is about related #2962157: TestSiteApplicationTest requires a database despite being a unit test

Debugged last failed test and the issue is that now subrequest in \Drupal\Tests\taxonomy\Kernel\Views\TaxonomyDefaultArgumentTest::initViewWithRequest() does not populate route match parameters
Which is caused by subrequest returns 404 for /node/1

Route rebuild no longer needed

re-roll for 9.3

Fix usage of deprecated getMasterRequest()

Could this be the cause of #3218022: Running an HTTP subrequest leaves the request stack in an incorrect state?

Looks it could be combined with #2331913: Move DrupalKernel::preHandle code into kernel decorator
it makes sense in context of #2467021: Make it possible to just use Kernel::boot() and not require preHandle

few TODO added in #2484991: Add the session to the request in KernelTestBase, BrowserTestBase, and drush

Rerolled.
Changes affected by CR Drupal\Core\Http\RequestStack is deprecated
Created MR

Thank you, some tests failed and it needs CR

As there's many+1 in related issue it could be follow-up to prevent popping of ech request twice

Fixed tests after https://www.drupal.org/node/3357408

2 tests still fail

I've looked at the MR and the current solution is quite confusing for me. I wonder if instead of having a wrapped request stack we can pass in a NULL for the request stack and allow the Symfony HttpKernel to use it's own request stack - separate from Drupals. This would decouple the request stacks which I think might make this make more sense than wrapping.

Alternatively we could admit the \Symfony\Component\HttpKernel\HttpKernel doesn't work for us and we could copy and maintain the code in a way that does.

So the idea of just allowing the Symfony code to instantiate its own request stack did not work - see https://git.drupalcode.org/project/drupal/-/merge_requests/9861 - interesting fails.

I've updated the old MR to pass PHPStan and coding standards on 11.x plus I've made the service it adds private and less useful looking. I'm still not convinced that adding a new service is better than reworking Symfony\Component\HttpKernel\HttpKernel to work the way we want it to.

> Alternatively we could admit the \Symfony\Component\HttpKernel\HttpKernel doesn't work for us and we could copy and maintain the code in a way that does.

The fact that we use the Symfony HttpKernel is pretty useful over at #3390193: Add a drupalGet() method to KernelTestBase.

Hit a pretty gnarly bug from this yesterday on a client project that was making use of sub-requests and cacheable json responses.

The net result was cache poisoning via the route cache context.

With a sub-request we push each request to the stack twice but only pop it once, the current route match can end up still pointing at the sub-request rather than the parent request and as a result any cacheable responses that use the route cache context end up with the wrong route name and parameters.

This lead to a scenario where an anonymous user could request a JSON page that made a subrequest to a regular node page but the JSON page ended up being cached with the node page's route params. After that any request to the node page was getting the cached JSON response.

What's left here?

What's wrong with the original MR that tries to pop off the extra request?

Not sure which to review as there are 2 MRs but both have test failures

Okay I think all the tests are fixed here. The approach now pops the request in DrupalKernel::terminate(). I think this is a nice symmetry with the push in DrupalKernel::preHandle(). The same object does the push and pop - which makes sense to me and I think addresses #126.

The test fix in core/modules/taxonomy/tests/src/Kernel/Views/TaxonomyDefaultArgumentTest.php is interesting and I think shows why #125 is gnarly and there are probably more problems for sub requests here.

It would be fantastic to be done here.

Actually I still think this is wrong because when we handle a sub request we're not popping the sub request off the stack once we've handled it.

Updated the issue summary to reflect what is changing/being fixed here.

I've changed the test coverage to have very explicit testing of the request stack when making sub-requests and after termination.

What is very interesting about this work is that I think it is wrong for our services like the route builder that runs in termination to need the request. But changing this is going to be hard and this MR is a definite improvement. I think moving using Drupal in an event loop like frankenphp is going to make this part of the issue rear it's head again and I suggest that we file a follow-up to discuss solutions for that. We also need a follow-up to remove the unnecessary push in \Drupal\Core\Update\UpdateKernel::handle().

The kernel.terminate Event

Typical Purposes: To perform some "heavy" action after the response has been streamed to the user

Logging or tracing-related tasks generally are going to want the request object, but also qualify as "heavy" actions that should be performed after the response has been streamed. If terminate event listeners shouldn't have access to the request, then.. do we need another event that is aware of the request but is dispatched after the response is sent?

What is very interesting about this work is that I think it is wrong for our services like the route builder that runs in termination to need the request.

Indeed. Same for the session and currentUser. I feel that long(ish) running tasks should be created during the request-response cycle, but in a self-contained way (i.e., independent of state or stateful services). Like this they can be fired in the terminate event without access to the request context. And later also in a background fiber when a runloop becomes available.

So looking deeper into request stack usages after we should have popped the request stack - so for example we have:

• \Drupal\jsonapi\EventSubscriber\ResourceObjectNormalizationCacher::onTerminate() which calls \Drupal\jsonapi\EventSubscriber\ResourceObjectNormalizationCacher::set(), \Drupal\Core\Cache\Context\QueryArgsCacheContext::getContext(), \Drupal\Core\Cache\Context\SiteCacheContext::getContext(), \Drupal\Core\Cache\Context\CacheContextsManager::convertTokensToKeys() which all access the request stack.
• \Drupal\user\EventSubscriber\UserRequestSubscriber::onKernelTerminate() which accessing the request via the Time service - this will fallback but the fallback could prove problematic on long running requests.

I can see two options:

1. Push the request on the stack again in terminate
2. Implement our own RequestStack that continued to return the previous main / current request even if the stack is empty.

The advantage of option 1 is that it keeps the request stack consistent with the upstream logic. The advantage of option 2 is that http kernel middle in contrib / custom that relies on the request still been on the stack in between \Drupal\Core\StackMiddleware\KernelPreHandle::handle() and \Drupal\Core\DrupalKernel::terminate() will continue to work.

I guess there is a question whether these things should actually be happening in terminate.

My gut feel is that we should combine option 1 and 2. Then we can deprecate accessing the request outside \Drupal\Core\DrupalKernel::terminate() and \Drupal\Core\StackMiddleware\KernelPreHandle::handle() to the request stack post \Drupal\Core\StackMiddleware\KernelPreHandle::handle()

So from implementing a combination of option 1 and 2 I've discovered that \Drupal\page_cache\StackMiddleware\PageCache::handle() relies on being able to get the current route match based on the current request. This happens when it does the response policy check in \Drupal\page_cache\StackMiddleware\PageCache::storeResponse() - it eventually hits \Drupal\Core\PageCache\ResponsePolicy\DenyNoCacheRoutes::check(). Interesting and very tricky.

I think we have to accept that Drupal's expectations around the kernel and the request stack are different from Symfony's. And the way the current MR is working is the best we can do. It is the most honest. We expect the main request to persist until kernel terminate is done. I'm not sure what this means for FrankenPHP and event loop based code. My best guess is that the event loop code is going to have to manage the request stack a little and how and when it calls terminate.

Doing some more reading. #3313404: Use symfony/runtime for less bespoke bootstrap/compatibility with varied runtime environments says:

From Symfony's intention, a Kernel is really focused on handling a single request and response (and perhaps only its subresponses). This can be seen for example in Symfony's TerminableInterface that we implement. It should not handle larger application lifecycles.

This convinces me even more that the current approach this MR takes is the correct approach. It is odd to leave the main request on the stack and not pop it off at the same place we pop sub requests BUT this reflects Drupal's requirements and at least now at the end of index.php execution we have no requests on the stack.

Left a couple of observations on the tests on the MR. I think other than that, this is RTBC

Addressed the reviews - thanks everyone!

Looks good, will be great to resolve this after all this time - thanks Alex

This looks great. Comments nicely explain why we're doing it this way. Would be major surgery to have the request not available in ::destruct() given how much we rely on that for end of request tasks so glad we found a different way to handle things.

Did my best with issue credit but this is a long issue so may have overlooked people, apologies if so.

Committed/pushed to main and 11.x, thanks!