Objectify session handler

+++ b/core/includes/session.inc
@@ -3,247 +3,30 @@
 use Drupal\Core\Session\UserSession;

Do you need to use UserSession in the new SessionHandler class?

Do you need to use UserSession in the new SessionHandler class?

Same namespace (Drupal\Core\Session\) so not needed

+++ b/core/lib/Drupal/Core/Session/SessionHandler.php
@@ -0,0 +1,307 @@
+    global $user;
...
+      $user = new UserSession();
...
+      $user = new UserSession($values);
...
+      $user = new UserSession(array(
...
+      $user = new UserSession();
...
+      'value' => $user->session,
...
+    return $user->session;
...
+    global $user;
...
+      if ($user) {
...
+      if ($user && ($is_changed || $needs_update)) {

A long time ago, chx made the recommendation of calling this variable "$account" instead of "$user" because that helps us think of the problem the right way.

These are really "people" they are just accounts that people use.

@sun: THANK YOU for this patch. It's so clean. Hopefully in the next few weeks I can have time to help fix test failures.

1. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -0,0 +1,307 @@
   +   * @param $sid
   

   The $sid param should be typed to be string.

2. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -0,0 +1,307 @@
   +    global $user;
   

   This variable of the global user is not really used in this destroy(). It is reset later but never used.

Here are a few documentation fixes. Also, I'm not sure if this was a good idea, but in order to fix documentation issues for destroy() I had it return TRUE if it can manipulate the session and FALSE if it can not.

Did you intentionally exclude the SessionHandlerInterface that your previous patch had?

Thanks for the docs fixes, that's much cleaner.

Small change to documentation. @sun you might find this change unneeded but it makes my IDE happy.

Just a note that PHP 5.4 testbots are now a thing (woohoo!), so we can ignore the 5.3-support now. Hopefully that makes life easier.

20: session.20.patch queued for re-testing.

In my previous adventures in this area it was stressed to me that part of what makes our session implementation uncommon is how we handle masquerading users and transitioning anonymous users to logged in users (for example in ecommerce situations).

As a result I think it may be important to link related session issues together.
#961508: Dual http/https session cookies interfere with drupal_valid_token()
#2180109: Change the current_user service to a proxy

I want to make sure that these issues stay related because I think they're all part of the same puzzle.

In testing the installation of this locally I found something interesting.

On my laptop I'm running Ubuntu 13.10 which has PHP 5.5 / Apache to run Drupal. Everything installs fine, but after the installation is complete I notice the following.

1. I'm logged in automatically (THANKS SUN)
2. I get this debug message:
"Debug: Failed to start the session: already started by PHP. in Drupal\Core\Session\Session->start() (line 104 of core/lib/Drupal/Core/Session/Session.php)."

One problem seems to be related to http://stackoverflow.com/questions/6249707/check-if-php-session-has-alre...

Which basically says that checks on whether session_id() returns an empty string are no longer sufficient for PHP 5.4. The only function in all of Drupal that this really impacts is drupal_session_started() but that clearly plays a roll in the debug message I'm getting.

It's also worth noting that Symfony's NativeSessionStorage handles this case appropriately. See the start() method, line 133.

Yep that fixed the debug message. Also, some documentation fixes. Let's see how this runs.

Reroll. This still won't work, but I'm posting it because there was a conflict in the code that was moved (key became keys). Otherwise the reroll was simple.

Added the recent issue about using the DrupalKernel for the installer as that should help address the installer errors we routinely have in this issue.

33: 2205295_33.patch queued for re-testing.

After seeing that this patch merges with #2213357: Use a proper kernel in early installer properly, I am hopeful that we'll get some proper errror messages again. I've tested installing locally with php 5.4 and this patch and everything works fine for me.

Great News! I can reliably get the installation to fail now.

I'm running PHP 5.4 and during the installation, after selecting the standing installation profile, I get this error:
Call to a member function isAnonymous() on a non-object in C:\Users\karen_000\Sites\d8\core\includes\session.inc on line 97

So we can start debugging again.

+++ b/core/includes/session.inc
@@ -304,14 +301,12 @@ function drupal_session_start() {
-  if ($user->isAnonymous() && empty($_SESSION)) {
+  if (\Drupal::currentUser()->isAnonymous() && empty($_SESSION)) {

Would this help? Since $user the global object is not always an object.

It's worth a shot, however I think the reason why we're using the super global $user is because we need to execute this logic before \Drupal can be used.

Either way I was able to fix it for me by checking for an empty() $user on that line. Perhaps despite clearing my files directory, dropping all my tables, and trying to reinstall I didn't clear all the remnants of a previous installation.

lets try it.

This likely doesn't have to do with the fail but the logic looks funky:

1. +++ b/core/includes/session.inc
   @@ -306,12 +86,12 @@ function drupal_session_start() {
   -  if ($user->isAnonymous() && empty($_SESSION)) {
   +  if ($user && $user->isAnonymous() && empty($_SESSION)) {
   

   Wouldn't a !$user be also considered a an anonymous? shouldn't it be

   if ((!$user || ($user && $user->isAnonymous())) && empty($_SESSION)) { or something with less parens...

I don't understand all of what's going on with #1808220: Remove run-tests.sh dependency on existing/installed parent site. Maybe with that issue being fixed we'll get a different test result.

42: drupal-2205295-42.patch queued for re-testing.

Reroll after #2219009: Improve DX of Settings class. I will try to look into this now.

Let's see what happens when we actually use the request_stack instead of the request service.

Actual Failures!!!!!! Whoohooo!

was #48 a straight reroll?

Yes, #48 was a straight reroll. Needs another one after #2178581: Add an AnonymousUserSession object in favour of using drupal_anonymous_user(). Maybe we better stick with the request service for the moment and do to switch to request_stack afterwards.

The interdiff is against #48 with the meta-hunks removed.

One of the leading causes of failure

+++ b/core/lib/Drupal/Core/Session/SessionHandler.php
@@ -0,0 +1,295 @@
+      $needs_update = !$user->getLastAccessedTime() || REQUEST_TIME - $user->getLastAccessedTime() > Settings::get('session_write_interval', 180);

This seems to be the leading cause for the failures. $user is defined from the use of "global $user" in the first line of this function.

As it was explained to me, the global $user is not always a UserSession and thus we end up in the situation we are now with these failures.

I think we need to either define a constructor where we ensure a UserSession is available or at least check to see if $user is a UserSession.

Yes, most failures are related to update / authorize scripts. Let's try your constructor-idea.

I've been busy debugging the test failures from #57. I found that sometimes it happens that the $user object is set to an anonymous session upon request termination in the simpletest parent site. Recalling that the only change introduced with #57 was the initialization of the $user global, it follows that under some circumstances SessionHandler is constructed more than once per request.

Let's see what happens when we only initialize $user if it has not been set before.

I took out the destructor and now the session write close is happening on the shutdown function, lets see like this.

Wow! cool, now we only need to fix #2108623: Installing via the UI no longer logs in user 1 every time to make the remaining tests pass.

#61 removed the dependency on the DestructableInterface and therefore the explicit destruction introduced into the install_drupal function is not necessary either. Let's see.

Test whether we can remove some other presumably unrelated change.

Also try to remove the hunk from install_bootstrap_full.

It's good to omit that hunk in #65 as I added it only to make some tests pass. However why get rid of drupal_session_start() in install_bootstrap_full().

Very Happy to see this go green.

However why get rid of drupal_session_start() in install_bootstrap_full().

I'm basically trying to remove every bit of this patch which is not directly related to the actual refactoring. The call to drupal_session_start does not exist in HEAD.

i say, someone rtbc this quickly and lets get it in, while bot is doing us favors!

+++ b/core/modules/system/lib/Drupal/system/Tests/Session/SessionTest.php
@@ -20,6 +20,8 @@ class SessionTest extends WebTestBase {
+  protected $loggedInUsers = array();

Newly introduced variables need to have a docblock. Why do we need to have the array of logged in users?

Otherwise looks good, this is a reasonable start!

Let's remove the alterations to the test code and see whether it still turns green.

+++ b/core/includes/session.inc
@@ -3,249 +3,27 @@
+use Drupal\Core\Session\SessionHandler;
 use Drupal\Core\Utility\Error;

We shall remove these lines, cause those classes aren't used in the session.inc file.

Next steps: convert all of the session related procedural functions as public service methods, split the session handling from the session storage.

What do you think?

@skipyT, I think a step before making session into a service is to objectify it. That should be quick and easy as we only have to convert the procedural functions to static methods to the least work possible. If you want to take it step further and make session into a service that would be cool too.

I'm thinking that given the amount of effort we've put into this over the past year, that it might make sense to shoot for small measurable changes to get to our final solution faster.

@cosmicdreams: I agree with you. I just need this task to be finished ASAP, cause I'm porting session to mongodb and it would be easier if we make this changes. I will see tomorrow with znerol how can I help.

There's no reason why we can't achieve both: advancing the code is small measureable steps and getting it all done ASAP. Shooting for major overhauls and trying to do too much is what has kept this task unfinished for more than 1.5 years. I think a lot of the complexities that need to be resolved will expose themselves by attempting to objectify the session.

After that making it a service should be simple.

It feels a bit awkward that the session handler is a service, given that its methods should never be called directly. I think we should aim at converting it into a private service later on (when the remaining stuff from session.inc has been converted into services.

In order to make this better accessible for reviewers I extracted all the session handler functions from session.inc (HEAD) and SessionHandler.php then diffed each pair.

Yay!!! patch is green. Great work everyone. Minor feedback.

1. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -0,0 +1,288 @@
   +   * {@inheritdoc}
   +   *
   +   * Initializes the global $user object for the user associated with the
   +   * session.
   

   We still can't mix these.

2. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -0,0 +1,288 @@
   +    // that is in the user's cookie is hashed before being stored in the database
   

   More then 80 chars.

3. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -0,0 +1,288 @@
   +      $values = $this->connection->query("SELECT u.*, s.* FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.ssid = :ssid", array(
   ...
   +          $values = $this->connection->query("SELECT u.*, s.* FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.sid = :sid AND s.uid = 0", array(
   ...
   +      $values = $this->connection->query("SELECT u.*, s.* FROM {users} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE s.sid = :sid", array(
   

   Can we add comment to explain the query?

4. +++ b/core/lib/Drupal/Core/Session/SessionHandler.php
   @@ -0,0 +1,288 @@
   +        // We don't have anything to do if we are not allowed to save the session.
   ...
   +      // $_SESSION has changed or more than 180 has passed since the last update.
   ...
   +          // secure and insecure session cookies. If enabled and both cookies are
   

   more then 80 chars.

Thanks for the review, incorporated the suggested changes and also the following two things:

• Discussed with @skipyT: We do not like to expose the session handler as a service but rather extract the session storage in a follow-up. Therefore removed the session_handler service. It was introduced anyway as a workaround in #14.
• Removed two spurious use statements (addressed #74)

I also looked up what PHP actually does with the return value of the session-handler callbacks. They are mostly ignored (or simply used for logging purposes), except for open and read (obviously). I interpret the changes to the return value of session handler callbacks to be just for consistency reasons (@sun perhaps could comment on that).

This is the baby step start that we want. I checked the patch and all we are doing now is moving the session functions to a class, which is fine.

+++ b/core/includes/session.inc
@@ -245,7 +16,14 @@ function _drupal_session_write($sid, $value) {
+  if (drupal_is_cli()) {

@@ -308,7 +86,7 @@ function drupal_session_start() {
+  if (!drupal_save_session() || drupal_is_cli()) {

@@ -358,7 +136,7 @@ function drupal_session_regenerate() {
+  if (!drupal_save_session() || drupal_is_cli()) {

Both znerol and I think this change is unnesarry and assume maybe things about our enviroment which is not right. We should change as little as possible

In fact there is already a call to drupal_is_cli in order to prevent initiating a session when executed from the command line in drupal_session_start. I guess that the additional checks were introduced for consistency reasons.

One of the next steps will be to move the rest of session.inc into a container based service, and therefore we need to touch that code again anyway.

Fine with me, looking still good otherwise.

Followup #2228341: Objectify session management functions + remove session.inc.

Committed 4ac79a1 and pushed to 8.x. Thanks!

Yay!!! Thanks @sun, @cosmicdreams, @znerol and all for making this happen! It is so much more organized, cleaned up and generally debatably nicer looking as OOP!

Not yet pushed

Another followup #2228393: Decouple session from cookie based user authentication

Ok, pushed this time. :) Thanks!