Convert uses of $_SESSION in forms and batch

Drupal 9.2.0-alpha1 → will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

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 → .

Drupal 8.9.0-beta1 → was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened → , as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.5.0-beta2 → and Drupal 10.0.0-beta2 → were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.4.0-alpha1 → was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.3.0-rc1 → was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

1 test failed

Test fails in BigPipe, this is going to be a long day...

So, CSRF $seed is null in the test runner. BT:

array (
  0 => 
  array (
    'file' => './core/lib/Drupal/Core/Access/CsrfTokenGenerator.php',
    'line' => 65,
    'function' => 'unknown',
  ),
  1 => 
  array (
    'file' => './core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipePlaceholderTestCases.php',
    'line' => 154,
    'function' => 'get',
    'class' => 'Drupal\\Core\\Access\\CsrfTokenGenerator',
    'type' => '->',
  ),
  2 => 
  array (
    'file' => './core/modules/big_pipe/tests/src/Functional/BigPipeTest.php',
    'line' => 437,
    'function' => 'cases',
    'class' => 'Drupal\\big_pipe_test\\BigPipePlaceholderTestCases',
    'type' => '::',
  ),
  3 => 
  array (
    'file' => './core/modules/big_pipe/tests/src/Functional/BigPipeTest.php',
    'line' => 165,
    'function' => 'getTestCases',
    'class' => 'Drupal\\Tests\\big_pipe\\Functional\\BigPipeTest',
    'type' => '->',
  ),
...

This means that before the changes in batch.inc and FormBuilder.php, the session (contents / session id) was shared between the test runner and the test site. After eliminating references to the $_SESSION global in production code, this is not the case anymore.

So, there is what happens. This is an excerpt of BigPipeTest::testBigPipe():

    $this->setCsrfTokenSeedInTestEnvironment();
    $cases = $this->getTestCases();

The first line extracts the CSRF token seed generated in the child site from the session record in the database table. Then it sets the CSRF seed via session_manager.metadata_bag service in the test runner.

The call in the second line ends up in BigPipePlaceholderTestCases::cases() where test data and expectations are constructed.

Among other things this function calls into the form_builder service in order to construct the Drupal\big_pipe_test\Form\BigPipeTestForm. Also it calls into RouteProcessorCsrf::renderPlaceholderCsrfToken in order to generate the token. That token is compared to the results retrieved from the child site.

With the changes in the MR, the call into the session service via FormBuilder.php implicitly starts a session. That in turn will initialize the session metadata bag, and as a result the CSRF seed will be empty.

One very simple way to work around this problem is to simply swap the test case construction in BigPipePlaceholderTestCases::cases().

Note that this scenario isn't likely to cause problems in production. This is because the session is initialized unconditionally in the session middleware. Hence, access to the session from places like the FormBuilder will not trigger implicit initialization.

Note that this scenario isn't likely to cause problems in production

Not sure as contrib/custom code probably can cause session initialization, for example masquerade module but it needs to check manually

Not sure as contrib/custom code probably can cause session initialization, for example masquerade module but it needs to check manually

True. Core code can cause session initialization as well. But that only may lead to problems if all of the following conditions are met:

• The request did not go through the Session middleware and hence didn't get initialized.
• Something tries to set a value before a session initialization was caused by random core/contrib/custom code.
• Something tries to use that value after a session initialization was caused by random core/contrib/custom code.

The pointed issue is RTBC so how to solve chicken/egg problem?

If the other issue lands before this one, then the MR from this issue needs to be updated. Or the other way around.

All changes look good to me.
The MR does what the IS states.
For me it is RTBC.

Needs work for one small comment addition to ensure we understand that there is a reason the BigPipe test cases have been flipped.

Also, AFAICT 📌 Add the session to the request in KernelTestBase, BrowserTestBase, and drush Fixed would ideally land first, because there are only @todos HERE referencing the other issue, not the other way around.

Rebased and removed TODOs about commited 📌 Add the session to the request in KernelTestBase, BrowserTestBase, and drush Fixed

Views tests still needs some love and consistency in 📌 Use $this->request in ViewsExecutable::getExposedInput() consistently Needs review

All the code changes look good to me.
For me it is RTBC.

Unaddressed MR thread that seems legit.

Prashant.c → made their first commit to this issue’s fork.

Thanks @Prashant.c, back to RTBC (identical to #24).

Whoops. 😅 in that case, Let’s open the follow up and add the @todo comment pointing it to it as part of this issue.

Opened the follow-up 📌 Remove calls to Request::hasSession() Active .

Added @todo

The requested TODO has been added to the correct issue.
Back to RTBC.

+1 for RTBC changes looks good.

This issue created a followup with an @todo in the MR, I am adding that issue as a related item.

I did not find any unanswered questions. I updated credit.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Reroll succeeded without conflicts.

Restoring status from #34 as reroll seems fine.

Couple of questions on the MR.

Still appears to be one thread

Making it more clear that this needs input from somebody working on the MR for this to proceed.

The reordering explained in #16 maybe comment should be paraphrased somehow but the thing is that session now living in both testing and tester sites.

So the session started in test via CSRF seed now should go before its usage

Let's try to keep the order of test cases. Instead, ensure that a session is started before storing the CSRF value in the session bag on the test runner.

So question, could this change be a breaking change for contrib modules? Saw that some core modules had to be updated

Good question.

The following command lists changes in production code. The one in LanguageNegotiationSession could be pushed to 📌 Remove calls to Request::hasSession() Active . But since the comment needs to be updated to point to the correct issue anyway, it doesn't hurt to fix it right here I think.

% COLUMNS=999 git diff `git merge-base HEAD 11.x` --stat | grep -vi test
 core/includes/batch.inc                                                             | 16 ++++++++++------
 core/includes/form.inc                                                              | 14 +++++++++-----
 core/lib/Drupal/Core/Form/FormBuilder.php                                           |  8 +++++---
 core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationSession.php |  4 +---

The rest of the changes are in tests, those are mostly necessary due to 📌 Add the session to the request in KernelTestBase, BrowserTestBase, and drush Fixed . Plus the CSRF fix in BigPipe.

% COLUMNS=999 git diff f8a2ccac36a8402ec404e4b7c421e073fcf0d052 --stat | grep -i test
 core/modules/big_pipe/tests/src/Functional/BigPipeTest.php                          | 15 +++++++++++++++
 core/modules/user/tests/src/Kernel/UserAccountFormPasswordResetTest.php             |  9 +--------
 core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php                               |  4 ++++
 core/tests/Drupal/Tests/Core/Form/FormTestBase.php                                  |  3 +++

With the current MR we are down to zero references to the $_SESSION superglobal (except where it is required in SessionManager and SessionTestController).

% git grep -l '$_SESSION'
core/lib/Drupal/Core/Session/SessionManager.php
core/modules/system/tests/modules/session_test/src/Controller/SessionTestController.php

Makes sense I think? The replacement seems fine I was just concerned about failing tests abruptly in contrib but think you answered. Rest looks fine.

Committed and pushed 9334d6f100 to 11.x and 173217df7e to 10.3.x. Thanks!

znerol → closed merge request !4407

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