Problem/Motivation
The Request should be marked as synthetic in the DIC. It's not possible for reasons explained below, but should be.
The drupal_session_initialize()
function and all other related session functions need access to the Request object. Unfortunately, the session handling is managed before the Request is available. So, when the session calls Drupal::request(), and because the synthetic setting is not set, Symfony is creating an instance of the Request class. This object is created via the constructor, and as such is totally wrong and not based on the real current Request.
Proposed resolution
The synthetic setting has been added and commented with a @TODO so that we can remember to uncomment it at some point and before the final release at the latest.
In the meantime, the best way to get the real Request object early on is to tweak the Request setting by forcing the Symfony DIC to actually use the createFromGlobals()
method when it creates the Request. This solution is temporary and must be removed whenever the synthetic setting is set to true (the @TODO mentioned that as well).
Comment | File | Size | Author |
---|---|---|---|
#76 | request-2004086.patch | 17.1 KB | dawehner |
#76 | interdiff.txt | 1.25 KB | dawehner |
#71 | request-2004086.patch | 16.96 KB | dawehner |
#71 | interdiff.txt | 755 bytes | dawehner |
#69 | interdiff.txt | 1.95 KB | dawehner |
Comments
Comment #1
Crell CreditAttribution: Crell commentedWe discussed this stopgap at DrupalCon Portland, and it seems reasonable. We still need to actually fix it, but that will take a bit more work.
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedSee also #1929812-14: Fatal error on web-based install. The patch above does fix the problem of Drupal::request() returning a completely bogus request object, but only by making the assumption that Request::createFromGlobals() is a better object to use when one hasn't been explicitly set. Which it is in the sense that getClientIp() and isSecure() will return more correct results, but that just ends up covering up the real problem.
The real solution here is to make it synthetic. I'm gonna give that a try, and see how far the rabbit hole goes.
Also, bumping this to critical, because we currently have code getting incorrect results for isSecure() among other problems.
Comment #3
effulgentsia CreditAttribution: effulgentsia commented#2 was xpost. I'm ok with the OP as a stopgap if others are.
Comment #4
ParisLiakos CreditAttribution: ParisLiakos commentedsynthetic.patch queued for re-testing.
Comment #5
alexpottCommitted b011332 and pushed to 8.x. Thanks!
Comment #6
jenlamptonConfirmed that the patch in this issue also solves #1929812: Fatal error on web-based install
Comment #7
alexpottxpost
Comment #8
effulgentsia CreditAttribution: effulgentsia commentedBack to active for doing the non-stopgap.
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedOnce more, with grammar.
Comment #10
pwolanin CreditAttribution: pwolanin commentedcombining the change suggested above with the patch from https://drupal.org/node/1929812#comment-7530369
Comment #11
pwolanin CreditAttribution: pwolanin commentedBased on local testing, that patch is going to fail. Here's one that lets the tests run.
Comment #13
pwolanin CreditAttribution: pwolanin commentedThis is still not working, but isn't an immediate fatal.
Comment #14
katbailey CreditAttribution: katbailey commentedComment #16
katbailey CreditAttribution: katbailey commentedThe wrong exception is being caught in watchdog - when the request is not synthetic, no exception gets thrown here at all because the container just instantiates a new request.
Comment #17
pwolanin CreditAttribution: pwolanin commentedRe-ordering the code in update.php so we set the request before initializing the session solves a lot of the fails.
Comment #18
pwolanin CreditAttribution: pwolanin commentedsimilar fix needed for authorize.php
Comment #19
pwolanin CreditAttribution: pwolanin commentedFix bootstrap - we have to set the request as soon as the kernel is booted since it may be needed in the rest of bootstrap.
Comment #20
pwolanin CreditAttribution: pwolanin commentedThis sets the request in _drupal_bootstrap_kernel() which is called in places like statistics.php that don't handle the request in the normal way. This fixes StatisticsLoggingTest
Comment #22
katbailey CreditAttribution: katbailey commentedFixes the entity_reference tests...
Comment #24
katbailey CreditAttribution: katbailey commentedTHe FilterHtmlImageSecureTest passes for me locally both via the UI and using run-tests.sh. I've added fixes for the other two problems, hoping the fix for CronRunTest doesn't break anything else...
Comment #25
Damien Tournoud CreditAttribution: Damien Tournoud commentedNo request created from the global environment in the test harness, please. This is just an violation of the encapsulation. If you really have to, create a blank request, but I would prefer fixing the code elsewhere that assume that a request is always present.
Comment #26
pwolanin CreditAttribution: pwolanin commentedThis converts to _account and creates just a dummy request from the '/' path. Let's see how it tests.
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedThis patch now has _drupal_bootstrap_kernel() doing this, so this can be removed from here.
Shouldn't this one also use create('/') instead of leaking globals?
I don't get what this comment is trying to tell me.
Not the fault of this issue, but this makes it very clear that it's bad for a method this low-level to depend on $request. Can we have a follow up issue to find a way to remove that dependency, and add a @todo here linking to that?
Comment #28
vijaycs85Re-rolled and fixed below in #27
#27.1 FIXED: This patch now has _drupal_bootstrap_kernel() doing this, so this can be removed from here.
#27.2 FIXED: Shouldn't this one also use create('/') instead of leaking globals?
#27.3 FIXED: I don't get what this comment is trying to tell me.
#27.4 FIXED: Can we have a follow up issue to find a way to remove that dependency, and add a @todo here linking to that? - #2059003: Remove try/catch around getRequest() in DisplayPluginBase::getHandlers()
Comment #30
pwolanin CreditAttribution: pwolanin commented"exception 'Symfony\Component\DependencyInjection\Exception\RuntimeException' with message 'You have requested a synthetic service ("request"). The DIC does not know how to construct this service.'"
Comment #31
dawehnerThere is the persist tag which would do that for us. Does anyone know whether it would fit here?
Comment #32
dawehner#31: request-2004086-31.patch queued for re-testing.
Comment #34
dawehnerThe problems is the following backtrace. What you can see there is that drupal_get_js is calling drupal_get_token which is a service which needs the request.
The compiled container looks like the following code:
so $this->get('request') calls the following code:
which is not not caught by $this->get('request')
I tried to pull latest symfony, so https://github.com/symfony/symfony/pull/8392 is included but this did not helped.
Neither did adding 'scope': request nor 'synchronized: true on the request service. (which is what symfony fullstack does).
tl;dr The solution was in the initial sentence ... no need for scrolling.;
Comment #35
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis by itself looks wrong. The request should not be optional for the CSRF Token service. I assume our configuration is wrong.
Comment #36
dawehnerWell, even if you think it is not optional, this even more does not solve the problem that you don't have the request available.
If it is optional this code could at least theoretically work.
Comment #37
dawehnerThis could be it.
Comment #38
tim.plunkettInterdiff
Comment #40
Crell CreditAttribution: Crell commentedWe are going to be removing the Drupal\HttpKernel class. Don't rely on it. :-) Symfony\HttpKernel's handle() method is handling the container properly; you can rely on that, I'm fairly certain.
Comment #41
dawehnerWell, the problem is actually the drupal httpkernel. In constrast to the one from symfony we do create request scopes, which symfony does not do anymore, probably because they have synched services now.
Comment #42
dawehnerRerolled and removed the scope: request from the current_user service.
Comment #44
dawehnerMissed a git pull.
Comment #45
Crell CreditAttribution: Crell commentedMy Dreditor is misbehaving at the moment, but the patch itself looks fine to me. Not RTBCing because I didn't give it a line by line review but it seems OK from what I saw.
Comment #46
fabpot CreditAttribution: fabpot commentedThis patch seems to try to keep the request instance as much as possible in the container by setting it in a large number of places, and it looks wrong to me.
I don't know Drupal well enough to be able to comment all changes, but this one is definitely wrong (philosophically at least ;)):
After leaving the request scope, the request must be null in the container as we are not handling a request anymore. The code depending on a request should be able to deal with a request object or null (happens before starting to handle a request and after the request is handled). Or put another way, when not handling a request, there is no request.
If this is just a temporary hack, no worries, ... in which case, you should probably add some more setters, like here (as the exception might be caught somewhere):
Comment #47
webchickThat sounds like a "needs work." Thanks for the feedback, Fabien! :)
Comment #48
dawehnerThanks for the review, highly appreciate!
This change was due to a service called "csrf_generator" which depends on the request and generates a failure when the service was instantiated first after the request scope.
I tried to collect my research on https://drupal.org/node/2004086#comment-7766809
Comment #49
fabpot CreditAttribution: fabpot commentedCSRF management should be tightly coupled with the handling of a request as the CSRF protection only makes sense for a given request. So, at the time some code gets the CSRF service, the request service should be not null. If that's not the case, it probably means that the code that first initiates the CSRF is doing that too early.
Comment #50
dawehnerThis leads me towards an idea:
This moves calling the preparation of the response into a kernel::response subscriber. At least some custom checked ajax still works.
Comment #51
dawehnerBack to needs review.
Comment #53
effulgentsia CreditAttribution: effulgentsia commentedYou all have been doing a great job driving this issue. No need to have it still assigned to me :)
Comment #54
fabpot CreditAttribution: fabpot commentedAll the
Drupal::getContainer()->set('request', $request);
code should be removed. If that's not possible, it means that there are some other cases where the request is expected to be available outside the handling of a request.Comment #55
dawehnerRerolled and fixed the tests.
Comment #57
dawehnerJust a simple reroll.
Comment #59
dawehnerThis just fixes some of them.
Comment #61
dawehnerComment #62
dawehner.
Comment #64
dawehnerComment #66
tim.plunkettRerolled after #2073531: Use current user service instead of _account, remove _account from the request object. No changes.
Comment #68
Crell CreditAttribution: Crell commentedThis is redundant redundant.
Comment #69
dawehnerLet's see how many of the tests failing before now work after _account.
Comment #71
dawehnerIt is green.
Comment #73
dawehner71: request-2004086.patch queued for re-testing.
Comment #74
tim.plunkettExcellent! @dawehner++
Comment #75
catchI wasn't expecting to see this RTBC for ages, nice work!
Very minor nitpicks - please move straight back to RTBC after re-roll.
s/ajax/AJAX and missing period at the end of the sentence.
Should this be inheritdoc?
Comment #76
dawehnerHere is a quick "reroll".
Comment #77
damiankloip CreditAttribution: damiankloip commented+1 for that 'reroll'
Comment #78
catchCommitted/pushed the 'reroll' to 8.x, thanks!
Comment #80
YesCT CreditAttribution: YesCT commented#1907750: Symfony update exposes fatal error during installation due to 'access_manager' service depending on 'request' was postponed on this, so opened that one back up.