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

CommentFileSizeAuthor
#76 request-2004086.patch17.1 KBdawehner
#76 interdiff.txt1.25 KBdawehner
#71 request-2004086.patch16.96 KBdawehner
#71 interdiff.txt755 bytesdawehner
#69 interdiff.txt1.95 KBdawehner
#69 request-2004086.patch17.69 KBdawehner
#66 request-2004086-66.patch16.98 KBtim.plunkett
#64 synthetic-2004086-64.patch22.4 KBdawehner
#64 interdiff.txt3.35 KBdawehner
#61 interdiff-synthetic.txt3.6 KBdawehner
#61 synthetic-2004086-61.patch20.47 KBdawehner
#59 interdiff.txt1.64 KBdawehner
#59 synthetic-2004086-59.patch17.34 KBdawehner
#57 synthetic-2004086-57.patch15.98 KBdawehner
#55 synthetic-2004086-55.patch16.11 KBdawehner
#50 synthetic-2004086-50.patch14.63 KBdawehner
#50 interdiff.txt2.54 KBdawehner
#44 synthetic-2004086-44.patch12.64 KBdawehner
#42 synthetic-2004086-42.patch12.66 KBdawehner
#37 request_synthetic-2004086-37.patch12.36 KBdawehner
#31 request-2004086-31.patch11.9 KBdawehner
#31 interdiff.txt3.24 KBdawehner
#28 2004086-request-service-synthetic-28.patch12.18 KBvijaycs85
#28 2004086-diff-26-28.txt2.98 KBvijaycs85
#26 2004086-26.patch16.55 KBpwolanin
#26 2004086-24-26.increment.txt5.54 KBpwolanin
#24 2004086.synthetic-request.24.patch15.54 KBkatbailey
#24 interdiff.txt2.17 KBkatbailey
#22 2004086.synthetic-request.21.patch13.59 KBkatbailey
#22 interdiff.txt2.98 KBkatbailey
#20 2004086-20.patch10.61 KBpwolanin
#20 2004086-19-20.increment.txt753 bytespwolanin
#19 2004086-19.patch10.04 KBpwolanin
#19 2004086-18-19.increment.txt735 bytespwolanin
#18 2004086-18.patch9.49 KBpwolanin
#18 2004086-17-18.increment.txt829 bytespwolanin
#17 2004086-17.patch8.68 KBpwolanin
#17 2004086-15-17.increment.txt1.08 KBpwolanin
#16 2004086.synthetic-request.15.patch7.6 KBkatbailey
#16 interdiff.txt550 byteskatbailey
#13 2004086-13.patch7.06 KBpwolanin
#13 2004086-11-13.increment.txt1.9 KBpwolanin
#11 2004086-11.patch5.84 KBpwolanin
#11 2004086-10-11.increment.txt2.37 KBpwolanin
#10 2004086-10.patch3.47 KBpwolanin
synthetic.patch723 bytesfabpot
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Status: Needs review » Reviewed & tested by the community

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

effulgentsia’s picture

Priority: Normal » Critical
Status: Reviewed & tested by the community » Needs review

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

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

#2 was xpost. I'm ok with the OP as a stopgap if others are.

ParisLiakos’s picture

synthetic.patch queued for re-testing.

alexpott’s picture

Title: The Request service must be synthetic » Stopgap fix for making the Request service synthetic
Status: Reviewed & tested by the community » Fixed

Committed b011332 and pushed to 8.x. Thanks!

jenlampton’s picture

Title: Stopgap fix for making the Request service synthetic » The Request service must be synthetic
Status: Fixed » Reviewed & tested by the community

Confirmed that the patch in this issue also solves #1929812: Fatal error on web-based install

alexpott’s picture

Title: The Request service must be synthetic » Stopgap fix for making the Request service synthetic
Status: Reviewed & tested by the community » Fixed

xpost

effulgentsia’s picture

Title: Stopgap fix for making the Request service synthetic » The Request service synthetic
Assigned: fabpot » effulgentsia
Status: Fixed » Active

Back to active for doing the non-stopgap.

effulgentsia’s picture

Title: The Request service synthetic » The Request service must be synthetic

Once more, with grammar.

pwolanin’s picture

Status: Active » Needs review
FileSize
3.47 KB

combining the change suggested above with the patch from https://drupal.org/node/1929812#comment-7530369

pwolanin’s picture

Based on local testing, that patch is going to fail. Here's one that lets the tests run.

Status: Needs review » Needs work

The last submitted patch, 2004086-11.patch, failed testing.

pwolanin’s picture

This is still not working, but isn't an immediate fatal.

katbailey’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2004086-13.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
550 bytes
7.6 KB

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

pwolanin’s picture

Re-ordering the code in update.php so we set the request before initializing the session solves a lot of the fails.

pwolanin’s picture

similar fix needed for authorize.php

pwolanin’s picture

FileSize
735 bytes
10.04 KB

Fix bootstrap - we have to set the request as soon as the kernel is booted since it may be needed in the rest of bootstrap.

pwolanin’s picture

FileSize
753 bytes
10.61 KB

