Add the session to the request in KernelTestBase, BrowserTestBase, and drush

So should we setup a request by default with an empty session object inside $request?

@znerol
It would be great if you could explain why a blocker for a normal is critical.

debuggin #2478167-18: Generate proper value for sessionName property in BrowserTestBase I found that child site reports incremental amount of cookies which grows with every next test* method

Over in #2421503: SA-CORE-2014-002 forward port only checks internal cache we're trying to replace key/value storage of form state with session based and found that tests run from run-tests.sh don't always have the session on the request object (because php is cli) - would this help that?

for #3

Crazy idea: KeyValue is an interface, so for normal requests have the form state backed by a session-based key-value, while in test use a disk key/value. Interfaces are fun like that.

Maybe that will be enough, let's see how many tests will fail

It looks ready to go!

What about WebTestBase as per IS and title? The patch only covers KTB

Great! Let's ping @Berdir or @alexpott to review this.

So there's plenty of change to all the tests here that's not truly necessary.

+++ b/core/modules/block_content/tests/src/Functional/BlockContentCacheTagsTest.php
@@ -83,8 +83,10 @@ public function testBlock() {
     //   https://www.drupal.org/node/2367555 is fixed and the
     //   corresponding $request->isMethodCacheable() checks are removed from
     //   Drupal\Core\Render\Renderer.
+    $request = new Request();
+    $request->setSession($this->getSessionFromRequest());
     $request_stack = $this->container->get('request_stack');
-    $request_stack->push(new Request());
+    $request_stack->push($request);
     $this->container->get('renderer')->renderRoot($build);
     $request_stack->pop();

If I remove this from the patch then the test is not going to fail. I have two concerns here which are kind of in opposition:

• If the old code doesn't fail how is contrib and custom going to know how to "do it right"
• If the old code did fail then this would be an extremely disruptive change - given that it doesn't fail is the new code really "doing it right" or is it being done for the appearance of correctness?

I think what I'm driving at is the why of

Problem is that we need to populate the session on those requests as well.

from #25 is not answered.

Also this patch needs to apply to 9.0.x and it doesn't.

Reroll for 9.1.x and clean-up comment

Re-rolled for 9.2.x

The patch in #40 is failing due to a cspell check on the use of MOCKSESSID in MockSessionManager. This could be resolved by either changin the MOCKSESSID string to pass the checks or by adding MOCKSESSID to cspell's dictionary.txt. I chose the latter since dictionary.txt already contains two similar strings: sess and ssess and modifying the string would require additional changes elsewhere. Patch and inter diff attached.

Rerolled the patch in #41 and fixed custom command error. Thanks!

+++ b/core/tests/Drupal/KernelTests/Core/Session/MockSessionManager.php
@@ -0,0 +1,139 @@
+    $this->storage = new MockArraySessionStorage('MOCKSESSID', $metadata_bag);

How about using proper words here. ie. MOCK_SESSION_ID then we don't need to add something to the dictionary.

Changed MOCKSESSID to MOCK_SESSION_ID

$this->container->get('session') the only trickery prevents me from rtbc but I see no other way (except some request factory)

I found no other way

Please do not ask the testbot to try again until #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed. I have not reviewed the code.

I still have the concerns I outlined in #31. IN #32 @znerol addresses this concerns directly and I agree that if production code only has to check for session's existence due to test code then that would be a bug but what about CLIs like drush and console. They don't have a session either so are we in danger of making things more difficult for them?

+++ b/core/tests/Drupal/Tests/RequestSessionTestTrait.php
@@ -0,0 +1,33 @@
+      @trigger_error('Failed to retrieve session from current request, request stack was modified during test.', E_USER_WARNING);

Is this likely to cause contrib / custom tests to start failing? Do we want this or should we deprecate and then start failing in D10?

+1 to deprecate it in 9.5 and fail in 10.1

Re-roll and looks mostly no changes needed for tests nowadays

PS: see interdiff - few tests removed $request->setSession($this->getSessionFromRequest()); but passed

bit more clean-up

Fix CS

and one more

revert last useless test change

Left few comments, primary is to fix deprecation message according standard https://www.drupal.org/about/core/policies/core-change-policies/drupal-d...

43 failed tests! Great job!

Thanks, 👍 looks ready

I set it to Needs work due to unresolved threads in MR.

All review comments are addressed.

bump
As soon as will be merged, that will unblock #2529170: Remove DrupalKernel::initializeRequestGlobals and replace base_root, base_url and base_path with a service

bump

The change record for this issue has no text. Adding tag and setting to NW.

Apologies to all for my rather terse comment.

@znerol, thanks for updating the CR.

Setting this to NW was unintentional. Sorry for the noise.

CR looks good, thank you

MAINTAINERS.txt has this for the "base system" "subsystem" [sic]:

Base system
- ?

1. It's weird that the "base system" is considered a "subsystem" (out of scope for this issue, obviously). 😅
2. There is no dedicated subsystem maintainer for "base system". In this case, I think a "framework manager" review would be most appropriate.
3. @alexpott already reviewed, but let's be explicit that a framework manager reviews this and signs off before commit.

Doing a review of the MR. Found some issues. Will save that soon so they appear here, but downgrading to NW now...

Pushed a commit to fix all my own nits and the deprecation message typo. Also edited the CR to fix the same typo there. 😅 I'd resolve most of the threads I just opened (if I could). The two that should remain open until addressed are:

1. https://git.drupalcode.org/project/drupal/-/merge_requests/3320#note_168867
2. https://git.drupalcode.org/project/drupal/-/merge_requests/3320#note_168868

WHOOPS! Holy cross-posting and cross-pushing, batman. 😢 Sorry, @andypost! Was trying to resolve my own stuff, didn't see you were already responding. Yikes. I'll stop now and let the dust settle before I do anything else... 🙏

NP, As I see the only debatable moment left is https://git.drupalcode.org/project/drupal/-/merge_requests/3320#note_168867

I think it could be solved with #2613044: Requests are pushed onto the request stack twice, popped once

Per Slack chat w/ @andypost, we agreed a @todo/@see comment pointing to #2613044: Requests are pushed onto the request stack twice, popped once was worth adding to justify the switch to rebuildContainer()...

All new threads should now be resolved. This is ready for framework manager review, although I can't re-RTBC it now. 😅

Posted in #needs-review-queue-initiative to solicit the framework manager review this needs, so tagging for that. 😉

Re-queue'ed PHP 8.1 + MySQL 5.7 - random fail in

Ckeditor5.Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest

.

Needs followup: Opened #3355243: cspell does not flag "ist" as a typo to deal with the cspell vs. "ist" weirdness.

I think it ready and now more polished

Needs to change branch in MR, and update versions in deprecation messages 10.1.0 -> 10.2.0

Looks good to me.
The MR does what the IS states.
For me it is RTBC.

I forgot to change the status.

#3355243: cspell does not flag "ist" as a typo is merged in 11.x

This looks awesome. 👏 I could only find 3 language nits, for which I left suggestions.

applied suggestions. thanks

Sorry folks, The change record has an an incorrect branch, version and example deprecation message. And the deprecation message in the MR needs a change to conform to coding standards. I have left suggestions in the MR.

Addressed #112

MR is green, text is fixed

Raised this in committer Slack to hopefully get that needed FM feedback.

Both Request::create and Request::createFromGlobals end up calling Request::createFromFactory

I wonder if our test setup trait could call \Symfony\Component\HttpFoundation\Request::setFactory which would add a factory method that could do the session setup for all of those instances. It won't help new Request but those should probably be using ::create or ::createFromGlobals anyway.

It would mean the size of the change here is much smaller, and the need for contrib/custom projects to make chances would be reduced.

We could mention in the CR that in most cases they should switch to ::create or ::createFromGlobals instead of using new Request and setting the session.

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

Thank you @znerol that is a very thorough and eloquent explanation of why the disruption is actually better.

I think we probably want to target 10.3.0 with this now, which will mean we need to update the deprecation message. I'll confirm with release managers.

The MR needs to be updated for that this will go in 10.3 instead of 10.2

Addressed #122

Back to RTBC.

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

Hah, whoops, cross rebase. 😅 But no harm done.

Appear all threads have been addressed and no tests appear to be breaking.

CR is full of detail (branch versions need to be bumped)

Edited the CR to bump the versions to 10.3.0.

Went through this again, only found one out of scope change, that I reverted locally before committing.

@alexpott's concerns in #31, #32 and #59 were addressed by @znerol by a change of approach.

My point in #117 was addressed in #118

We know this could be disruptive so I think getting it in early in the 10.3 cycle is the best approach.

So on that basis, committed and pushed to 11.x

Published the change record.

Thanks everyone for the mammoth effort over a prolonged period here.

Special thanks to @znerol for your patient and thorough explanations when required.

Sorry folks, I had to revert this as it caused failure in HEAD

Looks like there's a new test that creates a Request but doesn't push a session

See \Drupal\Tests\file\Kernel\RequirementsTest::setServerSoftware

Fine to self RTBC

I'll keep an eye out for changes

Thanks, compared the two patches locally and confirmed the only difference was this

diff --git a/core/modules/file/tests/src/Kernel/RequirementsTest.php b/core/modules/file/tests/src/Kernel/RequirementsTest.php
index 4d472644dc4..224616e089b 100644
--- a/core/modules/file/tests/src/Kernel/RequirementsTest.php
+++ b/core/modules/file/tests/src/Kernel/RequirementsTest.php
@@ -7,6 +7,8 @@
 use Drupal\KernelTests\KernelTestBase;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\RequestStack;
+use Symfony\Component\HttpFoundation\Session\Session;
+use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage;
 
 /**
  * Tests the file requirements.
@@ -69,6 +71,7 @@ public function testUploadRequirements(): void {
    */
   private function setServerSoftware(?string $software): void {
     $request = new Request();
+    $request->setSession(new Session(new MockArraySessionStorage()));
     if (is_string($software)) {
       $request->server->set('SERVER_SOFTWARE', $software);
     }

Committed and pushed to 11.x, re-published the change record.

Thanks for the quick turnaround

Thank you! Removal of global session remains now unblocked #3109970: Convert uses of $_SESSION in forms and batch