Problem/Motivation

After #2372389: Expose session handler in container, disabling and enabling of session saving has moved to WriteSafeSessionHandlerInterface::setSessionWritable() while SessionManager::isEnabled(), ::enable() and ::disable() have been deprecated. This issue removes the usages of SessionManager::isEnabled(), ::enable() and ::disable() in core.

Proposed resolution

Replace in the following files. @znerol in #2372389-28: Expose session handler in container

Calls to those methods should be replaced by the respective methods of the write-safe handler. A quick grep through the source tree turned up the following files which need to be updated: 

core/lib/Drupal/Core/Session/AccountSwitcher.php
core/modules/system/src/Tests/Datetime/DrupalDateTimeTest.php
core/modules/system/src/Tests/Session/AccountSwitcherTest.php
core/modules/system/src/Tests/Session/SessionTest.php
core/modules/system/tests/modules/session_test/src/Controller/SessionTestController.php
core/modules/user/src/Tests/Views/ArgumentDefaultTest.php

Remaining tasks

Patch
Reviews
Commit

User interface changes

None

API changes

SessionManager::enable() and SessionManager::disable() will be removed. WriteSafeSessionHandlerInterface::setSessionWritable() is the new proper way of doing session write enabling/disabling.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Prioritized changes The main goal of this issue is switching away from previously deprecated code.
Disruption Moderately disruptive for contributed and custom modules because it will require a minor internal refactoring
CommentFileSizeAuthor
#21 interdiff.txt4.53 KBalmaudoh
#21 2426031-remove-deprecated-enable-disable-21.patch10.74 KBalmaudoh
#15 interdiff.txt3.6 KBalmaudoh
#15 2426031-remove-deprecated-enable-disable-15.patch10.48 KBalmaudoh
#9 2426031-remove-deprecated-enable-disable-9.patch11.79 KBcpj
#5 2426031_remove-deprecated-enable-disable-5.patch11.8 KBcpj
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented
Status: Active » Postponed

Postponed until #2372389: Expose session handler in container is in.

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented
Status: Postponed » Active

#2372389: Expose session handler in container is in so we can unpostpone this issue.

cpj’s picture

cpj CreditAttribution: cpj commented
Issue summary: View changes
cpj’s picture

cpj CreditAttribution: cpj commented
Issue summary: View changes
cpj’s picture

cpj CreditAttribution: cpj commented
FileSize
2426031_remove-deprecated-enable-disable-5.patch11.8 KB
cpj’s picture

cpj CreditAttribution: cpj commented
Status: Active » Needs review
cpj’s picture

cpj CreditAttribution: cpj commented
Title: Remove deprecated uses of SessionManager::enable() and SessionManager::disable() » Remove deprecated uses of SessionManager::isEnabled(), SessionManager::enable() and SessionManager::disable()

Status: Needs review » Needs work

The last submitted patch, 5: 2426031_remove-deprecated-enable-disable-5.patch, failed testing.

cpj’s picture

cpj CreditAttribution: cpj commented
FileSize
2426031-remove-deprecated-enable-disable-9.patch11.79 KB
cpj’s picture

cpj CreditAttribution: cpj commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: 2426031-remove-deprecated-enable-disable-9.patch, failed testing.

cpj’s picture

cpj CreditAttribution: cpj commented

@almaudoh - I've made the changes & submitted a patch. I don't understand the errors. Can you help ? Thanks

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented

Some small nitpicks. 

  1. +++ b/core/lib/Drupal/Core/Session/AccountSwitcher.php
