Rebase SessionManager onto Symfony NativeSessionStorage

OMG What? It's that simple? Well I'm happy it passes.

However, I'm not sure we have enough tests. Can we Unit Test the SessionManager?

1. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -49,26 +51,37 @@ class SessionManager implements SessionManagerInterface {
   +  protected static $anyEnabled = TRUE;
   ...
   +  protected static $anyStarted = FALSE;
   

   Why are we renaming these variables?

2. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -167,7 +180,7 @@ public function save() {
   +      parent::save();
   

   Awesome

3. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -297,4 +316,45 @@ protected function isCli() {
   +    // If anything is remaining, this is data set directly on the $_SESSION
   +    // superglobal.
   +    return empty($remaining_keys);
   

   Freaking Awesome

1. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -267,14 +285,14 @@ public function delete($uid) {
      public function isEnabled() {
   -    return static::$enabled;
   +    return static::$anyEnabled;
   

   Perhaps we should rename the internal/protected static property name to $locked now?

   We'd just negate its value then (and keep the existing methods).

   Just an idea. Not sure.

   ...The concept is a bit flawed anyway... When session saving is disabled and re-enabled, then we are still saving potentially unsecure data, because the original session (from the time at which session saving was disabled) has not been cloned/retained as a separate copy (which could then be restored).

2. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -297,4 +315,45 @@ protected function isCli() {
   +    $remaining_keys = array_flip(array_keys($session));
   ...
   +      unset($remaining_keys[$key]);
   ...
   +    unset($remaining_keys[$this->metadataBag->getStorageKey()]);
   ...
   +    return empty($remaining_keys);
   

   Somehow I've the impression this code could be vastly simplified by using array_diff_key() ?

Can you explain in the summery in more detail the motivation for using the symfony code here?

#7 wasn't addressed yet.

Additionally reviewed the latest patch:

1. +++ b/core/core.services.yml
   @@ -723,9 +723,12 @@ services:
   +  session_manager.metadata_bag:
   ...
      session_manager:
   

   1) Can we move this definition below session_manager? (as it's more specific)

   2) Could we rename the service ID to just session_metadata? → That would inherently make the Settings name consistent withi that.

2. +++ b/core/lib/Drupal/Core/Session/MetadataBag.php
   @@ -0,0 +1,30 @@
   +    $storage_key = $settings->get('session_metadata_key', '_sf2_meta');
   

   Is there any sensible use-case for overriding this key?

   If so, it would be good to add an inline comment that states so accordingly; i.e.:

   // Allow ... to ... in case ...

   If not, then we could consider to remove this setting?

3. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -126,8 +138,8 @@ public function start() {
   +    parent::start();
   +    static::$anyStarted = TRUE;
   

   Is there actually a difference between the two, aside from ours being static?

   If there is no difference, then I think it would be time to figure out how we can get rid of this static. — Perhaps we should attempt that in a separate sister issue first?

   Created #2239181: SessionManager::isStarted() returns TRUE even after session_write_close() has been called

When the SessionManager::regenerate() is called by eg. user_login_finalize($this->user) the session id invalid:

Warning: session_start(): The session id is too long or contains illegal characters, valid characters are a-z, A-Z, 0-9 and '-,' in Drupal\Core\Session\SessionManager->start() (line 129 of /sites/drupal8/core/lib/Drupal/Core/Session/SessionManager.php).

( fresh git checkout )

adding #2245003: Use a random seed instead of the session_id for CSRF token generation to the issue summary, because this issue is not "blocking" that one (yet) but getting this in would help there.

So it seems as if this is ready to go. It just needs some serious reviews. I tried to review this with dreditor but I think I need to apply the patch and review this in context of the rest of the code.

mostly just nits. still needs a good read through.

1. +++ b/core/lib/Drupal/Core/Session/MetadataBag.php
   @@ -0,0 +1,29 @@
   + * Contains \Drupal\Core\Session\MetadataBag
   

   missing period at end of sentence.

2. +++ b/core/lib/Drupal/Core/Session/MetadataBag.php
   @@ -0,0 +1,29 @@
   + * Metadata container for application specific session metadata.
   

   https://drupal.org/coding-standards/docs#classes

   "Use a third person verb to start the summary of a class, interface, or method. For example: "Represents a ..." or "Provides..."."

   Tried to figure out what a 'Bag' does...
   "A bag contains variables or parameters. http://stackoverflow.com/questions/19798108/in-symfony-what-is-a-bag"

3. +++ b/core/lib/Drupal/Core/Session/MetadataBag.php
   @@ -0,0 +1,29 @@
   +   * Construct a new metadata bag instance.
   

   similar. Constructs

4. +++ b/core/lib/Drupal/Core/Session/MetadataBag.php
   @@ -0,0 +1,29 @@
   +   * @param \Drupal\Core\Utility\Settings $settings
   

   there is no such class. :)
   Core -> Component

5. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -54,14 +64,29 @@ class SessionManager implements SessionManagerInterface {
   +    // @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 as soon as we are using
   +    // the Symfony session object (which registers an attribute bag with the
   +    // manager upon instantiation).
   +    $this->bags = array();
   

   reformatting (https://drupal.org/coding-standards/docs#todo),

   and adding the link to the actual issue #2229145: Register symfony session components in the DIC and inject the session service into the request object

   But maybe this todo actually needs it's own issue which would be a follow-up of 2229145.

6. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -289,4 +334,39 @@ protected function isCli() {
   +   * Test whether the session is empty.
   

   Third person verb again.

7. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -289,4 +334,39 @@ protected function isCli() {
   +   *
   +   * @return bool
   +   *   TRUE when the session does not contain any values.
   +   */
   +  protected function isEmptySession(array $session = NULL) {
   

   adding @param

   Is it an array of sessions, or.. an array of info that describes a session?

   Looked at the usages, and both calls do not pass in an argument. So I'm not sure what $session might be.

8. +++ b/core/lib/Drupal/Core/Session/SessionManagerInterface.php
   @@ -77,4 +57,24 @@ public function disable();
   +   * Whether or not to enable mixed mode SSL sessions in the session manager.
   +   *
   +   * @param bool $mixed_mode
   +   *   New value for the mixed mode SSL sessions flag.
   +   */
   +  public function setMixedMode($mixed_mode);
   

   changing the doc to Enables or disables.

   But, question: should this also return $this; ?
   I think that's a common pattern for set methods so that we can do chaining.

9. +++ b/core/lib/Drupal/Core/Session/SessionManagerInterface.php
   @@ -77,4 +57,24 @@ public function disable();
   +   * The name of the insecure session when operating in mixed mode SSL.
   +   */
   +  public function getInsecureName();
    }
   

   adding a @return
   https://drupal.org/coding-standards/docs#return

   (and the newline before the class end } )
   "Leave an empty line between end of method and end of class definition:"
   https://drupal.org/node/608152#indenting

1. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -72,18 +98,20 @@ public function initialize() {
   -    $handler = new SessionHandler($this, $this->requestStack, $this->connection);
   -    session_set_save_handler($handler, TRUE);
   +    $save_handler = new SessionHandler($this, $this->requestStack, $this->connection);
   +    $write_check_handler = new WriteCheckSessionHandler($save_handler);
   +    $this->setSaveHandler($write_check_handler);
   

   this made my head spin. I think I read it 5 time.

   we had a save handler, then we got a write_check handler, then we set the save handler to the write_check handler. maybe this makes sense to people who know about sessions?

2. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -72,18 +98,20 @@ public function initialize() {
   -    if (($cookies->has(session_name()) && ($session_name = $cookies->get(session_name()))) || ($is_https && Settings::get('mixed_mode_sessions', FALSE) && ($cookies->has(substr(session_name(), 1))) && ($session_name = $cookies->get(substr(session_name(), 1))))) {
   +    $insecure_session_name = $this->getInsecureName();
   +    if (($cookies->has($this->getName()) && ($session_name = $cookies->get($this->getName()))) || ($is_https && $this->isMixedMode() && ($cookies->has($insecure_session_name) && ($session_name = $cookies->get($insecure_session_name))))) {
   
   @@ -94,12 +122,8 @@ public function initialize() {
   -        $insecure_session_name = substr(session_name(), 1);
   

   ah, I see, the setting the insecure name was moved up before the if, out of the else part, since it was used in the condition up there anyway.

3. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -94,12 +122,8 @@ public function initialize() {
   -      // Less random sessions (which are much faster to generate) are used for
   -      // anonymous users than are generated in regenerate() when
   -      // a user becomes authenticated.
   -      session_id(Crypt::randomBytesBase64());
   -      if ($is_https && Settings::get('mixed_mode_sessions', FALSE)) {
   

   why did we delete this comment. was it incorrect?

   it might have been explaining why we use 64 instead of something else? i'm not sure what it is less random than.

Oh, I think the issue summary was updated after #10 asked for it. removing that tag.

Updating remaining tasks.

this isn't a true interdiff. it's just a diff of the last two patches with some noise removed.

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -305,11 +351,29 @@ protected function isCli() {
+    $used_session_keys = array_filter($this->getSessionDataMask());
+    return empty($used_session_keys);

hm.

http://www.php.net/manual/en/function.array-filter.php

If no callback is supplied, all entries of array equal to FALSE (see converting to boolean) will be removed.

so isSessionObsolete() removes any keys that evaluate to false, and then checks if it's empty, and if it is, it is obsolete.

Are the docs on this method still ok? I looked and I think they are ok still.

tiniest nit. just fixed it. (https://drupal.org/node/1354#functions third person verb, Returns instead of Return)

Next:
I'll open the followups.

I created #2256257: Move token seed in SessionManager in isSessionObsolete() and regenerate() to the MetadataBag. removing the needs follow-up tag.

Seems like the two @todos that reference this issue (2238087) were the only two. And that they should be done in the same issue. (If not, please explain/create the other issue.)

Looks good to me

The title/issue summary confuses me a bit - given we don't actually want to use NativeSession Storage.

Is implementing the interface directly that bad?

If it is, then that seems like a good case for adding a BaseSessionStorage abstract class upstream, rather than extending a specific implementation.

@catch: This issue is just one more "baby step" of the parent issue. The ultimate end goal is still to use all of Symfony's Session handling natively.

Great comment, Still Good.

Thanks! That makes it a lot more obvious what's going on.

Committed/pushed to 8.x.