This 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

Status: Needs review » Needs work

The last submitted patch, 2004086-20.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
2.98 KB
13.59 KB

Fixes the entity_reference tests...

Status: Needs review » Needs work

The last submitted patch, 2004086.synthetic-request.21.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
15.54 KB

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

Damien Tournoud’s picture

Status: Needs review » Needs work
--- a/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
+    $request = Request::createFromGlobals();
+    $request->attributes->set('account', $GLOBALS['user']);
+    $this->container->set('request', $request);

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

pwolanin’s picture

Status: Needs work » Needs review
FileSize
5.54 KB
16.55 KB

This converts to _account and creates just a dummy request from the '/' path. Let's see how it tests.

effulgentsia’s picture

Status: Needs review » Needs work
+++ b/core/authorize.php
@@ -68,7 +70,9 @@ function authorize_access_allowed() {
+// A request object from the HTTPFoundation to tell us about the request.
+$request = Request::createFromGlobals();
+Drupal::getContainer()->set('request', $request);

This patch now has _drupal_bootstrap_kernel() doing this, so this can be removed from here.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
@@ -183,7 +184,8 @@ public function containerBuild(ContainerBuilder $container) {
+    $request = Request::createFromGlobals();

Shouldn't this one also use create('/') instead of leaking globals?

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -992,16 +995,20 @@ protected function prepareConfigDirectories() {
+    // The global $user object was replaced in TestBase::prepareEnvironment().

I don't get what this comment is trying to tell me.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
@@ -869,8 +870,13 @@ public function getHandlers($type) {
-        if (\Drupal::request()->request->get('form_id') && isset($this->view->temporary_options[$type][$id])) {
-          $info = $this->view->temporary_options[$type][$id];
+        try {
+          $request = \Drupal::request();
+          if ($request->get('form_id') && isset($this->view->temporary_options[$type][$id])) {
+            $info = $this->view->temporary_options[$type][$id];
+          }
+        }
+        catch (DependencyInjectionRuntimeException $e) {
         }

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?

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.98 KB
12.18 KB

Re-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()

Status: Needs review » Needs work

The last submitted patch, 2004086-request-service-synthetic-28.patch, failed testing.

pwolanin’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.24 KB
11.9 KB
  • Fixed some small changes which made the patch bigger than needed.
  • If you look at isScopeActive there is no need to check whether the scope existing before.
  • Rewrote the bits in TestBase to just persist the request over a container rebuild.
    There is the persist tag which would do that for us. Does anyone know whether it would fit here?
  • The change in DisplayPluginBase switched for $request->request->get() to $request->get() so this is corrected as well.
dawehner’s picture

#31: request-2004086-31.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, request-2004086-31.patch, failed testing.

dawehner’s picture

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

    protected function getCsrfTokenService()
    {
        $this->services['csrf_token'] = $instance = new \Drupal\Core\Access\CsrfTokenGenerator($this->get('private_key'));

        if ($this->has('request')) {
            $instance->setRequest($this->get('request', ContainerInterface::NULL_ON_INVALID_REFERENCE));
        }

        return $instance;
    }

so $this->get('request') calls the following code:

    protected function getRequestService()
    {
        throw new RuntimeException('You have requested a synthetic service ("request"). The DIC does not know how to construct this service.');
    }

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

[11]
[5]
/var/www/d8/sites/default/files/php/service_container/service_container_prod_simpletest994200.php/7040f3c9086e9a4efa30eba9bc6e112689385bffae1944f6d8a90bd6ac827bbf.php
2438
getRequestService
service_container_prod_simpletest994200
::
[7]
/var/www/d8/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Container.php
298
getRequestService
service_container_prod_simpletest994200
[10]
->
[0]
[7]
/var/www/d8/sites/default/files/php/service_container/service_container_prod_simpletest994200.php/7040f3c9086e9a4efa30eba9bc6e112689385bffae1944f6d8a90bd6ac827bbf.php
1133
get
Symfony\Component\DependencyInjection\Container
[10]
->
[2]
[7]
/var/www/d8/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/Container.php
298
getCsrfTokenService
service_container_prod_simpletest994200
[10]
->
[0]
[7]
/var/www/d8/core/lib/Drupal.php
401
get
Symfony\Component\DependencyInjection\Container
[10]
->
[1]
[6]
/var/www/d8/core/includes/common.inc
3063
csrfToken
Drupal
::
[0]
[4]
/var/www/d8/core/includes/common.inc
2358
drupal_get_token
[1]
[4]
/var/www/d8/core/lib/Drupal/Core/Ajax/AjaxResponse.php
114
drupal_get_js
[4]
[7]
/var/www/d8/core/lib/Drupal/Core/Ajax/AjaxResponse.php
58
ajaxRender
Drupal\Core\Ajax\AjaxResponse
[10]
->
[1]
[7]
/var/www/d8/core/includes/bootstrap.inc
1880
prepare
Drupal\Core\Ajax\AjaxResponse
[10]
->
[1]
[4]

tl;dr The solution was in the initial sentence ... no need for scrolling.;

Damien Tournoud’s picture

      if ($this->has('request')) {
            $instance->setRequest($this->get('request', ContainerInterface::NULL_ON_INVALID_REFERENCE));
        }

This by itself looks wrong. The request should not be optional for the CSRF Token service. I assume our configuration is wrong.

dawehner’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.36 KB

This could be it.

tim.plunkett’s picture

Interdiff

diff --git a/core/lib/Drupal/Core/HttpKernel.php b/core/lib/Drupal/Core/HttpKernel.php
index 07c79c7..9e19200 100644
--- a/core/lib/Drupal/Core/HttpKernel.php
+++ b/core/lib/Drupal/Core/HttpKernel.php
@@ -57,6 +57,7 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
         }
 
         $this->container->leaveScope('request');
+        $this->container->set('request', $request);
 
         return $response;
     }

Status: Needs review » Needs work

The last submitted patch, request_synthetic-2004086-37.patch, failed testing.

Crell’s picture

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

dawehner’s picture

Issue tags: +WSCCI

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

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.66 KB

Rerolled and removed the scope: request from the current_user service.

Status: Needs review » Needs work

The last submitted patch, synthetic-2004086-42.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.64 KB

Missed a git pull.

Crell’s picture

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

fabpot’s picture

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

diff --git a/core/lib/Drupal/Core/HttpKernel.php b/core/lib/Drupal/Core/HttpKernel.php
index 07c79c7..9e19200 100644
--- a/core/lib/Drupal/Core/HttpKernel.php
+++ b/core/lib/Drupal/Core/HttpKernel.php
@@ -57,6 +57,7 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
         }
 
         $this->container->leaveScope('request');
+        $this->container->set('request', $request);
 
         return $response;
     }

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

diff --git a/core/lib/Drupal/Core/HttpKernel.php b/core/lib/Drupal/Core/HttpKernel.php
index 07c79c7..b51f74a 100644
--- a/core/lib/Drupal/Core/HttpKernel.php
+++ b/core/lib/Drupal/Core/HttpKernel.php
@@ -52,11 +52,13 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
             $response = parent::handle($request, $type, $catch);
         } catch (\Exception $e) {
             $this->container->leaveScope('request');
+            $this->container->set('request', $request);

             throw $e;
         }

         $this->container->leaveScope('request');

         return $response;
     }
