Problem
-
In order to move forward in review-able baby steps, #2228341: Objectify session management functions + remove session.inc converted the former static
$started
variable from session.inc as-is into aSessionManager
class property. -
This property is not static in Symfony, and it should not have to be.
-
Tests + various
DrupalKernel
rebuilds will likely disagree though. :P
Proposed solution
-
Remove the static variable entirely.
#201122: Drupal should support disabling anonymous sessions originally introduced it in a custom wrapper around
session_start()
.#477944: Fix and streamline page cache and session handling introduced lazy-sessions and changed the condition to additionally check for
session_id() !== ''
, but forgot to remove the static variable.
Comment | File | Size | Author |
---|---|---|---|
#5 | 2239181-make-started-non-static-5.diff | 1.06 KB | znerol |
#3 | interdiff.txt | 6.35 KB | sun |
#3 | session.started.3.patch | 7.04 KB | sun |
#1 | session.started.1.patch | 998 bytes | sun |
Comments
Comment #1
sunLet's see what happens.
Comment #3
sunWhile testbot only shows 1 failing test case, that failure/error effectively means all web tests - but only when executed through the Simpletest UI.
session_start()
, session is started already."TestBase::prepareEnvironment()
.DrupalKernel::updateModules()
— it does not apply to complete replacements ofDrupalKernel
.SessionManager
has been re-instantiated from scratch for the installer kernel,install_bootstrap_full()
tries toinitialize()
the session — but it is initialized already.Attached patch attempts to
TestBase::prepareEnvironment()
.TestBase::restoreEnvironment()
.That apparently works... until the restore, which blows up with this exception:
As you can see in this patch, I already tried various things to figure out the cause for that exception, but to no avail.
Comment #5
znerol CreditAttribution: znerol commentedDo we need the static property at all? Let's try to remove it completely.
Comment #6
sunAwesome idea! :-)
Let's do this. We can look into the problem discovered in #3 in a separate issue (session of UI test runner leaking into all web tests).
Comment #7
sun@znerol additionally dug out the original issue that introduced this static variable — however, it does not contain any discussion nor an explanation for why the static variable was added.
Even before PHP 5.4,
session_status() === PHP_SESSION_ACTIVE
is identical to this:See also http://php.net/manual/en/function.session-status.php#113468
Therefore, it is not clear why that static variable ever existed to begin with. It looks like it was unnecessary all of the time.
Comment #8
znerol CreditAttribution: znerol commentedI think it the history is like follows:
This might indicate that the authors were not aware that it was possible to determine the session status using
session_id() === '';
(see session_status).This patch introduced the
session_id()
check but failed to remove the static variable.Comment #9
sunGreat! — That makes some more sense now.
Definitely fine to simply remove it then. :-)
Comment #10
sunAdjusting issue title + summary accordingly.
After my debugging investigations in #3, I actually consider this a bug now, since
session_write_close()
ends a session, but due to the static variable, and because that variable is never reset to FALSE, Drupal still believes that there is an active session.There is no code in core/HEAD that checks whether a session is active after writing+closing it (typically at the end of a request), but
SessionManager::isStarted()
definitely returns a wrong status in that case right now.Comment #11
sunSorry, one more issue title adjustment — I think this new one properly describes the hidden bug.
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commented+1000 to commit this
This static variable is the reason i wasted many hours of my life in original symfony/session patch, and #10 nicely explains it, thanks!
arg
Comment #13
sunMoved #3 into new #2239969: Session of (UI) test runner leaks into web tests
Comment #14
alexpottCommitted 3a7d54d and pushed to 8.x. Thanks!