Problem/Motivation

All callbacks in the session manager that start, save or regenerate a session check whether the application runs in a cli environment and if that is the case no session is actually started. The destroy callback lacks this check.

Steps to reproduce

I've put together a simple browser test that that displays this behaviour.

Proposed resolution

Add the if ($this->isCli()) { check to the destroy method.

CommentFileSizeAuthor
#6 interdiff.txt1.39 KBraman.b
#6 3172757-6.patch1.34 KBraman.b
session_destroy_from_cli_with_test.patch1.38 KBErik Frèrejean
session_destroy_from_cli_fix_only.patch477 bytesErik Frèrejean
session_destroy_from_cli_current_behavior.patch935 bytesErik Frèrejean
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Erik Frèrejean created an issue. See original summary.

The last submitted patch, session_destroy_from_cli_current_behavior.patch, failed testing. View results

Erik Frèrejean’s picture

Erik Frèrejean
Primary language Dutch
Location 🏴‍☠️
CreditAttribution: Erik Frèrejean at Dutch Open Projects commented
Title: Session manager destroy doesn't check for cli » Session manager destroy misses isCli check
liesm’s picture

liesm CreditAttribution: liesm as a volunteer commented
Status: Needs review » Reviewed & tested by the community

That seems like a reasonable change and with the provided tests being green this can be RTBC'ed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/tests/Drupal/FunctionalTests/SessionManagerDestroyNoCliCheckTest.php
@@ -0,0 +1,27 @@
+namespace Drupal\FunctionalTests;

    Should be moved to core/tests/Drupal/KernelTests/Core/Session directory and have namespace Drupal\KernelTests\Core\Session; as a namespace. 

  2. +++ b/core/tests/Drupal/FunctionalTests/SessionManagerDestroyNoCliCheckTest.php
@@ -0,0 +1,27 @@
+ * @group browsertestbase

    @group Session

  3. +++ b/core/tests/Drupal/FunctionalTests/SessionManagerDestroyNoCliCheckTest.php
@@ -0,0 +1,27 @@
+class SessionManagerDestroyNoCliCheckTest extends BrowserTestBase {

    Can be a Kernel test - ie extend from KernelTestBase 

  4. +++ b/core/tests/Drupal/FunctionalTests/SessionManagerDestroyNoCliCheckTest.php
@@ -0,0 +1,27 @@
+   * Test case that starts and stops a session from the CLI.

    Tests starting and destroying a session from the CLI.

  5. Apart from theoretical consistency is there any reason for these issue? The reason I ask is because the only way we can really end up in session code (when used properly) is when we're not in the CLI. See \Drupal\Core\StackMiddleware\Session::handle().
raman.b’s picture

raman.b CreditAttribution: raman.b at OpenSense Labs commented
Version: 9.0.x-dev » 9.1.x-dev
Status: Needs work » Needs review
FileSize
3172757-6.patch1.34 KB
interdiff.txt1.39 KB

Addressing pointers from #5

Not sure if it's relevant in CLI context, but there is one usage of \Drupal::service('session_manager')->destroy(); in user_logout() other than Drupal\Core\Session\SessionManager itself.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Erik Frèrejean’s picture

Erik Frèrejean
Primary language Dutch
Location 🏴‍☠️
CreditAttribution: Erik Frèrejean at Dutch Open Projects commented
Status: Needs review » Reviewed & tested by the community

@alexpott

Apart from theoretical consistency is there any reason for these issue? The reason I ask is because the only way we can really end up in session code (when used properly) is when we're not in the CLI. See \Drupal\Core\StackMiddleware\Session::handle().

Well consistency is one. The main reason I found this was when creating tests for a bit of logic that forces a logout before continuing. We call user_logout() at that point, which in turn hard calls \Drupal::service('session_manager')->destroy();. Logging in a user from the CLI isn't a problem because all those methods have the if ($this->isCli()) { construct. So you'll end up with a user that identifies as authenticated but without a session. 

if ($this->currentUser->isAuthenticated()) {
  user_logout();
}

The alternative is that we check for the existence of a session but IMO that should be responsibility of the session manager, especially because it already handles that case for all other session calls.

@raman.b thanks for the update.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

I'd love to see a SessionManager without all the isCli checks - but in lieu of that the consistency argument is persuasive.

Committed and pushed eb75aa3d3d to 9.2.x and 8146809d28 to 9.1.x. Thanks!

  • alexpott committed eb75aa3 on 9.2.x 
    Issue #3172757 by Erik Frèrejean, raman.b: Session manager destroy...

  • alexpott committed 8146809 on 9.1.x 
    Issue #3172757 by Erik Frèrejean, raman.b: Session manager destroy...

Status: Fixed » Closed (fixed)

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