Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Symfony\Component\HttpKernel\HttpKernel::handleRaw()
pushes $request onto the stack andSymfony\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
Comment | File | Size | Author |
---|
Issue fork drupal-2613044
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:
Comments
Comment #2
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #3
effulgentsia CreditAttribution: effulgentsia at Acquia commentedBy 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.
Comment #4
dawehnerLet's see what happens.
Comment #6
dawehnerHa, wrong project, wrong patch.
Comment #8
Crell CreditAttribution: Crell at Palantir.net commentedThe 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.
Comment #9
Wim Leers+1 to everything in #8.
Comment #10
dawehnerI 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)Comment #11
cpj CreditAttribution: cpj commentedBy 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
Comment #12
cpj CreditAttribution: cpj commentedI'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.
Comment #13
dawehnerHere 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.
Comment #15
Wim LeersDrupalKernel::preHandle()
also needs to push$request
onto the stack. If that weren't necessary, removing it would solve all this at once?Super confusing issue.
Comment #16
dawehnerJust 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.
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
So we have a onion of middlewares:
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:
So here is an alternative approach. Let's see how this helps
More failures as? Patch number 1 which was the wrong patch? Or do you compare with #6 and use some more advanced math?
Comment #18
Wim LeersI 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 :)
Comment #19
dawehnerWell, its so simple. People relied on that behaviour over time.
Comment #20
cpj CreditAttribution: cpj commentedAs per #16, this patch is not intended as a final solution, but just to test what I said in #12
Comment #21
cpj CreditAttribution: cpj commentedComment #22
cpj CreditAttribution: cpj commentedI 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 ?
Comment #23
dawehner@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.
GRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRR I love symfony's idea of sharing code
Comment #24
cpj CreditAttribution: cpj commented@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 ?
Comment #25
dawehnerMh, 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,
Comment #26
dawehnerSo we do we agree that this is the behaviour we want?
Comment #28
cpj CreditAttribution: cpj commentedThis 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.
Comment #30
cpj CreditAttribution: cpj commentedFinally 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.
Comment #31
cpj CreditAttribution: cpj commentedComment #34
dawehner@cpj Any ideas about the remaining failures?
Comment #35
cpj CreditAttribution: cpj commented30 works fine for me against 8.1.x, so am resubmitting for testing
Comment #36
cpj CreditAttribution: cpj commentedComment #38
cpj CreditAttribution: cpj commented@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.
Comment #39
cpj CreditAttribution: cpj commentedAdded 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...
Comment #41
dawehnerreally nice progress!
Comment #42
cpj CreditAttribution: cpj commentedSo this may fix one tiny failure in RequestStackKernelTest, which needs to call the request stack middleware service rather than http_kernel.
Comment #44
cpj CreditAttribution: cpj commentedAnother attempt to fix one failure in RequestStackKernelTest. This time we let RequestStackMiddleware handle the request to push/pop correctly
Comment #46
cpj CreditAttribution: cpj commentedAfter 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.
Comment #47
cpj CreditAttribution: cpj commentedThis 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...
Comment #50
cpj CreditAttribution: cpj commentedA new approach which attempts to solve the following issues:
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
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.
Comment #51
cpj CreditAttribution: cpj commentedThis 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).
Comment #53
dawehnerHaha, 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.
Comment #54
dawehnerThat seems to be an unrelated change.
For documentation purposes I think we should try to answer what terminating the environment means. What is terminated ...
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 :)
Nice documentation!
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!
It is great to have test coverage here
Comment #55
cpj CreditAttribution: cpj commentedHi @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 ?
Comment #56
sinasalek CreditAttribution: sinasalek as a volunteer and at Practicalidea commentedHere is the meta issue #2218651: [meta] Make Drupal compatible with persistent app servers like ReactPHP, PHP-PM, PHPFastCGI, FrankenPHP, Swoole
Comment #57
cpj CreditAttribution: cpj commentedWith some help from @dawehner, I think I finally have a set of tests that should work - at least they pass phpunit locally
Comment #58
cpj CreditAttribution: cpj commentedSame patch after phpcbf
Comment #59
cpj CreditAttribution: cpj commentedRemoved redundant code from test.
Updated documentation as per #54
I think I'm done here.
Comment #60
dawehnerGreat work! Thank you @cpj!
We should explain more why we are doing things here.
Why can't we pass in the request stack as normal parameter in the constructor?
Comment #61
cpj CreditAttribution: cpj commentedHi @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.
Comment #62
almaudoh CreditAttribution: almaudoh commentedNice 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
andHttpKernelWrapper
should be moved to more appropriate namespaces - maybe\Drupal\Core
where theDrupalKernel
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 beRequestStackSubscriber
or something similar since this is no more stack middleware.4. Great test coverage!
Comment #63
cpj CreditAttribution: cpj commentedHi @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.
Comment #64
almaudoh CreditAttribution: almaudoh commented#63.2 Ok, thanks for clarifying.
The doc in RequestStackWrapper needs to be updated:
This should be adjusted to reflect the constructor injection now.
Comment #65
cpj CreditAttribution: cpj commentedThanks @almaudoh - I've updated the doc in RequestStackWrapper.
Comment #66
dawehnerI 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.
Comment #67
cpj CreditAttribution: cpj commentedHi @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"
Comment #68
cpj CreditAttribution: cpj commentedHi @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.
Comment #69
dawehnerI 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?
Comment #70
cpj CreditAttribution: cpj commentedOK @dawehner - is this version of RequestStackMiddlewareTest easier to understand ? I rearranged the code and added more comments.
Comment #71
dawehnerI personally think so. Thank you @cpj
Comment #72
cpj CreditAttribution: cpj commentedHi @dawehner - OK, thanks. Are we done here ?
Comment #74
dawehner@cpj
Can you add an issue summary to describe the possible solutions and why we went with the one in the patch?
Comment #75
fubhy CreditAttribution: fubhy commentedI 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
Comment #77
Anonymous (not verified) CreditAttribution: Anonymous commented8.4 is already out. 8.5 is closing in. What's the current state?
Comment #78
Wim LeersBug 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! :)
Comment #80
martin107 CreditAttribution: martin107 as a volunteer commentedComment #81
martin107 CreditAttribution: martin107 as a volunteer commentedNeeded a reroll.
Comment #82
andypostDocs clean-up, btw why not add this "poping" in
\Drupal\Core\EventSubscriber\KernelDestructionSubscriber::getSubscribedEvents()
this one also supposed to run lasts/Pop/Pops
needs doc block
should be inherited
Comment #83
martin107 CreditAttribution: martin107 as a volunteer commentedThe smallest of changes
array() becomes [].
Comment #84
andypostIt just needs to update summay why another subscriber used and that is what not clear to me in #82 about KernelDestructionSubscriber
Comment #85
mglamanThis 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.
So we need to communicate that into the issue summary, then?
Comment #89
andypostReroll for 8.9
Comment #90
andypostSo this one and related the only blocker remaining
Updated issue summary a bit
Comment #92
andypostPatch fixes test but failures in views looks valid, the last one is about related #2962157: TestSiteApplicationTest requires a database despite being a unit test
Comment #94
andypostDebugged last failed test and the issue is that now subrequest in
\Drupal\Tests\taxonomy\Kernel\Views\TaxonomyDefaultArgumentTest::initViewWithRequest()
does not populate route match parametersWhich is caused by subrequest returns 404 for
/node/1
Comment #96
andypostRoute rebuild no longer needed
Comment #97
andypostComment #101
andypostre-roll for 9.3
Comment #103
andypostFix usage of deprecated
getMasterRequest()
Comment #105
andypostComment #106
joachim CreditAttribution: joachim as a volunteer commentedCould this be the cause of #3218022: Running an HTTP subrequest leaves the request stack in an incorrect state?
Comment #108
andypostLooks 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
Comment #111
andypostfew TODO added in #2484991: Add the session to the request in KernelTestBase, BrowserTestBase, and drush
Comment #115
volegerRerolled.
Changes affected by CR Drupal\Core\Http\RequestStack is deprecated
Created MR
Comment #116
andypostThank 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
Comment #117
andypostFixed tests after https://www.drupal.org/node/3357408
Comment #118
andypost2 tests still fail