Remove the request from the container

Not 100% sure but I think this is very similar to #2223189: Use regular upstream HttpKernel instead of Drupal's custom ?

I like this issue. I don't think we should have both the request and request_stack services defined - it is both confusing and takes code to maintain. However, this issue should not be beta blocking - it does not meet the criteria. However we should aim to get this done as soon as possible. It is critical as I don't think we should release Drupal 8 without this being fixed.

This patch breaks the Simpletest UI, fyi.
Was taking a look at the fails and can't get any tests to run via the UI, all failing with

LogicException: Cannot change the ID of an active session in Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy->setId() 

Runs fine via the command-line.
Note that one of the failing tests is SimpleTestTest, so hopefully that means we have coverage for the fact that the UI is broken.

/me keeps digging

I have been getting the same error lately for different reasons, but I have not been able to reproduce the problem consistently.

Re #31: this is #2239969: Session of (UI) test runner leaks into web tests which currently is blocked by #2272987: Do not persist session manager.

Re #35 only the UI test runner is affected, the test-bot isn't.

So the good news is that the remaining fails are fixed by other patches.

• LocaleLookupTest - the patch in #5 on #2284111: Exceptions caught and printed from index.php should not return a 200 status code - in fact this patch is just exposing an error that is already occurring.
• SimpleTestTest/InstallationProfileModuleTestsTest - we need to fix the session manager to stop bleeding into the test framework - this needs #2272987: Do not persist session manager and #2239969: Session of (UI) test runner leaks into web tests

The patch attached contains all these patches. Also I think we need to do #2272879: Can not run a WebTestBase or KernelTest base tests from inside a simpletest if we're going to do #2272987: Do not persist session manager and #2239969: Session of (UI) test runner leaks into web tests since the last time we committed #2239969: Session of (UI) test runner leaks into web tests we discovered that we were not really testing the UI for the test framework.

alexpott: Silly question. If those patches all pass when we merge them together here, how come several are still not RTBC themselves? :-) What's the commit strategy here for this chain?

Patch didn't apply, rebased and fixed two conflicts.

Interdiff might be hosed, interdiff complained.

regarding #43 @fabpot I suspect people are not posting patches because it assigned to somebody, perhaps try unassigning

I fixed one of the test failures and deleted the test that covered synchronized services, as @fabpot said this is no longer necessary in #39.

deleted the test that covered synchronized services

Hmmm... but the test shows that we do still need it. If we were not overriding ContainerBuilder::set() to allow us to set services on a frozen container then we would not need it. I looked at trying to remove this override but that would cause this patch to get even bigger.

Also @xano your post was an xpost with mine so interdiff is back to #44.

In order to get this in we need to fix #2272987: Do not persist session manager and #2239969: Session of (UI) test runner leaks into web tests first.

Rerolled.

Had some interesting issues with DrupalKernel::getContainer(). This code always calls watchdog until the container can be dumped - but because there is syslog or db_log module enabled this goes no where! In InstallerKernel we currently prevent dumping until the settings.php is written with the hash salt. I've removed this a properly set the allows dumping on DrupalKernel construction by doing:

  // Only allow dumping the container once the hash salt has been created.
  $kernel = InstallerKernel::createFromRequest($request, drupal_classloader(), $environment, (bool) Settings::get('hash_salt', FALSE));

In install.core.inc

We still need #2239969: Session of (UI) test runner leaks into web tests to land. Nearly there.

Anyone care to push this Critical along? Would be a pity to lose momentum.

#2239969: Session of (UI) test runner leaks into web tests got in, so here's a re-roll, just a bunch of trivial conflicts due to different comments, empty lines and type safe comparisons.

@moshe: This was blocked on that other issue, there wasn't much to push around here :)

These are failing from the call to watchdog() in getContainer() as it is not dumped to disk. The watchdog call is then trying to get the user id() in the LoggerChannel:

    if ($this->currentUser) {
      $context['user'] = $this->currentUser;
      $context['uid'] = $this->currentUser->id();
    }

As the request is only added to the stack in preHandle(), there is no request for AccountProxy > AuthenticationManager to use.

Simplest approach is to not add the id() to the context, and just keep the user. Else, we need to think about either the watchdog call or how the request can be added to the stack. Failure is happening really because the request is not added to the stack.

This should at least isolate and show it passes. Probably not the right solution though.

DrupalKernelTest was only passing in HEAD because this code

      if ($this->container) {
        try {
          $instance->setRequest($this->container->get('request'));
          $instance->setCurrentUser($this->container->get('current_user'));
        }
        catch (RuntimeException $e) {
          // We are not in a request context.
        }
      }

