Problem/Motivation

  • Symfony\Component\HttpKernel\HttpKernel::handleRaw() pushes $request onto the stack and Symfony\Component\HttpKernel\HttpKernel::finishRequest() pops it.
  • DrupalKernel::preHandle() also pushes $request onto the stack, but there's no matching pop.
  • As a result, at the conclusion of a subrequest, the stack still contains the subrequest. One consequence of this is that on a comment permalink page (e.g., /comment/1), drupalSettings.path.currentPath is 'node/1' instead of 'comment/1'. I don't know if there are other, more important, consequences.

Proposed resolution

Introduce RequestStackWrapper decorator for request_stack and pass it to http_kernel.basic instead of symfovy's one to be able to remove it in additional subscriber StackMiddlewareSubscriber runnig after KernelDestructionSubscriber

Sum-up in #2613044-85: Requests are pushed onto the request stack twice, popped once

Remaining tasks

Get RTBC and commit

User interface changes

no

API changes

2 new services for core: decorator of request stack and stack clean-up after kernel destroyed

Data model changes

CommentFileSizeAuthor
#103 2613044-103.patch11.15 KBandypost
#103 interdiff.txt3.66 KBandypost
#101 2613044-101.patch11.05 KBandypost
#96 2613044-96.patch11.06 KBandypost
#96 interdiff.txt747 bytesandypost
#92 2613044-92.patch11.21 KBandypost
#92 interdiff.txt1.1 KBandypost
#89 2613044-83-89x.patch11.3 KBandypost
#83 intediff-2613044-82-83.txt564 bytesmartin107
#83 2613044-83.patch10.97 KBmartin107
#82 2613044-82.patch10.98 KBandypost
#82 interdiff-2613044-81.txt7.53 KBandypost
#81 2613044-81.patch10.61 KBmartin107
#70 2613044-70.patch10.57 KBcpj
#68 2613044-68.patch10.27 KBcpj
#65 2613044-65.patch11.87 KBcpj
#61 2613044-61.patch11.87 KBcpj
#59 2613044-59.patch10.64 KBcpj
#58 2613044-58.patch10.55 KBcpj
#57 2613044-57.patch10.53 KBcpj
#51 2613044-51.patch9.29 KBcpj
#50 2613044-50.patch7.75 KBcpj
#47 2613044-47.patch7.57 KBcpj
#44 2613044-44.patch6.56 KBcpj
#42 2613044-42.patch6.37 KBcpj
#39 2613044-39.patch6.25 KBcpj
#35 2613044-35.patch5.9 KBcpj
#30 2613044-30.patch5.9 KBcpj
#28 2613044-28.patch6.15 KBcpj
#26 2613044-26.patch1.45 KBdawehner
#20 2613044-20.patch8.2 KBcpj
#16 2613044-16.patch3.14 KBdawehner
#13 2613044-13.patch2.3 KBdawehner
#6 2613044-6.patch1.84 KBdawehner
#4 2613172-4.patch15.39 KBdawehner

Issue fork drupal-2613044

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia created an issue. See original summary.

effulgentsia’s picture

Issue summary: View changes
effulgentsia’s picture

By the way, I initially thought #2613040: The "Permalink" comment link links to the non-permanent URL might be due to this, but it's not. That's a separate issue. But the drupalSettings problem that's in the issue summary is due to this issue, and not that one.

dawehner’s picture

Status: Active » Needs review
FileSize
15.39 KB

Let's see what happens.

Status: Needs review » Needs work

The last submitted patch, 4: 2613172-4.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.84 KB

Ha, wrong project, wrong patch.

Status: Needs review » Needs work

The last submitted patch, 6: 2613044-6.patch, failed testing.

Crell’s picture

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.

Wim Leers’s picture

+1 to everything in #8.

dawehner’s picture

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)

cpj’s picture

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

cpj’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.3 KB

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.

Status: Needs review » Needs work

The last submitted patch, 13: 2613044-13.patch, failed testing.

Wim Leers’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.14 KB

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?

Status: Needs review » Needs work

The last submitted patch, 16: 2613044-16.patch, failed testing.

Wim Leers’s picture

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 :)

dawehner’s picture

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.

