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

Files: 
CommentFileSizeAuthor
#76 request-2004086.patch17.1 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,049 pass(es).
[ View ]
#76 interdiff.txt1.25 KBdawehner
#71 request-2004086.patch16.96 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,933 pass(es).
[ View ]
#71 interdiff.txt755 bytesdawehner
#69 interdiff.txt1.95 KBdawehner
#69 request-2004086.patch17.69 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,085 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#66 request-2004086-66.patch16.98 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 10,630 pass(es), 728 fail(s), and 0 exception(s).
[ View ]

Comments

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.

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.

Status:Needs review» Reviewed & tested by the community

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

synthetic.patch queued for re-testing.

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

Committed b011332 and pushed to 8.x. Thanks!

Title:Stopgap fix for making the Request service syntheticThe 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

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

xpost

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

Back to active for doing the non-stopgap.

Title:The Request service syntheticThe Request service must be synthetic

Once more, with grammar.

Status:Active» Needs review
StatusFileSize
new3.47 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

StatusFileSize
new2.37 KB
new5.84 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: tests were executed, but no results were found.
[ View ]

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.

StatusFileSize
new1.9 KB
new7.06 KB
FAILED: [[SimpleTest]]: [MySQL] 55,324 pass(es), 156 fail(s), and 118 exception(s).
[ View ]

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new550 bytes
new7.6 KB
FAILED: [[SimpleTest]]: [MySQL] 55,354 pass(es), 156 fail(s), and 117 exception(s).
[ View ]

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.

StatusFileSize
new1.08 KB
new8.68 KB
FAILED: [[SimpleTest]]: [MySQL] 56,890 pass(es), 78 fail(s), and 32 exception(s).
[ View ]

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

StatusFileSize
new829 bytes
new9.49 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

similar fix needed for authorize.php

StatusFileSize
new735 bytes
new10.04 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

StatusFileSize
new753 bytes
new10.61 KB
FAILED: [[SimpleTest]]: [MySQL] 57,337 pass(es), 17 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.98 KB
new13.59 KB
FAILED: [[SimpleTest]]: [MySQL] 57,108 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

Fixes the entity_reference tests...

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.17 KB
new15.54 KB
FAILED: [[SimpleTest]]: [MySQL] 57,449 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new5.54 KB
new16.55 KB
PASSED: [[SimpleTest]]: [MySQL] 57,555 pass(es).
[ View ]

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

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?

Status:Needs work» Needs review
StatusFileSize
new2.98 KB
new12.18 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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 dependency on Request object.

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new3.24 KB
new11.9 KB
FAILED: [[SimpleTest]]: [MySQL] 58,208 pass(es), 17 fail(s), and 23 exception(s).
[ View ]
  • 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.

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

Status:Needs review» Needs work

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

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:

<?php
   
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:

<?php
   
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.;

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

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.

Status:Needs work» Needs review
StatusFileSize
new12.36 KB
FAILED: [[SimpleTest]]: [MySQL] 58,343 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

This could be it.

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new12.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch synthetic-2004086-42.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new12.64 KB
PASSED: [[SimpleTest]]: [MySQL] 59,043 pass(es).
[ View ]

Missed a git pull.

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.

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;
     }

Status:Needs review» Needs work

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

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

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.

StatusFileSize
new2.54 KB
new14.63 KB
FAILED: [[SimpleTest]]: [MySQL] 59,190 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review

Back to needs review.

Status:Needs review» Needs work

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

Assigned:effulgentsia» Unassigned

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

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.

Status:Needs work» Needs review
StatusFileSize
new16.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch synthetic-2004086-55_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled and fixed the tests.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new15.98 KB
FAILED: [[SimpleTest]]: [MySQL] 59,319 pass(es), 21 fail(s), and 2 exception(s).
[ View ]

Just a simple reroll.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new17.34 KB
FAILED: [[SimpleTest]]: [MySQL] 59,600 pass(es), 21 fail(s), and 0 exception(s).
[ View ]
new1.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.

Issue summary:View changes
StatusFileSize
new20.47 KB
FAILED: [[SimpleTest]]: [MySQL] 59,255 pass(es), 52 fail(s), and 0 exception(s).
[ View ]
new3.6 KB

Status:Needs work» Needs review

.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new3.35 KB
new22.4 KB
FAILED: [[SimpleTest]]: [MySQL] 58,690 pass(es), 49 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
Issue tags:+WSCCI
StatusFileSize
new16.98 KB
FAILED: [[SimpleTest]]: [MySQL] 10,630 pass(es), 728 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new17.69 KB
FAILED: [[SimpleTest]]: [MySQL] 58,085 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.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.

Status:Needs work» Needs review
StatusFileSize
new755 bytes
new16.96 KB
PASSED: [[SimpleTest]]: [MySQL] 57,933 pass(es).
[ View ]

It is green.

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

Status:Needs review» Reviewed & tested by the community

Excellent! @dawehner++

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?

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.25 KB
new17.1 KB
PASSED: [[SimpleTest]]: [MySQL] 59,049 pass(es).
[ View ]

Here is a quick "reroll".

+1 for that 'reroll'

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.