Expose session handler in container

+1 this looks great. Nice tests and everything.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/StackedSessionHandlerPass.php
   @@ -0,0 +1,51 @@
   +/**
   + * Provides a compiler pass for stacked session save handlers.
   + */
   +class StackedSessionHandlerPass implements CompilerPassInterface {
   +
   +  /**
   

   As talked yesterday, we should document/justify why its okay to create a pass for just a single session service by default.

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/StackedSessionHandlerPass.php
   @@ -0,0 +1,51 @@
   +
   +    $container->setAlias('session_handler', $decorated_id);
   

   It would be great to expose the session_handler somehow at least by default in the core.services.yml file, so tools like PHPStorm can do its autocompletion based upon it. Can't you point to some class by default?

3. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -92,13 +90,7 @@ public function __construct(RequestStack $request_stack, Connection $connection,
   -    // Register the default session handler.
   -    // @todo Extract session storage from session handler into a service.
   -    $save_handler = new SessionHandler($this, $this->requestStack, $this->connection);
   -    $write_check_handler = new WriteCheckSessionHandler($save_handler);
   -    $this->setSaveHandler($write_check_handler);
   -
   -    parent::__construct($options, $write_check_handler, $metadata_bag);
   +    parent::__construct($options, NULL, $metadata_bag);
    
   

   +1 to no longer create the session handler automatically

4. +++ b/core/modules/system/tests/modules/session_test/src/Session/TestSessionHandlerProxy.php
   @@ -0,0 +1,100 @@
   +   *   The decorated kernel.
   

   nitpick ... its not a decorated kernel.

#2338727: Replace static SessionManager::$enabled property with WriteSafeSessionHandler class and resolve hidden circular dependency between SessionManager and SessionHandler just went in.

Straight re-roll

Fixed test fail due to class members' rename.

@znerol: No sweat. You did all the hard work :)

+++ b/core/lib/Drupal/Core/Session/SessionHandler.php
@@ -209,6 +206,22 @@ public function gc($lifetime) {
+   * Sets the session manager.
+   *
+   * @param \Drupal\Core\Session\SessionManagerInterface $session_manager
+   *   The session manager.
+   *
+   * @todo Remove dependency on session manager.
+   *   @see https://www.drupal.org/node/2342593
+   *   @see https://www.drupal.org/node/2338727
+   *
+   * @internal
+   */
+  public function setSessionManager(SessionManagerInterface $session_manager) {
+    $this->sessionManager = $session_manager;
+  }
+

The sessionManager variable is no longer used after #2338727: Replace static SessionManager::$enabled property with WriteSafeSessionHandler class and resolve hidden circular dependency between SessionManager and SessionHandler

Fixes #17 and #18 and adds the WriteSafeSessionHandler as a session handler proxy service.

Question about the tests - not sure if this is necessary, but does this simulate a stack with more than one member ?

@cpj: the test simulates a two member stack and verifies that the higher priority member is called before the lower priority one, as designed.

@almaudoh - thanks. I've reviewed and tested the code as far as I can, meaning it runs in our development environment, which is Beta-6 plus other relevant recent patches, and I've stepped through it in the debugger for a simple, non-multi-level stack. I couldn't get HEAD to run yesterday, but I that didn't appear to be related to this patch. Is that sufficient for RTBC ?

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -83,20 +82,17 @@ class SessionManager extends NativeSessionStorage implements SessionManagerInter
+   * @param \Drupal\Core\Session\WriteSafeSessionHandlerInterface $write_safe_handler
+   *   The session handler proxy that allows session writes to be disabled.
...
+  public function __construct(RequestStack $request_stack, Connection $connection, MetadataBag $metadata_bag, SessionConfigurationInterface $session_configuration, WriteSafeSessionHandlerInterface $write_safe_handler) {
...
+    $this->writeSafeHandler = $write_safe_handler;

Given that the plan is to not have the write safe handler in the session manger how about extracting the write safe handler in the setSaveHandler method? That way we have less constructor change.

Addressed #24

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -289,6 +286,19 @@ public function enable() {
+    else {
+      $this->writeSafeHandler = \Drupal::service('session_handler.write_safe');
+    }

Not sure if this is the best way, or whether to allow the exception if null is passed in.

This option caters for the case where another session proxy (higher priority) is added by some other module.

I like the idea to use the setter method for the handler rather than passing it in via the constructor. But I don't understand the need for the hard-coded fall back. Why would the presence of a higher priority session proxy trigger the setter to be called with null ?

@cpj: sorry I wasn't very clear in my earlier comment. A higher priority session proxy handler would mean the setter is called not with null, but an object that doesn't implement WriteSafeSessionHandlerInterface hence the setSessionWritable() method call would fail.

The fallback would allow us to still disable session writes since the higher priority proxy may still write through the write-safe handler.

But as has already been stated, this would only be temporary since the writeSafeHandler will be removed from SessionManager in a later refactoring.

Raised follow-up #2426031: Remove deprecated uses of SessionManager::isEnabled(), SessionManager::enable() and SessionManager::disable() to remove deprecated methods. We need someone to RTBC #28 so we can meet the next beta-release window. Anyone??

I tested #19 pretty thoroughly in #22. Reviewing the interdiffs for #25 and #28, I understand the changes to remove then add back the session injection, so as far as I can see #28 is very similar to #19, right ? Therefore I'm gonna set this as RTBC. OK ?

Straightforward re-roll after #2229145: Register symfony session components in the DIC and inject the session service into the request object got in. Back to RTBC per #32.

This issue is a major task that is part of on-going efforts to reduce fragility and improve extensibility of the session system by using Symfony's session components. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase. Committed a32a88c and pushed to 8.0.x. Thanks!

Thanks @alexpott. Now on to #2426031: Remove deprecated uses of SessionManager::isEnabled(), SessionManager::enable() and SessionManager::disable() which will remove the deprecated uses of SessionManager::enable(), ::disable(), etc.