Problem/Motivation

The introduction of the Symfony Session object as in #2229145: Register symfony session components in the DIC and inject the session service into the request object requires the SessionManagerInterface to comply fully with the Symfony SessionStorageInterface in order to make session starting/saving working as expected. That means that it is necessary to remove the startLazy() method and instead implement that behavior in the start() method.

Proposed resolution

Rename SessionManager::start() into SessionManager::startReal() and SessionManager::startLazy() into <code>SessionManager::start().

Remaining tasks

User interface changes

API changes

Remove SessionManagerInterface::startLazy() and SessionManagerInterface::isStartedLazy().

CommentFileSizeAuthor
#9 interdiff.txt1.75 KBznerol
#9 2338571-remove-session-manager-start-lazy-9.diff36.88 KBznerol
#6 interdiff.txt580 bytesznerol
#5 2338571-remove-session-manager-start-lazy-5.diff36.88 KBznerol
#5 2338571-remove-session-manager-start-lazy-5.diff36.88 KBznerol
#3 interdiff.txt36.88 KBznerol
#3 2338571-remove-session-manager-start-lazy-3.diff36.88 KBznerol
#1 2338571-remove-session-manager-start-lazy.diff7.94 KBznerol
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Active » Needs review
FileSize
2338571-remove-session-manager-start-lazy.diff7.94 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2338571-remove-session-manager-start-lazy.diff, failed testing.

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Needs work » Needs review
FileSize
2338571-remove-session-manager-start-lazy-3.diff36.88 KB
interdiff.txt36.88 KB

Fixing test-failures:

  1. We can tell whether a session has been started by simply examining the return value of SessionManager::start() in SessionTestSubscriber.
  2. There is absolutely no need for running the request-subscriber with a priority of 100 in the service provider TestClass.
  3. EntityCrudHookTest is a DrupalUnitTest, there is no need to use the $_SESSION here at all. Just substitute it with $GLOBALS

From the fixes above, #2 may be objectionable. In this case a drupal_set_message() call was issued before the session was active. In my opinion this points to an issue code with drupal_set_message() which simply assumes that someone else will care about setting up the session.

The reason why this does not work anymore is the change in SessionManager::startReal() which only will backup session data if the session actually was (lazy-)started before. This is more explicit and IMHO the correct behavior.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Can you explain the drupal_set_message() change a bit more? Calling this function will still open a session?

I Just found the tiniest possible nit.

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -129,53 +129,63 @@ public function startLazy() {
+      // Initialize the session gloabl and attach the Symfony session bags.

typo: gloabl

znerol’s picture

znerol CreditAttribution: znerol commented
FileSize
2338571-remove-session-manager-start-lazy-5.diff36.88 KB
2338571-remove-session-manager-start-lazy-5.diff36.88 KB

Can you explain the drupal_set_message() change a bit more?

First thing to note is that drupal_set_message() stores a message in the $_SESSION.

In Drupal 7 drupal_session_initialize() was called unconditionally during the bootstrap and all code running after DRUPAL_BOOTSTRAP_SESSION could expect that things just work. It even was possible to populate the $_SESSION before that bootstrap stage because drupal_session_start() would just merge preexisting data after starting the session.

In HEAD, the session currently is started as soon as something triggers authentication by calling some method of the current_user service. Under normal circumstances, this happens in the MaintenanceModeSubscriber. Basically we have the same behavior in HEAD like in D7, except that it is implicit. Granted this is very fragile and there is still a considerable amount of work to do in order to completely separate user authentication from the session code.

The patch changes the behavior slightly when starting a session. Instead of merging preexisting data it does the following: If a session was loaded from the disk, only data from the disk will be used. If a session was started lazily before, only preexisting data added to $_SESSION between the lazy start and the real start is kept.

The reason for giving up the unconditional data-merge in the startReal()-method is that due to the Symfony session bags, the content of $_SESSION is not flat anymore. It would be rather difficult to always merge the flat (Drupal) session data and the Symfony session bags correctly.

The session-merge is what changed in the patch, and the service provider test exposed that.

znerol’s picture

znerol CreditAttribution: znerol commented
FileSize
interdiff.txt580 bytes
moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

That all sounds reasonable to me. Would be great to get another eye on this before RTBC.

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented 
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -672,16 +672,13 @@ protected function getKernelParameters() {
-        if ($session_manager->isStartedLazy()) {
-          $session_manager_state |= 0x1;
-        }
         if ($session_manager->isStarted()) {
-          $session_manager_state |= 0x2;
+          $session_manager_started = TRUE;

YES! THIS!

This was pretty confusing the first time I saw it. So happy we're making this easier to understand.

+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -129,53 +129,63 @@ public function startLazy() {
+  protected function startReal() {

sorry to quibble but startReal makes me thinking your creating a new reality.

How about forceStart

otherwise the patch looks good to me.

znerol’s picture

znerol CreditAttribution: znerol commented
FileSize
2338571-remove-session-manager-start-lazy-9.diff36.88 KB
interdiff.txt1.75 KB

sorry to quibble but startReal makes me thinking your creating a new reality.

What about startNow()?

cosmicdreams’s picture

cosmicdreams CreditAttribution: cosmicdreams commented
Status: Needs review » Reviewed & tested by the community

SOLD

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 195503e and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Core/Session/SessionManager.php b/core/lib/Drupal/Core/Session/SessionManager.php
index ea6ba6e..64b8fdd 100644
--- a/core/lib/Drupal/Core/Session/SessionManager.php
+++ b/core/lib/Drupal/Core/Session/SessionManager.php
@@ -170,10 +170,13 @@ public function start() {
 
   /**
    * Forcibly start a PHP session.
+   *
+   * @return boolean
+   *   TRUE if the session is started.
    */
   protected function startNow() {
     if (!$this->isEnabled() || $this->isCli()) {
-      return;
+      return FALSE;
     }
 
     if ($this->startedLazy) {

I'm not 100% sold on startNow or startReal - but it is a protected method so I don't care too much. It should return a bool - just like parent::start(). Fixed on commit.

  • alexpott committed 195503e on 8.0.x 
    Issue #2338571 by znerol: Remove SessionManager::startLazy().

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.