webchick’s picture

Status: Needs review » Needs work

That sounds like a "needs work." Thanks for the feedback, Fabien! :)

dawehner’s picture

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.

Thanks 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

fabpot’s picture

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

dawehner’s picture

FileSize
2.54 KB
14.63 KB

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

dawehner’s picture

Status: Needs work » Needs review

Back to needs review.

Status: Needs review » Needs work

The last submitted patch, synthetic-2004086-50.patch, failed testing.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned

You all have been doing a great job driving this issue. No need to have it still assigned to me :)

fabpot’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.11 KB

Rerolled and fixed the tests.

Status: Needs review » Needs work

The last submitted patch, synthetic-2004086-55.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
15.98 KB

Just a simple reroll.

Status: Needs review » Needs work

The last submitted patch, synthetic-2004086-57.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.34 KB
1.64 KB

This just fixes some of them.

Status: Needs review » Needs work
Issue tags: -WSCCI

The last submitted patch, synthetic-2004086-59.patch, failed testing.

dawehner’s picture

Issue summary: View changes
FileSize
20.47 KB
3.6 KB
dawehner’s picture

Status: Needs work » Needs review

.

Status: Needs review » Needs work

The last submitted patch, 61: synthetic-2004086-61.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.35 KB
22.4 KB

Status: Needs review » Needs work

The last submitted patch, 64: synthetic-2004086-64.patch, failed testing.

tim.plunkett’s picture

Status: Needs review » Needs work

The last submitted patch, 66: request-2004086-66.patch, failed testing.

Crell’s picture

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -46,17 +47,25 @@ public function addCommand($command, $prepend = FALSE) {
+   * Sets the rendered ajax ajax right before the response is prepared

This is redundant redundant.

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.69 KB
1.95 KB

Let's see how many of the tests failing before now work after _account.

Status: Needs review » Needs work

The last submitted patch, 69: request-2004086.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
755 bytes
16.96 KB

It is green.

Status: Needs review » Needs work

The last submitted patch, 71: request-2004086.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

71: request-2004086.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Excellent! @dawehner++

catch’s picture

Status: Reviewed & tested by the community » Needs work

I wasn't expecting to see this RTBC for ages, nice work!

Very minor nitpicks - please move straight back to RTBC after re-roll.

  1. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
    @@ -46,17 +47,25 @@ public function addCommand($command, $prepend = FALSE) {
    +   * Sets the rendered ajax right before the response is prepared
    

    s/ajax/AJAX and missing period at the end of the sentence.

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/AjaxResponseSubscriber.php
    @@ -0,0 +1,39 @@
    +   * @
    

    Should this be inheritdoc?

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.25 KB
17.1 KB

Here is a quick "reroll".

damiankloip’s picture

+1 for that 'reroll'

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed the 'reroll' to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.