cpj’s picture

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

cpj’s picture

Status: Needs work » Needs review
cpj’s picture

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 ?

dawehner’s picture

@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

cpj’s picture

@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 ?

dawehner’s picture

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,

dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, 26: 2613044-26.patch, failed testing.

cpj’s picture

Status: Needs work » Needs review
FileSize
6.15 KB

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.

Status: Needs review » Needs work

The last submitted patch, 28: 2613044-28.patch, failed testing.

cpj’s picture

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’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 30: 2613044-30.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

@cpj Any ideas about the remaining failures?

cpj’s picture

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

cpj’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 35: 2613044-35.patch, failed testing.

cpj’s picture

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

cpj’s picture

Status: Needs work » Needs review
FileSize
6.25 KB

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

Status: Needs review » Needs work

The last submitted patch, 39: 2613044-39.patch, failed testing.

dawehner’s picture

really nice progress!

cpj’s picture

Status: Needs work » Needs review
FileSize
6.37 KB

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

Status: Needs review » Needs work

The last submitted patch, 42: 2613044-42.patch, failed testing.

cpj’s picture

Status: Needs work » Needs review
FileSize
6.56 KB

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

Status: Needs review » Needs work

The last submitted patch, 44: 2613044-44.patch, failed testing.

cpj’s picture

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.

cpj’s picture

Status: Needs work » Needs review
FileSize
7.57 KB

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

Status: Needs review » Needs work

The last submitted patch, 47: 2613044-47.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cpj’s picture

Status: Needs work » Needs review
FileSize
7.75 KB

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.

cpj’s picture

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

Status: Needs review » Needs work

The last submitted patch, 51: 2613044-51.patch, failed testing.

dawehner’s picture

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.

dawehner’s picture

  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

cpj’s picture

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 ?

sinasalek’s picture

cpj’s picture

Status: Needs work » Needs review
FileSize
10.53 KB

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

cpj’s picture

Same patch after phpcbf

cpj’s picture

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

dawehner’s picture

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?

cpj’s picture

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.

almaudoh’s picture

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!

cpj’s picture

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.

almaudoh’s picture

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

cpj’s picture

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

dawehner’s picture

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

cpj’s picture

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"

cpj’s picture

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.

dawehner’s picture

+++ 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?

cpj’s picture

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

dawehner’s picture

I personally think so. Thank you @cpj

cpj’s picture

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

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

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

fubhy’s picture

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Anonymous’s picture

Version: 8.4.x-dev » 8.5.x-dev

8.4 is already out. 8.5 is closing in. What's the current state?

Wim Leers’s picture

Version: 8.5.x-dev » 8.4.x-dev

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! :)

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

martin107’s picture

Version: 8.5.x-dev » 8.6.x-dev
martin107’s picture

Status: Needs work » Needs review
FileSize
10.61 KB

Needed a reroll.

andypost’s picture

FileSize
7.53 KB
10.98 KB

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

martin107’s picture

The smallest of changes

array() becomes [].

andypost’s picture

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

mglaman’s picture

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?

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

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 }
andypost’s picture

So this one and related the only blocker remaining

Updated issue summary a bit

Status: Needs review » Needs work

The last submitted patch, 89: 2613044-83-89x.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

Status: Needs review » Needs work

The last submitted patch, 92: 2613044-92.patch, failed testing. View results

andypost’s picture

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

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

Route rebuild no longer needed

andypost’s picture

Issue tags: +Bug Smash Initiative

Status: Needs review » Needs work

The last submitted patch, 96: 2613044-96.patch, failed testing. View results

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

Status: Needs review » Needs work

The last submitted patch, 101: 2613044-101.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
FileSize
3.66 KB
11.15 KB

Fix usage of deprecated getMasterRequest()

Status: Needs review » Needs work

The last submitted patch, 103: 2613044-103.patch, failed testing. View results

andypost’s picture

joachim’s picture

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

voleger made their first commit to this issue’s fork.

voleger’s picture

Status: Needs work » Needs review

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

andypost’s picture

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

andypost’s picture

Status: Needs work » Needs review
andypost’s picture

Status: Needs review » Needs work

2 tests still fail