Objectify session management functions + remove session.inc

NativeSessionStorage comes with tons of own special behavior and presents a significant change to our session handling.

As discussed in IRC, I'd really really prefer to limit this issue to a second baby step:

Just simply objectify the remainder of session.inc + get rid of session.inc.

That's a large enough task of its own. Similarly hairy as #2205295: Objectify session handler, because we need to resolve some Simpletest problems. I'd like to avoid any additional cause for potential problems and change in behavior within this step.

i agree. we should bring the symfony code in over at #1858196: [meta] Leverage Symfony Session components. Then we can have a clearer picture on whats going wrong with testbot and less places to debug.

Lets not remove session.inc here though. Just keep the procedural wrappers and just deprecate them?

@ParisLiakos: To my knowledge, there are only a handful of calls to those functions throughout core, so converting them should be a no-brainer. That said, we can happily defer that decision to "later" in this issue + keep the procedural wrappers for now — let's focus on the real meat first :-)

Imo we should:

1. Create a session service
2. Make the Cookie authentication provider instead of requiring session.inc, have the session service injected.
3. Call $this->session->init() instead of drupal_session_initialize().
4. On cleanup call $this->session->save() instead of drupal_session_commit()

And in general
Stop loading session.inc
Move session.inc's public functions to public methods in session service

This way, we move the session handling to DIC -> win!

