Register symfony session components in the DIC and inject the session service into the request object

Back to active again.

Here you go. Session tests and phpunit tests pass. Lets see what the test bot can do with everything else.

I wouldn't say this is a perfect patch. I wasn't able to completely get rid of the manager interface which I see as sort of a sign we haven't completely made it through the conversion but this meets the goals of this issue at least.

Woops, missed a file.

https://github.com/neclimdul/drupal/commits/symfony_session for commits to get here. Tried to make the steps I took as clean and explicit as possible.

#2272987: Do not persist session manager Is in, this needs a re-roll.

1. +++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
   @@ -18,27 +18,10 @@
   +    return $request->hasSession();;
   

   Let's add more semicolons, what about 12? :)

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -707,6 +710,14 @@ protected function initializeContainer($rebuild = FALSE) {
   +    if (($request_stack = $this->container->get('request_stack', ContainerInterface::NULL_ON_INVALID_REFERENCE))) {
   +      if ($request = $request_stack->getCurrentRequest()) {
   +        if ($request->hasSession()) {
   +          $request->setSession($this->container->get('session'));
   +        }
   +      }
   +    }
   

   It would be great to explain why we have to still set the session here. Isn't that covered by the session middleware + prepareLegacyRequest?

3. +++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
   @@ -101,13 +101,15 @@ public function handlePageCache(Request $request);
   +   *   Whether or not the session should accessible from the request.
   

   There is certainly a verb missing in here :) Can you explain when you would actually not need this here? Would drush be an example of it?

4. +++ b/core/lib/Drupal/Core/StackMiddleware/Session.php
   @@ -0,0 +1,63 @@
   +   * @param \Symfony\Component\HttpFoundation\Session\Session $session
   

   ... so we cannot typehint the interface because of SessionInterface::getFlashBagpretty neat!

5. +++ b/core/lib/Drupal/Core/StackMiddleware/Session.php
   @@ -0,0 +1,63 @@
   +    if ($type === self::MASTER_REQUEST) {
   +      $request->setSession($this->session);
   +    }
   ...
   +    if ($type === self::MASTER_REQUEST) {
   +      $this->session->save();
   +    }
   

   Can you add some short comment about why we check for just the master request?