In LoggerChannelFactory ... an exception was being through from $this->container->get('request') because we're getting an unset synthetic service... the request stack is not synthetic so this does not occur anymore.

Looks good!

+++ b/core/lib/Drupal/Core/Logger/LoggerChannel.php
@@ -90,15 +90,15 @@ public function log($level, $message, array $context = array()) {
     if ($this->requestStack && $request = $this->requestStack->getCurrentRequest()) {

Do we need to bother checking for a request stack now? We will always have one?

Ignore me, looks good!

CRs that need updating:

• https://www.drupal.org/node/1539454
• https://www.drupal.org/node/1635626
• https://www.drupal.org/node/1851490 (this looks like it might need a total rewrite - not because of this issue)
• https://www.drupal.org/node/1969794
• https://www.drupal.org/node/1983438 (maybe)
• https://www.drupal.org/node/2026025

looks great:)

1. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
   @@ -18,14 +19,11 @@
   +  public function __construct(ParameterBagInterface $parameterBag = null)
   +  {
   +      $this->setResourceTracking(false);
   
   +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
   @@ -99,24 +100,26 @@ public function __construct(RouteProviderInterface $provider, OutboundPathProces
   +  // @todo this should probably be inline in the constructor as this is only
   +  // useful to get some current tests pass
   +  public function updateRequest()
   +  {
   
   +++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
   @@ -83,6 +84,8 @@ class AccessManagerTest extends UnitTestCase {
    
   +  protected $requestStack;
   +
   
   +++ b/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
   @@ -133,12 +134,15 @@ function setUp() {
   +    $generator = new UrlGenerator($provider, $processor_manager, $this->routeProcessorManager, $config_factory_stub, new Settings(array()), null, $requestStack);
   ...
   +    $generator = new UrlGenerator($provider, $processor_manager, $this->routeProcessorManager, $config_factory_stub, new Settings(array('mixed_mode_sessions' => TRUE)), null, $requestStack);
   

   should obey drupal coding standards.
   also some of them miss docs

2. +++ b/core/lib/Drupal/Core/Form/FormBase.php
   @@ -128,7 +128,7 @@ public function resetConfigFactory() {
      protected function getRequest() {
        if (!$this->request) {
   -      $this->request = $this->container()->get('request');
   +      $this->request = \Drupal::request();
        }
        return $this->request;
   

   shouldnt this method now always use the request stack and not store it somewhere? that way we make sure request doesnt get outdated

3. +++ b/core/lib/Drupal/Core/Logger/LoggerChannelFactory.php
   @@ -39,16 +39,12 @@ public function get($channel) {
   -        try {
   ...
   -        }
   -        catch (RuntimeException $e) {
   ...
   -        }
   

   thats great:) but lets also remove the use statement from the top of the exception

4. +++ b/core/modules/book/src/Plugin/Block/BookNavigationBlock.php
   @@ -67,7 +67,7 @@ public static function create(ContainerInterface $container, array $configuratio
   -      $container->get('request'),
   +      $container->get('request_stack')->getCurrentRequest(),
   

   shouldnt we inject the stack instead? that way request wont get outdated

1. Fixed - what's wrong with core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php - could see it. The 80 char line length is not mandatory for code.
2. Using the request stack now
3. Removed use - that shouldn't have been using the symfony exception anyway.
4. Using the request stack now

Fixed tests...

thanks

1. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
   @@ -18,14 +19,9 @@
   +  public function __construct(ParameterBagInterface $parameterBag = null) {
   

   needs docblock and null should be uppercase

2. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
   @@ -18,14 +19,9 @@
   +    $this->setResourceTracking(false);
   

   false should be uppercase

3. +++ b/core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php
   @@ -125,12 +126,15 @@ function setUp() {
   +    $generator = new UrlGenerator($provider, $processor_manager, $this->routeProcessorManager, $config_factory_stub, new Settings(array()), null, $requestStack);
   ...
   +    $generator = new UrlGenerator($provider, $processor_manager, $this->routeProcessorManager, $config_factory_stub, new Settings(array('mixed_mode_sessions' => TRUE)), null, $requestStack);
   

   null should be uppercase

Thanks - couldn't see it :)

thanks, this looks really great!

Not really a review but more of a series of questions about things I didn't really understand. I'd appreciate it if you can clarify on these.

1. +++ b/core/lib/Drupal.php
   @@ -158,7 +158,7 @@ public static function hasService($id) {
   -    return static::$container && static::$container->has('request') && static::$container->initialized('request') && static::$container->isScopeActive('request');
   +    return static::$container && static::$container->has('request_stack') && static::$container->get('request_stack')->getMasterRequest() !== NULL;
      }
   

   Should we document why we are using getMasterRequest here, compared to getCurrentRequest everywhere else. It might be important.

2. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
   @@ -74,7 +74,7 @@ public function setAccount(AccountInterface $account) {
   -      $this->setAccount($this->authenticationManager->authenticate($this->request));
   +      $this->setAccount($this->authenticationManager->authenticate($this->requestStack->getMasterRequest()));
        }
   

   Same here.

3. +++ b/core/modules/content_translation/src/Tests/ContentTranslationSyncImageTest.php
   @@ -116,7 +116,7 @@ function testImageFieldSync() {
   -    $attributes = $this->container->get('request')->attributes;
   +    $attributes = \Drupal::request()->attributes;
   

   Shouldn't we use the container to get the request, rather than using \Drupal? This happens in a few other places as well. I am not too sure about this one but I thought I'd bring it up.

1. +++ b/core/lib/Drupal/Core/Logger/LoggerChannel.php
   @@ -90,15 +90,15 @@ public function log($level, $message, array $context = array()) {
   +    if ($this->requestStack && $request = $this->requestStack->getCurrentRequest()) {
   ...
   +      if ($this->currentUser) {
   +        $context['user'] = $this->currentUser;
   +        $context['uid'] = $this->currentUser->id();
   +      }
   

   Is there a reason why the current user is now wrapped inside the request stack condition? I could imagine that there might be no request but a manual set user for CLI scripts

2. +++ b/core/modules/comment/tests/src/Entity/CommentLockTest.php
   @@ -25,8 +27,10 @@ public function testLocks() {
   +    $requestStack = new RequestStack();
   +    $requestStack->push(Request::create('/'));
   +    $container->set('request_stack', $requestStack);
   
   +++ b/core/modules/language/src/LanguageNegotiator.php
   @@ -87,11 +87,12 @@ class LanguageNegotiator implements LanguageNegotiatorInterface {
   +  public function __construct(ConfigurableLanguageManagerInterface $language_manager, PluginManagerInterface $negotiator_manager, ConfigFactoryInterface $config_factory, Settings $settings, RequestStack $requestStack) {
   ...
   +    $this->requestStack = $requestStack;
   
   +++ b/core/modules/language/src/Tests/LanguageUILanguageNegotiationTest.php
   @@ -63,7 +64,9 @@ function setUp() {
   +    $requestStack = new RequestStack();
   +    $requestStack->push($this->request);
   +    $this->container->set('request_stack', $requestStack);
   
   +++ b/core/modules/simpletest/src/TestBase.php
   @@ -1087,7 +1088,10 @@ private function prepareEnvironment() {
   +    $requestStack = new RequestStack();
   +    $requestStack->push($request);
   +    $this->container->set('request_stack', $requestStack);
    
   

   Just in case you have some time please convert that to non-camel-case

3. +++ b/core/modules/language/src/Tests/LanguageUILanguageNegotiationTest.php
   @@ -63,7 +64,9 @@ function setUp() {
        $this->request = Request::createFromGlobals();
   -    $this->container->set('request', $this->request);
   ...
    
   

   This example actually should just push a new request onto the container? It already will have a stack.

@hussainweb #74

1. This is checking if we have a request it is used is super early code... if we have a master request then we have a request.
2. Authentication should be done against the master request
3. This is a test - does not matter

@dawehner #75

1. The current_user depends on the request_stack if we want to decouple this then we have to decouple that - not for this issue.
2. Sure
3. Yeah that looks weird let's try this

I think we should add inline docs for #74/#76 #1, needs an explanation there to make it clear what's going on. Didn't spot anything else obvious.

Actually I don't think Drupal::hasRequest() has any real reason to use getMasterRequest so lets change that so it maps nicely to Drupal::request().

Added a comment for the authentication use case.

Needed a reroll due to #2196241: Remove string translation services from TestBase container a whole hunk of request and container futzing in TestBase was ripped out. Nice!

Need to use request_stack (not request) in the install tests.

Committed/pushed to 8.x, thanks!

Question:

I've studied this change intensively, but I'm not able to see where & how the request_stack gets re-injected into the rebuilt container after a call to DrupalKernel::updateModules().

It magically appears to exist, and the current Request object is the identical object as the one prior to calling updateModules(), but somehow I fail to see the responsible lines of code for that (since this patch seemingly removed the code that manually re-injected it).

Can someone enlighten me? (Or am I looking at a mystery?)