Replace static SessionManager::$enabled property with WriteSafeSessionHandler class and resolve hidden circular dependency between SessionManager and SessionHandler

1. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -65,15 +65,15 @@ class SessionManager extends NativeSessionStorage implements SessionManagerInter
   @@ -96,9 +96,9 @@ public function __construct(RequestStack $request_stack, Connection $connection,
   
   @@ -96,9 +96,9 @@ public function __construct(RequestStack $request_stack, Connection $connection,
   +    $this->writeBarrierHandler = new WriteBarrierSessionHandler($write_check_handler);
   ...
   +    parent::__construct($options, $this->writeBarrierHandler, $metadata_bag);
   

   I wonder whether we could add an optional dependency already, just to be clear that we actually want to get in the dependency from outside?

2. +++ b/core/lib/Drupal/Core/Session/WriteBarrierSessionHandler.php
   @@ -0,0 +1,100 @@
   +<?php
   +/**
   

   Missing empty line.

3. +++ b/core/lib/Drupal/Core/Session/WriteBarrierSessionHandler.php
   @@ -0,0 +1,100 @@
   +  /**
   +   * @var \SessionHandlerInterface
   +   */
   +  private $wrappedSessionHandler;
   +
   +  /**
   +   * Whether or not the write barrier is enabled.
   +   *
   +   * @var bool
   +   */
   +  private $sessionWritable;
   +
   

   I really don't like the fact that we introduce privates here.

Is there an issue where the queries are being eliminated?

@znerol this looks great in terms of brittleness removal! I've added the beta evaluation table to the IS and added your note to move to Major into it, if you can fill out any of the other commented out rows that could help.

As said on IRC, I really don't get why we need this additional layer of abstraction. Why can't we just make SessionManager::$enabled a normal property and accept that a SessionHandler
needs the SessionManager ? Its a dependency we are aware of, and solve (which we do already).

I see what the code does, but I'm also unclear what problem we're trying to solve here. How can we decide if this is needed or not ?

Thanks @znerol - that makes sense. I've reviewed the code and have tested the code in our environment. I've stepped through the code in my IDE's debugger and it all works as expected. However, I haven't tested this as part of an environment that swaps out the SessionHandler but I can see that the tests go some way to simulate this. Therefore if there are no objections, I'm going to mark this as "Reviewed & tested by the community" (= me).

+++ b/core/lib/Drupal/Core/Session/WriteBarrierSessionHandler.php
@@ -0,0 +1,101 @@
+/**
+ * Wraps another SessionHandlerInterface to prevent writes during dangerous operations.
+ */
+class WriteBarrierSessionHandler implements \SessionHandlerInterface {

The name of the class - barrier seems strange. Also comment is longer than 80 chars. How about WriteSafeSessionHandler?

The Interface defined functions only say that the Class allows you to enable/disable writing and check the status, so it doesn't really define a "Safe" session handler. So how about WriteCheckSessionHandler ?

How about ReadOnlySessionHandler?

+1.

How about ReadOnlySessionHandler?

+1 from me too...

@znerol, @cpj: now that I look at the patch closely, I don't think ReadOnlySessionHandler is a very good name, because there is the option in this class to make the session writable (which implies that it is not read only).

So thinking through, @alexpott's WriteSafeSessionHandler actually sounds most appropriate, or maybe SafeWriteSessionHandler. If we're going with ReadOnlySessionHandler, then we have to get rid of setSessionWritable() and isSessionWritable()

We can't get rid of setSessionWritable() / isSessionWritable(), so...here's the rename.

@almaudoh - OK, I agree. Either are fine by me but SafeWrite*** seems slightly better to me than WriteSafe***

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -63,16 +63,16 @@ class SessionManager extends NativeSessionStorage implements SessionManagerInter
+   * @var \Drupal\Core\Session\WriteSafeInterface

Isn't this now a WriteSafeSessionHandlerInterface ?

Thanks @cpj and also some minor clean ups.

Looks great to me so I'm gonna change this to RTBC (again...)

Committed 6faa686 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

Looks like it wasn't committed. Can't find it on cgit.drupalcode.org or my local git clone.

Thanks @alexpott. Now on to #2372389: Expose session handler in container which was blocked on this issue.