6. +++ b/core/modules/user/user.module
   @@ -1522,6 +1522,7 @@ function user_logout() {
    
   +  $user->setAccount($GLOBALS['user'] = new AnonymousUserSession());
   

   So this should have been done before?

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -569,8 +569,9 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch =
+    // Setup services which are normally initialized from within stack
+    // middleware or during the request kernel event.
+    $request->setSession($this->container->get('session'));

I'm not entirely clear on this comment. I read it as "we should do this in a middleware but we'll do it here instead", which seems odd because we do have a middleware.

Other than that, I'm happy here. znerol++

Let's do then.

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

And we need a change record here that details the 8.x to 8.x changes as well as going through the existing CRs and making sure the 7.x to 8.x is covered and correct.

Raised the priority.

Afaics we will need to update https://www.drupal.org/node/2228871? Can someone else confirm and attach any other CRs that need updating.

Sorry but #45 was not a proper reroll

1. +++ b/core/includes/install.core.inc
   @@ -414,13 +415,11 @@ function install_begin_request(&$install_state) {
   -  // After setting up a custom and finite module list in a custom low-level
   -  // bootstrap like here, ensure to use ModuleHandler::loadAll() so that
   -  // ModuleHandler::isLoaded() returns TRUE, since that is a condition being
   -  // checked by other subsystems (e.g., the theme system).
   -  $module_handler->loadAll();
    
   -  $kernel->prepareLegacyRequest($request);
   +  // Load all modules and perform request related initialization.
   +  $kernel->preHandle($request);
   +  $request->attributes->set(RouteObjectInterface::ROUTE_OBJECT, new Route('<none>'));
   +  $request->attributes->set(RouteObjectInterface::ROUTE_NAME, '<none>');
   

   Can you make at least a @see prepareLegacyRequest from which this code is copied from?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   @@ -111,6 +111,11 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $path, $s
   +        // Carry over the session to the subrequest.
   +        if ($session = $request->getSession()) {
   +          $sub_request->setSession($session);
   +        }
   

   So everyone who does a subrequest would need to do that? Given that this sounds like a generic functionality we need somewhere.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   @@ -111,6 +111,11 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $path, $s
   +        // Carry over the session to the subrequest.
   +        if ($session = $request->getSession()) {
   +          $sub_request->setSession($session);
   +        }
   
   +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -130,6 +130,10 @@ public function commentPermalink(Request $request, CommentInterface $comment) {
   +      // Carry over the session to the subrequest.
   +      if ($session = $request->getSession()) {
   +        $redirect_request->setSession($session);
   +      }
   

   I agree with @dawehner - can we automate this with an event subscriber?

2. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -329,7 +329,7 @@ protected function getSessionDataMask() {
   -      $mask[$key] = empty($_SESSION[$key]);
   +      $mask[$key] = !empty($_SESSION[$key]);
   

   This is a fairly major change in logic - can you elaborate why it is needed?

Fixes #52 - attempts to add event subscriber to hand down session from parent requests to sub-requests.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/SubRequestSessionSubscriber.php
   @@ -0,0 +1,66 @@
   +use Drupal\Core\Authentication\AuthenticationProviderInterface;
   ...
   +use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent;
   +use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
   

   unneeded

2. +++ b/core/lib/Drupal/Core/EventSubscriber/SubRequestSessionSubscriber.php
   @@ -0,0 +1,66 @@
   +  public function __construct(RequestStack $request_stack) {
   

   help if you added this to core.services.yml

so how in the world did #54 pass?

the subscriber seems to work - manual testing of comment permalink

So we need 51 with the docs changes of 54?

I was going to do the reroll but I ran into a problem.

@see doesn't clearly convey any meaning in this context without reading this issue. Especially since its missing so much of the logic from that method so any documentation you could gleam from that method looses meaning. I think it would be better to write clearer documentation of what we're doing and then if we really feel strongly document that it is similar to or loosely based on the other method.

I played around to better understand how to document this and the routing properties don't seem necessary. I guess the routing properties are sort of a "just in case" sort of thing.

Needed a reroll after #2347877: Move DrupalKernel::initializeCookieGlobals() into a SessionConfiguration service so that's done too.

I reviewed the code and have tested the patch from #61 in a similar fashion to #57. I have also stepped through the code in the debugger of my IDE to understand what the StackMiddleWare/Session is doing. As far as I can tell is works as expected, but what about the additional code in #55 ? Is this still needed (I think so...)

I've just noticed that the class explanation text, lines 14 - 16 of StackMiddleware/Session.php will need to change. The current text is the same as StackMiddleware/PageCache.php

In #55, line 10

+++ b/core/core.services.yml
@@ -1030,6 +1035,21 @@ services:
+  session.subrequest_session_subscrber:

Typo - I assume this should be session.subrequest_session_subscriber

Does anything use this service yet ?

So we need 51 with the docs changes of 54?

I made the assumption that we didn't need #55

@neclimdul & @znerol - thanks for the clarification. So I review & tested #65 with the latest HEAD and it works in our environment, so this looks good-to-go to me

#65 re-roll patch -p1 worked, but git apply didn't.

Is there any manual testing we could do to confirm this is good to go? It looks great:)

Is there some reason why this can't be marked as "Reviewed & tested by the community" ?

@cpj nope, if you have reviewed and tested, then you can set the status;)

Let me take a look at it...

Another re-roll. no changes.

Re-RTBC as per #73 and I double checked #68 vs #75 to see if there were any sneaky re-roll accident and there were not so if testbot agrees it will stay.

Testbot hickup, thanks @larowlan

git ac https://www.drupal.org/files/issues/register_symfony-2229145-75.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 20761  100 20761    0     0  78620      0 --:--:-- --:--:-- --:--:-- 79240
error: patch failed: core/core.services.yml:991
error: core/core.services.yml: patch does not apply

      $session_manager = \Drupal::service('session_manager');
      // If the password has been changed, delete all open sessions for the
      // user and recreate the current one.
      if ($this->pass->value != $this->original->pass->value) {
        $session_manager->delete($this->id());
        if ($this->id() == \Drupal::currentUser()->id()) {
          $session_manager->regenerate();
        }
      }

      // If the user was blocked, delete the user's sessions to force a logout.
      if ($this->original->status->value != $this->status->value && $this->status->value == 0) {
        $session_manager->delete($this->id());
      }

Does this code in \Drupal\user\Entity\User need to change too?

Straight re-roll

Well...the actual patch

Back to RTBC.

Thank you for rerolling @almaudoh

The change #81was already in the patch. So I'll mark this RTBC per #79.

Oh I missed that, @almaudoh, do you know if the other $session_manager variables are supposed to change as well for ->delete() calls? @alexpott may not have been refering that so just checking.

  $session_manager->delete($this->id());

In \Drupal\user\Entity\User

Good point @joelpittet

Right now, there is still a lot of coupling between SessionManager, SessionHandler and Sessions which would be decoupled in a followup (mentioned in the IS), these others would be in that follow up.

Committed b926f5d and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

I've published the change record - I think we need to update https://www.drupal.org/node/2228871

Drats... this breaks run-tests without a database. Which is about to be used to remove the drush dependency from testbots. I have to revert because we shouldn't be creating critical followup of major issues.

Here's the fix. The small patch is the interdiff.

The issue to remove drush from the testbots is #2206501: Remove dependency on Drush from test reviews

+++ b/core/core.services.yml
@@ -475,6 +475,11 @@ services:
+  http_middleware.session:
+    class: Drupal\Core\StackMiddleware\Session
+    arguments: ['@session']
+    tags:
+      - { name: http_middleware, priority: 50 }

@@ -1089,6 +1094,15 @@ services:
+  session:
+    class: Symfony\Component\HttpFoundation\Session\Session
+    arguments: ['@session_manager', '@session.attribute_bag', '@session.flash_bag']

This is problematic because session_manager depends on the database so we'll content to the database even we we don't need to.

#97:

we'll content to the database even we we don't need to

I think the concern is that we would tend to make writes to the session table in the db even when there is no session or nothing in the session.

Looking thru the code, there's nothing that tells me that would happen. But there's nothing that assures me it won't happen either. However, I know one of the follow ups to be created for decoupling authentication from session management would handle this.

@znerol: You may probably have a better answer than mine.

So without the additions in #95 running $ php ./core/scripts/run-tests.sh --list in a checkout of drupal which has not been installed - ie. no settings.php with db settings - generates the following error:

PHP Fatal error:  Uncaught exception 'Drupal\Core\Database\ConnectionNotDefinedException' with message 'The specified database connection is not defined: default' in /Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Database/Database.php:366
Stack trace:
#0 /Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Database/Database.php(171): Drupal\Core\Database\Database::openConnection('default', 'default')
#1 [internal function]: Drupal\Core\Database\Database::getConnection('default')
#2 /Volumes/devdisk/dev/drupal/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php(960): call_user_func_array(Array, Array)
#3 /Volumes/devdisk/dev/drupal/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php(490): Symfony\Component\DependencyInjection\ContainerBuilder->createService(Object(Symfony\Component\DependencyInjection\Definition), 'database')
#4 /Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php(36): Symfony\Compo in /Volumes/devdisk/dev/drupal/core/lib/Drupal/Core/Database/Database.php on line 366

This is happening because the session manager has the database injected and calling Database::openConnection(). So we're creating a connection to the db super early.

re #101 but then we still have more page_cache_without_database problems, no?

#104 works in our environment and the latest changes make sense to me, so I'm going to mark this as RTBC. OK ?

Discussed with @catch on IRC - he said "so we initialize the database connection just to check a cookie, yes I don't like that either." I feel the same. We should only connect to the db if the cookie exists.

@alexpott #107 - interesting comment, but I think @znerol is correct. What is there in this patch that changes this situation ?

Yeah I think cpj and znerol are right. The database behavior hasn't changed in this patch. The problem in #100 might have been miss-stated a bit though. The problem is actually run-test.sh calling DrupalKernel::prepareLegacyRequest() and prepareLegacyRequest changing slightly. Enough work has gone in that we don't have to pretend that we are working with a request in run-test so really run-test doesn't need to call prepareLegacyRequest anymore we can just call boot() and loadLegacyIncludes() directly.

Also, we can problem get rid of those PHP_SAPI checks. If something on the CLI is going through the http_kernel we've got problems.

Full disclosure for discussion, this does mean that cli tools have to assume a database connection or not use prepareLegacyRequest so that's sort of a regression. It doesn't affect drush though from what I can tell and run-test.sh was the only CLI tool we where using it in.

My bad, I was poking at some other code and forgot to remove it before making the patch.

Also, I was mistaken and some tests actually require the prepareLegacyRequests. They fail in the runner which means they're probably unit tests. Unit tests relying on global state is pretty much a bug but we can follow up to fix that. For now the runner can boot legacy request stuff when it is executing a test.

@neclimdul just a small nitpick:

+++ b/core/scripts/run-tests.sh
@@ -32,9 +32,11 @@
-$kernel->prepareLegacyRequest($request);
+$kernel->boot();
+$kernel->loadLegacyIncludes();;

Double ;;.

Removed the extra ';'.

Clearly I was wrong that we have done enough work in run-tests to get rid of those assumptions and my spot checks where not representative. The attached patch rolls back those CLI checks and run-tests changes and is the last RTBC'd patch#104 with a spelling fix.

To re-address alex and catch's concerns from when it was bumped NR, the database behavior is unchanged and if that is a problem it was an earlier regression not attached to this improvement. Since this basically gets us to the architecture we've been working for I would prefer if we can address that problem with in a separate issue with profiling and specific review. We might be able to just use a lazy proxy or something but we should better understand the problem.

that moment when you realize you where distracted by an email and forgot to attach the patches.

and the status. please confirm things are working testbot.

@neclimdul #118 - I agree that this is not the right place to start optimizing / fixing database behavior concerns. This patch changes nothing in that regard so "no harm, no foul", right ? But I'd be totally up for helping on a separate issue in that direction, if someone was to open one...

Are we sure that the middleware won't be instantiated on page cache hits? That would be a regression compared to the status quo too (if using memcache database backends + database session storage for example).

I opened #2424451: Session manager (and others) should not initialize database connection if no query needs to be run for the more general problem.

@catch #123 - I haven't been testing with caching enabled, so to be honest I'm not sure if the behavior is different than before on page cache hits... Logically I'm not sure why this patch should change what happens on page cache hits but I will try to take a look at it...

It depends whether all the middlewares get instantiated before any particular middleware gets executed or not.

There's a random failure in HEAD it seems. #2424743: Random testbot fail: "The page does not have double escaped HTML tags." in Drupal\field_ui\Tests\ManageFieldsTest.

Thanks catch, that's exactly the issue I was thinking and a great summary.

In case it wasn't clear because it wasn't in the comment, in #104 the patch has the middleware take the container instead of the session objects so it only creates services when its run and after it knows it needs them.

Core committer concerns raised in #107 have been addressed with a follow-up. The currrent patch does not introduce any regressions from HEAD and further optimizations can still be done in followups. Patch has been tested by @cpj in a dev environment and is ok.

Since all I did in this patch was a re-roll, I believe I can go ahead and RTBC this.

So I've not tested it again, but I've reviewed the latest version and there appear to be no substantial changes to the version I tested, so I agree this should be marked as RTBC as suggested by @almaudoh in #130.

Given that my contribution to this patch was only fixing run-tests.sh I think it is fine for me to commit.

Committed 7f6f61f and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

diff --git a/core/lib/Drupal/Core/StackMiddleware/Session.php b/core/lib/Drupal/Core/StackMiddleware/Session.php
index 0fb786a..f92e778 100644
--- a/core/lib/Drupal/Core/StackMiddleware/Session.php
+++ b/core/lib/Drupal/Core/StackMiddleware/Session.php
@@ -16,8 +16,7 @@
  *
  * Note, the session service is not injected into this class in order to prevent
  * premature initialization of session storage (database). Instead the session
- * is retrieved from the container only immediately before passing an incomming
- * request on to the wrapped kernel.
+ * service is retrieved from the container only when handling the request.
  */
 class Session implements HttpKernelInterface {
 

In discussion with @catch we decided to change the doblock on commit.

Wooot! :) Thanks @alexpott. We're getting there.

After this change, what uses SessionManager? I have attempted to find what code instantiates the SessionManager and have only found service container (generated) code that uses it.

Does that mean that we should remove the SessionManager in a follow up issue?

Edit: Found one reference to SessionManager's enable method in \Drupal\Core\Session\AccountSwitcher but that's the only one so far.

Ah looks like there's a follow up that addresses some of this: #2426031: Remove deprecated uses of SessionManager::isEnabled(), SessionManager::enable() and SessionManager::disable()

Related: #2597520: Don't save the session before $response->send().

We have a @todo against this issue in core/lib/Drupal/Core/Session/SessionManager.php

    // @todo When not using the Symfony Session object, the list of bags in the
    //   NativeSessionStorage will remain uninitialized. This will lead to
    //   errors in NativeSessionHandler::loadSession. Remove this after
    //   https://www.drupal.org/node/2229145, when we will be using the Symfony
    //   session object (which registers an attribute bag with the
    //   manager upon instantiation).

Do we have an issue to remove this?

Couldn't find an issue so opened #3206358: Remove bag initialization in SessionManager