@@ -31,11 +31,11 @@ class AccountSwitcher implements AccountSwitcherInterface {
+   * @var \Drupal\Core\Session\WriteCheckSessionHandlerInterface

    s/WriteCheckSessionHandlerInterface/WriteSafeSessionHandlerInterface/ 

  2. +++ b/core/modules/user/src/Tests/Views/ArgumentDefaultTest.php
@@ -39,7 +39,7 @@ public function test_plugin_argument_default_current_user() {
diff --git a/core/vendor/bin/phpunit b/core/vendor/bin/phpunit

diff --git a/core/vendor/bin/phpunit b/core/vendor/bin/phpunit
deleted file mode 120000

deleted file mode 120000
index 2c48930..0000000

index 2c48930..0000000
--- a/core/vendor/bin/phpunit

--- a/core/vendor/bin/phpunit
+++ /dev/null

+++ /dev/null
+++ /dev/null
@@ -1 +0,0 @@

@@ -1 +0,0 @@
-../phpunit/phpunit/phpunit
\ No newline at end of file
diff --git a/core/vendor/bin/phpunit b/core/vendor/bin/phpunit

diff --git a/core/vendor/bin/phpunit b/core/vendor/bin/phpunit
new file mode 100755

new file mode 100755
index 0000000..11f3cd5

index 0000000..11f3cd5
--- /dev/null

--- /dev/null
+++ b/core/vendor/bin/phpunit

+++ b/core/vendor/bin/phpunit
+++ b/core/vendor/bin/phpunit
@@ -0,0 +1,36 @@

@@ -0,0 +1,36 @@
+#!/usr/bin/env php
+<?php
+/*
+ * This file is part of PHPUnit.

    Out of scope

Checking the test fails now.

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented

Thanks @cpj for the patch. These are possibly the causes of the test fails, in addition to the phpunit file move mentioned in #13

+++ b/core/modules/system/src/Tests/Session/SessionTest.php
@@ -29,10 +29,10 @@ class SessionTest extends WebTestBase {
+    $this->assertFalse($session_handler->setSessionWritable(FALSE)->isSessionWritable(), '$session_handler->setSessionWritable(FALSE)->isSessionWritable() returns FALSE after disabling.');
+    $this->assertTrue($session_handler->setSessionWritable(TRUE)->isSessionWritable(), '$session_handler->setSessionWritable(TRUE)->isSessionWritable() returns TRUE after enabling.');

+++ b/core/modules/user/src/Tests/Views/ArgumentDefaultTest.php
@@ -30,7 +30,7 @@ public function test_plugin_argument_default_current_user() {
+    $session_handler = \Drupal::service('session_handler.write_safe')->setSessionWritable(FALSE);

WriteSafeSessionHandlerInterface::setSessionWritable() is not chainable. (I'm wondering maybe we should make it chainable in this issue).

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented
Status: Needs work » Needs review
FileSize
2426031-remove-deprecated-enable-disable-15.patch10.48 KB
interdiff.txt3.6 KB

Attached patch for #13 and #14

cpj’s picture

cpj CreditAttribution: cpj commented

Thanks @almaudoh - not sure where that phpunit stuff came from... It would be more like a drop-in replacement if it was chainable but I personally don't think its a "must-have". Or is there a "Best Practice" that says it should be chainable ?

#15 works in my system, but I guess we need someone else to do the RTBC ?

znerol’s picture

znerol CreditAttribution: znerol commented
Issue summary: View changes 
+++ b/core/modules/system/src/Tests/Session/SessionTest.php
@@ -29,10 +29,12 @@ class SessionTest extends WebTestBase {
    * Tests for \Drupal\Core\Session\SessionManager::isEnabled() and ::regenerate().

Comment should be updated too. Apart from that the patch is ready.

znerol’s picture

znerol CreditAttribution: znerol commented

Not sure whether we need a change record here. This functionality of the session manager has not been mentioned in the CR discussing the session manager. The one for the account switcher suggest that direct calls to drupal_save_session are not necessary anymore.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

I hate to be a stickler but the title and summary mention code being deprecated. I couldn't find documentation of this and the interface doesn't document it either. I agree with the issue but we should clarify the state of that code somewhere. A quick @deprecated with when it will be removed on SessionManagerInterface would address this for me.

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented

@neclimdul, those methods were deprecated by #2372389: Expose session handler in container.

I couldn't find documentation of this and the interface doesn't document it either.

There's nothing like a change notice if that's what you mean. But it's documented on the interface.

Do we need a change notice for this?

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented
FileSize
2426031-remove-deprecated-enable-disable-21.patch10.74 KB
interdiff.txt4.53 KB

Updates docs nits, renames $sessionHandler to $writeSafeHandler and changes ArgumentDefaultTest to use account_switcher service instead.

DrupalDateTimeTest also needs to be updated to use account_switcher service, but that bit of code is also being modified in a different way by #2328645: Remove remaining global $user.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

Sorry, I don't even know what I was looking at you are correct. Please ignore #19

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs review » Reviewed & tested by the community

Looked over this a couple more times and can't really see any problems. Looks good!

alexpott’s picture

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

Committed 7e41973 and pushed to 8.0.x. Thanks!

This is part of the on-going work on sessions - see #2372389: Expose session handler in container

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 7e41973 on 8.0.x 
    Issue #2426031 by almaudoh, cpj: Remove deprecated uses of...
neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

is there a follow up to follow for actually removing the methods from the interface?

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented

Created #2429103: Remove deprecated methods SessionManager::enable(), SessionManager::disable() and SessionManager::isEnabled()

Status: Fixed » Closed (fixed)

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