Problem/Motivation
Now that Symfony sessions are in place and available from $request->getSession()
, core should be updated that it uses that instead of accessing $_SESSION
directly. Benefits for doing that is a) code depending on the session will get easier to test b) Session::get()/Session::set()
takes care of starting the session automatically. That way we can eliminate direct access to the session_manager
in most places.
Proposed resolution
Fix any occurence of the $_SESSION
superglobal. Except in the following cases:
No change (legacy code):
core/lib/Drupal/Core/Messenger/LegacyMessenger.php
core/modules/simpletest/src/TestBase.php
LegacyMessenger will be removed in #2928994: Remove \Drupal\Core\Messenger\LegacyMessenger
No change (SessionManager and compatibility tests for $_SESSION superglobal):
core/lib/Drupal/Core/Session/SessionManager.php
core/modules/system/tests/modules/session_test/src/Controller/SessionTestController.php
core/modules/system/tests/modules/session_test/src/EventSubscriber/SessionTestSubscriber.php
core/modules/system/tests/src/Functional/Session/SessionTest.php
Other issue: #3109970: Convert uses of $_SESSION in forms and batch
core/includes/batch.inc
core/includes/form.inc
core/lib/Drupal/Core/Form/FormBuilder.php
Other issue: #3109042: Convert uses of $_SESSION in update module and authorize subsystem
core/authorize.php
core/lib/Drupal/Core/FileTransfer/Form/FileTransferAuthorizeForm.php
core/modules/system/system.module
core/modules/system/tests/src/Functional/System/SystemAuthorizeTest.php
core/modules/update/src/Form/UpdateReady.php
core/modules/update/update.authorize.inc
core/modules/update/update.manager.inc
Other issue: #3109600: Convert uses of $_SESSION in language module
core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationSession.php
Other issue: #3109971: Use $this->request in ViewsExecutable::getExposedInput() consistently
core/modules/views/src/ViewExecutable.php
Remaining tasks
ScopePatch- Review
- Commit
User interface changes
API changes
+++ b/core/modules/dblog/src/Controller/DbLogController.php
@@ -122,9 +126,9 @@ public static function getLogLevelClassMap() {
- public function overview() {
+ public function overview(Request $request) {
@@ -306,12 +310,16 @@ public function eventDetails($event_id) {
- protected function buildFilterQuery() {
+ 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() {
+ 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) {
Comment | File | Size | Author |
---|---|---|---|
#102 | interdiff_100-102.txt | 539 bytes | raman.b |
#102 | 2473875-102.patch | 37.94 KB | raman.b |
Comments
Comment #1
znerol CreditAttribution: znerol commentedVery old related issue: #2473875: Convert uses of $_SESSION to symfony session retrieved from the request
Comment #2
znerol CreditAttribution: znerol commentedComment #3
andypostInitial patch, converted: batch, authorize session vars
Suppose we need to split this one into sub-issues on per session key usage
Comment #4
andypostThe related issue postponed til 9.x so I think we should convert usage here
Comment #6
andypostall 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
Comment #7
znerol CreditAttribution: znerol commentedThat's likely because there is no session object on the request in the simpletest parent site. Seems like it is necessary to fix base test classes before.
Comment #8
znerol CreditAttribution: znerol commentedPostponing on #2484991: Add the session to the request in KernelTestBase, BrowserTestBase, and drush.
Comment #9
mgiffordComment #15
alexpottNo reason for this to be postponed
Comment #16
alexpottDid some work on this... there are still two files with runtime $_SESSION usages. And lots of comments.
Comment #18
alexpottWhoops.
Comment #20
alexpottSome fixes.
Comment #21
alexpottThis is a task and therefore for 8.6.x
Comment #23
alexpottSome more fixes.
Comment #25
alexpottHere'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.Comment #26
alexpottSome minor tidy-ups after reviewing the patch.
Comment #27
alexpottFixing all the comments - mostly
$_SESSION
becomesthe user's session
Comment #28
andypostAs few places are refactored maybe use to normalize calls to hasSession() & operations on session data... otoh it looks more like follow-up
Affects #2286235: Deprecate db_ignore_replica() and convert it to service
why master request, looks other checks & cache contexts use current request
Comment #29
alexpottRe #28
Comment #32
znerol CreditAttribution: znerol commentedReroll for 8.8.x. This had lots of merge conflicts because of #2935254: Remove all usages of drupal_get_message and drupal_set_message in core includes.
Comment #34
znerol CreditAttribution: znerol commentedFix some bad merges.
Comment #36
kim.pepperFixes some test fails.
Comment #38
kim.pepperHmm. Seem to have uncovered an issue with BigPipe...
Comment #40
kim.pepperNot sure what this means:
Fatal error: Uncaught session_write_close(): Failed to write session data using user defined save handler. (session.save_path: /tmp)
Comment #41
andypostIt means that comment test creates session but something now trying to save it
Comment #42
amit.drupal CreditAttribution: amit.drupal as a volunteer and at Valuebound for Valuebound commentedUpdate #38
Comment #43
amit.drupal CreditAttribution: amit.drupal as a volunteer and at Valuebound for Valuebound commentedFix issues and update patch.
Comment #44
andypostClean-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
Comment #45
andypostFix comment test - accessing not started session with
has()
orremove()
caused this bugComment #46
andypostProper fix
Comment #48
mglamanNeeds a reroll
Comment #49
andypostcontext changed, reroll
meantime, probably batch needs own session bag, also few checks for session started and exists, still confusing
Comment #51
znerol CreditAttribution: znerol commentedComment #52
znerol CreditAttribution: znerol commentedComment #53
znerol CreditAttribution: znerol commentedI agree, it is confusing. We have the following methods to check different aspects of the session / session cookie:
Tests whether a session object is on the request. This will return
TRUE
even for anonymous requests which do not have a session open unless when run from CLI, or when there is a problem with the container, a subrequest or some broken test. This value is populated in \Drupal\Core\StackMiddleware\Session::handle().In general if you need this method, then most probably there is something wrong with the request (or you are running from CLI).
Checks whether the session cookie is on the request. TBH I do not know a use case for that one.
This is the same as above, but doesn't require the session subsystem. It is used, e.g., in Drupal\Core\PageCache\RequestPolicy\NoSessionOpen
It looks like some session data is remaining after the test. Also it looks like the default PHP session save handler is in place during the test. That simply saves the session as a file in the temporary directory. I haven't figured out yet whether or not this is good or bad (during tests). Let's figure that one out.
Comment #54
znerol CreditAttribution: znerol commentedComment #55
znerol CreditAttribution: znerol commentedRemove call to
hasSession()
frombatch.inc
.Comment #56
znerol CreditAttribution: znerol commentedThe case in
CommentDefaultFormatterCacheTagsTest
is quite funny. That one just fabricates anew Session();
out of thin air. As a consequence that session object is associated with the default Symfony storage (akaNativeSessionStorage
) instead of the Drupal specificSessionManager
one.It might be possible to pass in a
new MockArraySessionStorage()
as the first parameter, but I guess we are better off by just sticking with the Drupal implementation. That means, adding thesystem
module to the test and also installing thesessions
table.Note that it is legitimate that
FormBuilder::buildForm()
unconditionally checks/removes thebatch_form_state
session variable. It needs to retrieve/clear the value from the database in all cases (if there is one). It cannot know whether the value is there before actually checking for it...Comment #58
znerol CreditAttribution: znerol commentedWell, I guessed wrong. If we want to be able to transparently use the session in kernel tests without needing to set up the database etc. in every single test, then it is necessary to actually have a working implementation by default. This patch adds a
SessionManagerMemory
relying onMockArraySessionStorage
. Interdiff is against #55.Comment #59
znerol CreditAttribution: znerol commentedComment #60
andypostI'd better keep this class out of core if it's used only in tests
Comment #61
znerol CreditAttribution: znerol commentedTrue. Depending on whether this gets complex or not, I also consider splitting the addition out of this issue.
Comment #63
znerol CreditAttribution: znerol commentedActually add the session to the request in
FormTestBase
andKernelTestBase
.Comment #64
znerol CreditAttribution: znerol commentedWe already have an issue for that: #2484991: Add the session to the request in KernelTestBase, BrowserTestBase, and drush
Comment #66
znerol CreditAttribution: znerol commentedMake session available in functional tests via
FunctionalTestSetupTrait
as well and also start it (this is in order to mimic the behavior of the session middleware).Comment #68
znerol CreditAttribution: znerol commentedFunctionalTestSetupTrait::rebuildContainer()
callsprepareRequestForGenerator()
. That function used to be important beforerun-tests.sh
was migrated to populate the request stack in #2869573: Remove usages of deprecated DrupalKernel::prepareLegacyRequest().Since
prepareRequestForGenerator()
pushes a new request on the stack without also setting the session, code running after that will end up without a session on the request.Maybe we can get away by just removing the invocation of the problematic method from
rebuildContainer()
.Comment #69
znerol CreditAttribution: znerol commentedComment #71
znerol CreditAttribution: znerol commentedLet's split up this task into the following smaller bits:
has()
,get()
,set()
,remove()
on the session retrieved from the request. No$request->hasSession()
andisStarted()
, etc.Comment #72
znerol CreditAttribution: znerol commentedConvert all occurrences of
$_SESSION
access in non-problematic classes, individual tests and modules.
Comment #73
znerol CreditAttribution: znerol commentedPosted #3109042: Convert uses of $_SESSION in update module and authorize subsystem
Comment #75
znerol CreditAttribution: znerol commentedRemove hunks in the following files:
Will transfer them to the other issue.
Comment #76
andypostIt 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
Comment #77
andypostHere's clean-up for calls and views, file one more follow-up #3109109: AccountForm should read pass-reset-token only from query string
Comment #78
andypostThe only changes with BC affected, but looks they are totally valid
this change is not clear - why URL removed in previous hunk
Comment #79
znerol CreditAttribution: znerol commentedOh, this is a
KernelTests
. So maybe better move those changes over to #2484991: Add the session to the request in KernelTestBase, BrowserTestBase, and drush and fix them over there using the new in memory session.Comment #80
znerol CreditAttribution: znerol commentedMove hunk in
ReplicaKillSwitchTest
to the other issue.Comment #81
znerol CreditAttribution: znerol commentedDo not delete tests verifying backward compatibility with
$_SESSION
superglobal.Comment #82
znerol CreditAttribution: znerol commentedRemoved hunks in
session_test
module andSessionTest.php
browser test.Comment #83
jibranIf the patch is ready then can we please update the issue summary?
Comment #84
znerol CreditAttribution: znerol commentedComment #85
znerol CreditAttribution: znerol commentedFix docs in service parameter definition.
Comment #86
andypostNeeds reroll after #3101108: Dblog event page for non-existing event returns 200, should be 404
Comment #87
andypostreroll
Comment #88
martin107 CreditAttribution: martin107 as a volunteer commented1) 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
Comment #89
znerol CreditAttribution: znerol commentedThanks for the review @martin107.
Note that you need to look for
$_SESSION
(including the underscore). Even after this patch, there will still remain a couple of occurrences. The reasons for the exceptions and follow-ups are noted in the issue summary.Comment #90
JeroenTComment #91
alexpottWe 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.
Comment #92
kim.pepperCreated draft change record: https://www.drupal.org/node/3115716
Comment #93
andypostIt should be commited to 9.1 first
Comment #94
andypostRe-roll for 9.1 and fix pass by reference
Comment #95
martin107 CreditAttribution: martin107 as a volunteer commentedCR looks good to me.
Comment #97
znerol CreditAttribution: znerol commentedComment #98
znerol CreditAttribution: znerol commentedComment #100
znerol CreditAttribution: znerol commentedFix reroll fail in
MigrateController.php
and updateUserAccountFormPasswordResetTest.php
to set the session on the request.Comment #102
raman.b CreditAttribution: raman.b at OpenSense Labs commentedAddressing the remaining test failure
Comment #104
joachim CreditAttribution: joachim as a volunteer commentedI find the use of remove() to get the values a bit weird, but that's a nitpick.
Comment #106
catchThis 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.
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!
Comment #107
andypost@catch CR needs to be published)
Comment #109
cilefen CreditAttribution: cilefen as a volunteer commentedSorry 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