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

Re #59

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?

We could add a session object backed by an in-memory store in DrupalKernel::preHandle(). This is sufficient for command line tools and tests. Web requests will be processed by \Drupal\Core\StackMiddleware\Session where the mock session is replaced by a session object backed with persistent storage and a real session handler.

This works with current drush php:script and the following snipped:

var_dump(\Drupal::request()->hasSession());

The last submitted patch, 70: 2484991-add-the-session-70.patch, failed testing. View results →

Session stack middleware should avoid attempting to save a session it didn't start.

Re #65

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

That is expected. All those calls were added for consistency reasons. The goal in this issue is that every Request in core has a Session. We only can hope that people start copying the $request->setSession($this->getSessionFromRequest()); pattern if it is applied consistently throughout core.

Switching this issue to MR workflow. I'm probably going to test another idea in a separate branch as well.

The last submitted patch, 72: 2484991-72.patch, failed testing. View results →

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!

Updated the issue summary in order to reflect the new approach.

znerol → closed merge request !3317

Thanks, 👍 looks ready

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

Thanks. Extracted two methods for session initialization in order to better document their responsibility and purpose:

• DrupalKernel::initializeEphemeralSession()
• StackMiddleware\Session::initializePersistentSession()

All review comments are addressed.

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

bump

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

Added the CR.

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 🐛 Requests are pushed onto the request stack twice, popped once Needs work

Per Slack chat w/ @andypost, we agreed a @todo/@see comment pointing to 🐛 Requests are pushed onto the request stack twice, popped once Needs work 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

.

: Opened 📌 cspell does not flag "ist" as a typo Fixed 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

Rebased on 11.x and adapted the deprecation messages.

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

I forgot to change the status.

📌 cspell does not flag "ist" as a typo Fixed 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

Rerolled.

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

While maybe convenient, I think that #117 will create more problems than it solves.

In Drupal 7, application code modified the $_SESSION superglobal directly. Every piece of code which accessed it was guaranteed to work with the same data. However, using the $_SESSION directly breaks encapsulation and makes testing difficult. Missing test coverage was probably one of the reasons why 🐛 Feature "Remember the last selection" for views exposed filters doesn't work anymore Fixed went unnoticed for quite a long time.

In times where $_SESSION was used exclusively, the responsibility to populate and store its contents was SessionManager.php alone (or session.inc in D7). Now that almost all core code is refactored to use the Request::getSession(), there are additional code paths involved in passing data form the PHP session to the places where its used and back again.

Basically every piece of code which creates, clones, duplicates a request also has the responsibility to copy the session. It is true that many of those are located in test cases, but some can be found in production code paths as well. The latter have the potential to introduce subtle bugs if they do not propagate the session. When everybody used $_SESSION this wasn't an issue. Nowadays it is.

I fear that installing a request factory in the test system will hide those potential bugs. Code which fabricates a request in order to pass that into non-trivial code paths without propagating the session will seemingly behave correctly in tests (because of the request factory) and will produce invalid requests (i.e., lacking a session) in production.

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.

larowlan → closed merge request !3320

larowlan → reopened merge request !3320

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

Fine to self RTBC

Thanks @larowlan. Done.

Oh, I might have overwritten work done by @voleger and @dww by basing off my changes on an outdated local branch. Let me fix that.

MR should be up-to-date now.

larowlan → closed merge request !3320

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 📌 Convert uses of $_SESSION in forms and batch Needs work

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