1. +++ b/core/core.services.yml
   @@ -718,6 +718,12 @@ services:
   +  session.storage:
   +    class: Drupal\Core\Session\Storage
   +    calls:
   +      - [initialize]
   
   +++ b/core/includes/session.inc
   @@ -14,43 +14,7 @@
    function drupal_session_initialize() {
   ...
   +  Drupal::service('session.storage')->initialize();
   
   @@ -59,19 +23,8 @@ function drupal_session_initialize() {
    function drupal_session_start() {
   ...
   +  Drupal::service('session.storage');
   

   I don't get why both drupal_session_initialize() + _start() are calling ::initialize()...?

   The former should not call ::initialize(), 'cos that's part of the DI construction already (very debatable).

   And the latter should call ::start(), because we're *explicitly* asked to start a session, not just initialize it.

2. +++ b/core/includes/session.inc
   @@ -80,47 +33,14 @@ function drupal_session_start() {
   +function drupal_session_started() {
   +  return Drupal::service('session.storage')->isStarted();
   

   Due to the DI service declaration, checking whether a session was started means that a session WILL BE started - no good ;)

Somehow, I can't see why this shouldn't work as-is, without any further adjustments...?

1. Removed call to initialize() from service definition.
2. Inject a proper database connection.
3. Relocate isStarted() for diff consistency with HEAD.
4. Renamed service to 'session'.
5. Include drupal_save_session() + fully sync-up old procedural code with new OO code.

For 100% completeness, I'm additionally attaching a diff between 8.x session.inc + Storage.php, generated via:

git diff -b 8.x:core/includes/session.inc core/lib/Drupal/Core/Session/Storage.php > session.manage.txt

Fixed invalid PHP syntax.

1. +++ b/core/core.services.yml
   @@ -718,6 +718,11 @@ services:
   +    class: Drupal\Core\Session\Storage
   
   +++ b/core/lib/Drupal/Core/Session/Storage.php
   @@ -0,0 +1,282 @@
   +class Storage {
   

   we should name it Session too:)

2. +++ b/core/lib/Drupal/Core/Session/Storage.php
   @@ -0,0 +1,282 @@
   +    $is_https = \Drupal::request()->isSecure();
   +    $cookies = \Drupal::request()->cookies;
   

   we should inject the request

3. +++ b/core/lib/Drupal/Core/Session/Storage.php
   @@ -0,0 +1,282 @@
   +  public function regenerate() {
   

   Can we name this migrate() instead to emulate Symfony\Component\HttpFoundation\Session\SessionInterface::migrate()? Makes transition easier

Fixed invalid method name. Sorry.

Addressing 1+3 of #18.

1. Renamed Storage into Session.
2. Renamed regenerate() to migrate().

re 2) I can only assume that constructor-injecting the request will open a huge can of worms... the usual lack-of-synchronization drama, if you will.

Sorry, I missed to rename the actual class in #21... Attached patch adjusts that patch in-place.

Constructor-inject the request into Session service.

I'm fairly sure this will break, so we probably want to skip that step here...

+++ b/core/includes/session.inc
@@ -80,47 +27,14 @@ function drupal_session_start() {
-function drupal_session_started($set = NULL) {
-  static $session_started = FALSE;
-  if (isset($set)) {
-    $session_started = $set;
-  }
-  return $session_started && session_status() === \PHP_SESSION_ACTIVE;
+function drupal_session_started() {
+  return \Drupal::service('session')->isStarted();

so now the $set is ignored if something passes it :)
we need to have the functionality in the Session class changing the started property

+++ b/core/lib/Drupal/Core/Session/Session.php
@@ -0,0 +1,292 @@
+class Session {

This is a service we need an interface.

fixed the failed tests. now I will do the remaining tasks:
- create an interface for the Session class
- remove session.inc
- class the service from the auth provider and the other files (this will be a little bit harder)
- pushing a new patch in about one hour

removed session.inc
rewrote every file to use the session service

TODO:
- rename the session service
- create the interface

Ok, I agree with this renaming strategy. And I think now, when we'll add the interfaces we should already rename those classes in the proper way.

i am just reuploading the patch in #30 to see if its green.

i am not gonna fight more over naming

On it.

Attached patch is based on @skipyT's #30 + #33.

(btw, it was a bit hard to figure out what exactly happened around ~#30 and which interdiffs to apply to my local branch...)

1. Updated remaining calls to drupal_session_regenerate().
2. Updated all references to "drupal_session_".
3. Relocate endUserSession() and rename to delete() for diff + clarity purposes.
4. Make ::initialize() return $this to allow for chained method calls.
5. Fixed Drupal\system\Tests\Session\SessionTest.
6. Renamed 'session' service to 'session_manager' (and Session class to SessionManager).
7. Renamed local variables from $session to $session_manager for clarity.
8. Properly inject SessionManager dependency into SessionHandler.

For completeness, I'm also attaching a new diff between 8.x session.inc vs. SessionManager again.

@jibran:

This patch will not introduce an interface for SessionManager, because this is just the second baby step, and subsequent follow-up issues will significantly change the session subsystem. Therefore, it does not make sense to introduce an interface (API), because the public API is going to be changed.

1. Inject SessionManager into Cookie auth provider.
2. Inject SessionManager into Cron service.
3. Fixed phpDoc of SessionManager::__construct().

re: #42:

Let's simply stick to the current reality. It doesn't help us to "hide" or ignore dependencies in this conversion.

Much rather the opposite: By injecting them properly, the result is a very clear big picture of how the current classes are inter-operating with each other.

In turn, that allows us to make very simple + clear statements in follow-up issues à la "This class should not get that class injected, because ..."

this look good for me, if we get green on the last patch we can ask for RTBC. I think this will be enough for this step.

i agree with #44
BUT

If we are removing session.inc and we dont add an interface, what do we write in the change notice? (I dont think we will ever get an api change approval this way)
Thats why we should add an interface. The public facing methods will all remain anyway.

So lets have a Drupal\Core\Session\SessionInterface with the following methods:

enable()
disable()
initialize()
isEnabled()
isStarted()
migrate()
save()
start()

In the future this interface will extend from the symfony one, but there will be no api change there, because those methods will remain.

Added SessionManagerInterface.

Not going to express any intentions in code, because there is still way too much discussion and disagreement on those fronts ;-) — The ultimate intentions will be figured out in issues, not code comments.

After lots of back and forth in IRC, this SessionManager class is actually the equivalent of the concept that Symfony calls "Session Storage" (yet another ugly misnomer!):

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Htt...

The migrate() method only exists on the Session class in Symfony, which is a concept that does not exist in Drupal core yet:

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Htt...

Focus is really hard on "yet" — the ultimate goal of the parent issue is to introduce and use that Symfony Session class (as-is) in Drupal core. But this issue here is just a baby step towards that goal - we're not there yet. But still, SessionManager != Session.

Therefore:

Reverted rename of SessionManager::regenerate().

Updated + rewrote issue summary to fully reflect the changes of this baby step.

Will create a draft change notice in a moment.

Draft change notice: https://drupal.org/node/2228871

Everyone involved in here thinks that this is RTBC, but no one feels bold enough to promote it. — Anyone else out there?

FWIW, the last cross-branch-file diff from #40 is still applicable — only the migrate() method name was reverted back to regenerate().

This should help reviewers to confirm that the actual meat of the former procedural session.inc functions are not actually changed here.

looks good to me. I had hoped that we could have removed more usages of global $user in this effort but agree that could happen in a next step.

The focus of this issue largely what we had attempted to do in #1858196: [meta] Leverage Symfony Session components. Great work!

1. +++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php
   @@ -18,6 +19,23 @@
   +   * @var \Drupal\Core\Session\SessionManager
   ...
   +   * @param \Drupal\Core\Session\SessionManager $session_manager
   

   We do have an interface so we can also typehint that.

2. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -0,0 +1,300 @@
   +  /**
   +   * The master request of this process.
   +   *
   +   * @var \Symfony\Component\HttpFoundation\Request
   +   */
   +  protected $request;
   

   If we explicit need the master request shouldn't we then use the request stack and ask for the master request? What ensures that the request here is the master one.

3. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -0,0 +1,300 @@
   +  /**
   +   * Whether session management is enabled or temporarily disabled.
   +   *
   +   * PHP session ID, session, and cookie handling happens in the global scope.
   +   * This value has to persist, since a potentially wrong or disallowed session
   +   * would be written otherwise.
   +   *
   +   * @var bool
   +   */
   +  protected static $enabled = TRUE;
   +
   +  /**
   +   * Whether the session has been started.
   +   *
   +   * @var bool
   +   */
   +  protected static $started = FALSE;
   

   I don't see why this properties should be static ... everything interacts with actual instances of the object anyway.

4. +++ b/core/lib/Drupal/Core/Session/SessionManager.php
   @@ -0,0 +1,300 @@
   +  /**
   +   * Returns whether the current PHP process runs on CLI.
   +   *
   +   * Command line clients do not support cookies nor sessions.
   +   *
   +   * @return bool
   +   */
   +  protected function isCli() {
   +    return PHP_SAPI === 'cli';
   +  }
   

   Will we have a better place for drupal_is_cli() in the future?

We should also look at SessionTestSubscriber (used in SessionTest.php) where we have to call the ugly "\Drupal::currentUser()->getAccount();" just to initialize the session. That should probably be replaced with some start() call to the session manager service.

Thanks for your reviews!

Please note that this issue intentionally is a second baby step only. We are purposively NOT changing any implementations and/or calling code. The existing functionality is converted as-is into a service only.

1. Type-hint SessionManagerInterface instead of SessionManager.
2. Fixed phpDoc for $request properties.

re: #56.3: Both $enabled and $started were static variables previously, so this is taken over literally from HEAD. It is required for tests to pass, in particular because web tests as well as the installer are futzing with these global/static session state properties. The code is moved as-is. Figuring out a more appropriate home for these properties and methods is out of scope of this issue. That will be part of the later steps in the transition to Symfony's session handling.

re: #56.4: drupal_is_cli() is only inlined into the SessionManager class to remove an unnecessary dependency on procedural code. This method is not supposed to be a replacement for drupal_is_cli() (is is protected to begin with).

re: #57: If that is required in HEAD, then it is also required with this patch, because the implementation is not changed at all. This patch converts the remainder of session.inc into a service/class.

As transparently demonstrated with the cross-branch-file diff session.manage.txt in #40, the class contains the exact same code. No functional changes.

Our one and only intention here are the steps outlined in the issue summary. We want to avoid scope-creep at all costs, because that is what caused #1858196: [meta] Leverage Symfony Session components to get stuck forever.

The vast majority of this code will most likely be relocated and significantly changed, once we have the baseline to get back to work on the conversion to Symfony's session handling.

1. Inject RequestStack instead of current Request into SessionManager.
2. Inject RequestStack instead of Request into SessionHandler.

2) is for consistency; I hope that it doesn't break anything.

For now using the current request is exactly what core does. We should think about using the master request explicit in a follow up.

Created #2229223: RequestStack is not persisted across kernel rebuilds

60: session.manage.60.patch queued for re-testing.

Merged 8.x.

Added CLI check to SessionManager::initialize() to account for Drush.

D'oh. It certainly helps to comply with the interface ;-)

The test failures all happen to be related to Locale. Debugging those yields the following backtrace:

Fatal error: Call to a member function isSecure() on a non-object
  in \core\lib\Drupal\Core\Session\SessionManager.php on line 85

drupal_handle_request( )	..\index.php:15
drupal_bootstrap( )	..\bootstrap.inc:1461
_drupal_bootstrap_code( )	..\bootstrap.inc:1408
file_get_stream_wrappers( )	..\common.inc:2926
Drupal\Core\Extension\ModuleHandler->invokeAll( )	..\file.inc:211
call_user_func_array ( )	..\ModuleHandler.php:315
locale_stream_wrappers( )	..\ModuleHandler.php:315
t( )	..\locale.module:210
Drupal\Core\StringTranslation\TranslationManager->translate( )	..\bootstrap.inc:1013
Drupal\Core\StringTranslation\TranslationManager->getStringTranslation( )	..\TranslationManager.php:139
Drupal\locale\LocaleTranslation->getStringTranslation( )	..\TranslationManager.php:119
Drupal\locale\LocaleLookup->__construct( )	..\LocaleTranslation.php:110
Drupal\Core\Session\AccountProxy->getRoles( )	..\LocaleLookup.php:99
Drupal\Core\Session\AccountProxy->getAccount( )	..\AccountProxy.php:93
Drupal\Core\Authentication\AuthenticationManager->authenticate( )	..\AccountProxy.php:77
Drupal\Core\Authentication\Provider\Cookie->authenticate( )	..\AuthenticationManager.php:95
Drupal\Core\Session\SessionManager->initialize( )	..\Cookie.php:51

→ file_get_stream_wrappers() is invoked during bootstrap, before HttpKernel::handle() adds the current request to the request stack.

In #2229223: RequestStack is not persisted across kernel rebuilds, I sorta intentionally skipped drupal_handle_request() + _drupal_boostrap_kernel(), because I (wrongly) assumed that this code path would not be possible...

→ Fixed current request is not available in request stack before HttpKernel::handle().

This effectively duplicates the master request in the request stack, but I do not see a way to work around that without significantly changing how Drupal is bootstrapped.

Fixed mock Request in DUTB is not added to request stack.

Oh, sorry. Last patch was bogus. :-/

Back green again. :-)

For completeness, I'm attaching a final diff between session.inc and SessionManager, generated via:

git diff -b 8.x:core/includes/session.inc core/lib/Drupal/Core/Session/SessionManager.php > session.manage.txt

@znerol shifting to setting injection instead of constructor injection doesn't buy us much.

The patch is green now so Yay! This is a solid step forward

Merged 8.x. Just a merge conflict in DUTB due to a changed comment.

85: session.manage.85.patch queued for re-testing.

@cosmicdreams: RTBC patches are automatically re-tested once per day now.

Moving forward here would allow us to validate some testing framework related changes in the kernelize-bootstrap patch.

@sun Oh good news. No more manual retests.

I'm seeing a lot of issues modify their titles to include the number of issues that are postponed by parent issues. It might make sense to round up all the session related issues that relate to this and see if any need to be postponed until this issue gets in. I'm specifically thinking of how #961508: Dual http/https session cookies interfere with drupal_valid_token() will have a significant reroll because of this issue. There are likely others.

This one's a bit over my head, and catch has a busy week, so tossing over to Alex.

Also this "feels" major-ish, since the related issue is as well, although I could be wrong.

While it seems major, it's not doing anything different than it used to. We're just objectifying here.

+++ b/core/core.services.yml
@@ -718,6 +719,11 @@ services:
+  session_manager:
+    class: Drupal\Core\Session\SessionManager
+    arguments: ['@request_stack', '@database']
+    tags:
+      - { name: persist }

Looking at the code in DrupalKernal I just can not see how a persisted service can ever actually be swapped out once the container is compiled. If a module swaps the session_manager then the persist logic will copy the old one back over the top and then the container will be dumped. The only way I can see would be to manually delete the container once the module that alters the session manager is installed.

Can we add a test to show that I'm wrong?

@alexpott: I'm not sure how that is related to this issue?

The current persist functionality does not persist service definitions, it only persists instances of services.

Instantiated services are persisted until DrupalKernel is shut down (which resets the entire container to null).

Therefore, if a (new) module swaps out a persisted service, then the new service definition will kick in as soon as DrupalKernel is shut down and rebooted from scratch (typically happening on a new request, but also possible through a manual shutdown() → boot() sequence).

Especially because SessionManager registers the SessionHandler to PHP core, that is a required behavior here, since swapping out and registering a new session handler to PHP core mid-request is not something that can reasonably work (without a facility to migrate an existing session from one session handler to a new session handler).

In addition, swapping out session.inc mid-request is and was not possible before in HEAD either, because procedural functions can only be loaded once.

Therefore, this patch only converts the existing functionality and behavior as-is. I know you have reservations regarding persisted services (though possibly also a misunderstanding of how persisted services work?), but that is status quo and the only existing mechanism in HEAD. — We can discuss to replace that mechanism with a better one and/or possible ways to eliminate ModuleHandler::updateModules(), but that's a very complex separate discussion to have and it's not clear why that should hold up this issue...?

So @sun is correct - I wrote a module to swap out the container and the dumped version contains the replaced class...

  /**
   * Gets the 'session_manager' service.
   *
   * This service is shared.
   * This method always returns the same instance of the service.
   *
   * @return Drupal\session\AltSessionManager A Drupal\session\AltSessionManager instance.
   */
  protected function getSessionManagerService()
  {
    return $this->services['session_manager'] = new \Drupal\session\AltSessionManager($this->get('request_stack'), $this->get('database'));
  }

Setting back to rtbc as I was responsible for derailing.

Committed 12abe62 and pushed to 8.x. Thanks!

Thanks! :-)