Convert uses of $_SESSION to symfony session retrieved from the request

Initial patch, converted: batch, authorize session vars

Suppose we need to split this one into sub-issues on per session key usage

The related issue postponed til 9.x so I think we should convert usage here

all failures are caused by
Fatal error: Call to a member function has() on a non-object in /.../sites/default/files/checkout/core/lib/Drupal/Core/Form/FormBuilder.php on line 193

No reason for this to be postponed

Did some work on this... there are still two files with runtime $_SESSION usages. And lots of comments.

Whoops.

Some fixes.

This is a task and therefore for 8.6.x

Some more fixes.

Here's the last fix. The only usages of $_SESSION left are in \Drupal\Core\Messenger\LegacyMessenger which is deprecated and I think we can't really touch. And \Drupal\Core\Session\SessionManager where I think we need to very carefully think about what we change. Imo I think we could do that in a follow-up as I think we'll need to add tests for legacy $_SESSION support.

Some minor tidy-ups after reviewing the patch.

Fixing all the comments - mostly $_SESSION becomes the user's session

As few places are refactored maybe use to normalize calls to hasSession() & operations on session data... otoh it looks more like follow-up

1. +++ b/core/includes/database.inc
   @@ -1039,12 +1039,12 @@ function db_ignore_replica() {
   +  if (count($connection_info) > 1 && \Drupal::request()->hasSession()) {
   ...
   -    $_SESSION['ignore_replica_server'] = REQUEST_TIME + $duration;
   +    \Drupal::request()->getSession()->set('ignore_replica_server', REQUEST_TIME + $duration);
   
   +++ b/core/lib/Drupal/Core/EventSubscriber/ReplicaDatabaseIgnoreSubscriber.php
   @@ -34,12 +34,13 @@ public function checkReplicaServer(GetResponseEvent $event) {
   +    if ($request->hasSession() && $request->getSession()->has('ignore_replica_server')) {
   +      if ($request->getSession()->get('ignore_replica_server') >= REQUEST_TIME) {
   ...
   +        $request->getSession()->remove('ignore_replica_server');
   

   Affects #2286235: Deprecate db_ignore_replica() and convert it to service

2. +++ b/core/modules/system/tests/modules/menu_test/src/Access/AccessCheck.php
   @@ -17,12 +35,9 @@ class AccessCheck implements AccessInterface {
   -    if (!isset($_SESSION['menu_test'])) {
   -      $result = AccessResult::allowed();
   ...
   -      $result = AccessResult::allowedIf($_SESSION['menu_test'] < 2);
   ...
   +    $result = AccessResult::allowedIf(
   +      $this->requestStack->getMasterRequest()->getSession()->get('menu_test', 0) < 2
   

   why master request, looks other checks & cache contexts use current request

Re #28

1. Sure whichever gets in first means the second one will need a re-roll. The changes in both issues are very similar so no harm done.
2. This can't matter as we're not going to be calling this test access handler in a sub request but changed anyway as I have no reason why I went for getMasterRequest()

Fixes some test fails.

Hmm. Seem to have uncovered an issue with BigPipe...

Not sure what this means:

Fatal error: Uncaught session_write_close(): Failed to write session data using user defined save handler. (session.save_path: /tmp)

It means that comment test creates session but something now trying to save it

Update #38

Fix issues and update patch.

Clean-up against #38
Comment test fails because of placeholder generation for comment form, guess big pipe fails for the same reason
Still not clear how to fix

@amit.drupal no reason to change legacy manager & session manager because they are backward compatibility

Fix comment test - accessing not started session with has() or remove() caused this bug

Proper fix

Needs a reroll

context changed, reroll

meantime, probably batch needs own session bag, also few checks for session started and exists, still confusing

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -535,6 +535,11 @@ public function register(ContainerBuilder $container) {
+      ->register('session_manager.memory', 'Drupal\Core\Session\SessionManagerMemory')

I'd better keep this class out of core if it's used only in tests

It looks great, just noted that views plugin could use request from a view's method...
Will check it deeper cos when pager was converted we hit few places with workarounds

Here's clean-up for calls and views, file one more follow-up #3109109: AccountForm should read pass-reset-token only from query string

1. +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -122,9 +126,9 @@ public static function getLogLevelClassMap() {
   +  public function overview(Request $request) {
   
   @@ -306,12 +310,16 @@ public function eventDetails($event_id) {
   +  protected function buildFilterQuery(Request $request) {
   
   +++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
   @@ -12,12 +13,14 @@ class MigrateController extends ControllerBase {
   +  public function showLog(Request $request) {
   
   +++ b/core/modules/user/src/Controller/UserController.php
   @@ -223,7 +225,7 @@ public function getResetPassForm(Request $request, $uid) {
   -  public function resetPassLogin($uid, $timestamp, $hash) {
   +  public function resetPassLogin($uid, $timestamp, $hash, Request $request) {
   

   The only changes with BC affected, but looks they are totally valid

2. +++ b/core/tests/Drupal/KernelTests/Core/Database/ReplicaKillSwitchTest.php
   @@ -27,12 +26,14 @@ public function testSystemInitIgnoresSecondaries() {
   +    $request = \Drupal::request();
   +    $request->setSession(\Drupal::service('session'));
   ...
   -    $event = new GetResponseEvent($kernel, Request::create('http://example.com'), HttpKernelInterface::MASTER_REQUEST);
   +    $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST);
   

   this change is not clear - why URL removed in previous hunk

If the patch is ready then can we please update the issue summary?

Needs reroll after #3101108: Dblog event page for non-existing event returns 200, should be 404

reroll

1) I have visually scanned the patch. All the conversions are clean... I see dependency injection where appropriate.

2) The text of all converted comments looks ok to me.

3) I have looked at the testbot output no additional coding standard errors introduced.

4) I searched an could not find any more instances of $SESSION in the codebase.

5) I think the convention of dropping $SESSION variables is a really good idea.
Plus in a really hand-wavey way it is an essential first step if we want ReactPHP style persistent threads.. in the future - and what ever approach we take. [ dreaming of Drupal22 ]

I think this is ready

+++ b/core/modules/dblog/src/Controller/DbLogController.php
@@ -123,9 +127,9 @@ public static function getLogLevelClassMap() {
-  public function overview() {
+  public function overview(Request $request) {

@@ -314,12 +318,16 @@ public function eventDetails($event_id) {
-  protected function buildFilterQuery() {
-    if (empty($_SESSION['dblog_overview_filter'])) {
+  protected function buildFilterQuery(Request $request) {

+++ b/core/modules/migrate_drupal_ui/src/Controller/MigrateController.php
@@ -12,12 +13,14 @@ class MigrateController extends ControllerBase {
-  public function showLog() {
-    $_SESSION['dblog_overview_filter'] = [];
-    $_SESSION['dblog_overview_filter']['type'] = ['migrate_drupal_ui' => 'migrate_drupal_ui'];
+  public function showLog(Request $request) {
+    $request->getSession()->set('dblog_overview_filter', ['type' => ['migrate_drupal_ui' => 'migrate_drupal_ui']]);

+++ b/core/modules/user/src/Controller/UserController.php
@@ -219,7 +221,7 @@ public function getResetPassForm(Request $request, $uid) {
-  public function resetPassLogin($uid, $timestamp, $hash) {
+  public function resetPassLogin($uid, $timestamp, $hash, Request $request) {

We need a CR to cover this controller change. As controllers they don't have a BC promise so we can make these changes but it is important that we announce them.

Other than the missing CR this change looks great! Once the CR exists for adding the request to these controllers then we're good to go.

Created draft change record: https://www.drupal.org/node/3115716

It should be commited to 9.1 first

Re-roll for 9.1 and fix pass by reference

CR looks good to me.

Addressing the remaining test failure

+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -396,6 +396,12 @@ protected function results(Request $request) {
+    // Retrieve and remove session information.
+    $session = $request->getSession();
+    $update_results = $session->remove('update_results');
+    $update_success = $session->remove('update_success');
+    $session->remove('update_ignore_warnings');
+

I find the use of remove() to get the values a bit weird, but that's a nitpick.

+++ b/core/modules/dblog/src/Controller/DbLogController.php
@@ -123,9 +127,9 @@ public static function getLogLevelClassMap() {
    * @see Drupal\dblog\Controller\DbLogController::eventDetails()
    */
-  public function overview() {
+  public function overview(Request $request) {
 

This is the closest thing in the patch to an API change. The only case this could cause an issue is if someone subclassed the controller, and then called parent::overview(), I can't see this being done (much less likely than with a plugin implementation), and even if it is, it's well within @internal policy to change in a minor release. We have an change record already, so that's fine.

I find the use of remove() to get the values a bit weird, but that's a nitpick.

Same here, but also ::get() followed by ::remove() seems redundant, so best left as-is I think.

Committed 492b7a1 and pushed to 9.3.x. Thanks!

@catch CR needs to be published)

Sorry for the noise but we have a good report about this issue causing a regression #3260652: Feature "Remember the last selection" for views exposed filters doesn't work anymore