Important! Discussion has moved to #1858196: [meta] Leverage Symfony Session components where work has been divided up into a smaller patch and sub-issues.

Important note to committers: make sure before commit this is cleared by the security team (see process concerns in #185).

I recommend jumping straight to comment #153. A lot of earlier comment relates to code that predates latest Symfony 2 improvements.

Issue status

We want to get rid of our custom bastard Session handling, and we need to make it pluggable in an easier way than rewritting the full session.inc file. For that, we want to try to use the Symfony 2 HttpFoundation component's session handling.

What will it bring to Drupal

  • Code consistency, and unification (using Symfony 2) and a lot less of legacy code to maintain
  • Session storage pluggability (session storage in Symfony vocabulary is the in-memory $_SESSION array)
  • Session handler pluggability (session handler in both PHP and Symfony vocabulary is the physical session data storage mecanism, for us, it's the database, but later, it will be anything compatible with Symfony)
  • A full OOP API to handle the session, session component interface and injection throught the DIC, thus removing yet again an global state out there (not totally true because PHP will allow only one session handler to be set, but conceptually and in design)

Where do we need to be extra careful

We have two major features that Symfony today does not handle natively we absolutely want to keep:

  • Lazy cookie sending (not lazy session initialization): this allows anonymous user to avoid having useless session, and allow core page cache mecanism to work seamlessly
  • HTTP/HTTPS dual cookie handling: this is a real security measure that disallow HTTP session hijack and re-use the HTTP session identifier with HTTPS session.

Proposed plan (where I need review, please!)

Here is the proposed plan (taken, simplified and updated from #153:

  1. Replace the full session.inc using custom Symfony component's code, at this stade we actually loose the features described upper [DONE in patch #166]
  2. Restore the custom cookie handling, needed it for the next point, by implement a proxy pattern arround Symfony's SessionProxy. At this stade, we still don't handle dual HTTP/HTTPS session. [DONE in patch #166]
  3. Put back lazy cookie sending in action, implement a proxy pattern arround Symfony's Session object (for exposing the public API) and arround NativeSessionStorage (because a method is missing for fully relying on the interface). I hope I will have the time and courage to post a Symfony PR for that issue (I will explain later). [DONE in patch #166]
  4. Put back the dual session cookie handling in action, using our previous implementation of SessionProxy. This until Symfony implements iself this mecanism or any alternative that brings the same security level.
  5. Find an upgrade path! And this one is not easy.

Once we have a clean solution for the problem enounced below, and working code for dual session cookie handling, we are good to go for final review process and core commit if the code pleases all eyes.

The upgrade problem

Symfony's session handling does not care about UID, but Drupal does: the actual core code uses a sessions database table composed of multiple mandatory fields, including the UID. Because per design Symfony's does not care, we cannot use the actual core table for storing sessions. This has a major impact for us: upgrade path is not possible, because in the DatabaseSessionHandler implementation, we have two possibilities:

  • Either we set a false UID always, but since the SQL query will be hardcoded, we cannot fix the database table because depend on the old schema.
  • Either we do not set a UID at all in the SQL insert/merge queries, but we will raise PDOExceptions because the field is mandatory, this until we fixed the table.

In both case, we are blocked, because in the first case, we cannot upgrade properly, in the second, we cannot even access the update once the code has been updated.

There is some solutions thought:

  • If we ask Drupal 7 maintainers to set a default value to the uid field to 0, our Drupal 8 handler will work flawlessly on the old schema, we can then update. But this is invasive in Drupal 7 (and the most easy solution thought)
  • Create a new table with a new name (dropping the trailing 's' in sessions) and use it directly: problem almost solved because we can drop the old one and just not use it, but we have to consider that doing that, we will have PDOExceptions trying to use an non existing database table.
  • Using another session handler for update.php (using PHP native one) was another solution, but not exploitable because we can't login the user in update.php.

Note that doing runtime check on the table schema is out of question, it's really a bad idea that will have a negative impact on performances on such early and ominpresent core system.

And now...

This issue needs brains for telling if that plan is bad, and if so why.
It also needs some more brains mostly for the update problem.
And finally it needs reviewers to see if the the code doesn't sound bad (please no whitespace reviews in such early development phase).

Now, for more reading, start there: #153
And for the first almost acceptable patch: #166

Related issues

#355513: Create an anonymous user API

Original issue

  • As part of the WSCCI initiative, it has been decided that we should use the Symphony2 HTTP Foundation Library (#40). That library comes with a session handler that we could build upon instead of rolling our own.
  • The session library comes with an equivalent function to drupal_set_message() ("flash"). We need to decide if we can use that, get it refactored upstream, or not use the session handler at all.
  • We could also just use their SessionStorage interface, and write our own Session class. This looks to be the way to go as of #50.
  • Since using the Session classes requires HttpFoundation, this issue has been marked as postponed pending the resolution of #1178246: Add Symfony2 HttpFoundation library to core
  • Do not repurpose the $_SESSION global unless it would work *perfectly* to expectations and behavior of general language usage and not impact performance. Otherwise layer a session object over it for use by Drupal. (#31, #32, #47)
Files: 
CommentFileSizeAuthor
#340 drupal-session-335411-340.patch69.06 KBmbrett5062
FAILED: [[SimpleTest]]: [MySQL] 0 pass(es), 705 fail(s), and 704 exception(s).
[ View ]
#338 drupal-session-335411-338.patch52.8 KBmbrett5062
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#336 session-335411-306-2.patch68.39 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] 0 pass(es), 700 fail(s), and 699 exception(s).
[ View ]
#333 session-335411-306_1.patch69.07 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch session-335411-306_1_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#332 335411_332_notest.patch0 bytescosmicdreams
PASSED: [[SimpleTest]]: [MySQL] 48,990 pass(es).
[ View ]
#326 335411_326.patch68.9 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/lib/Drupal/user/UserStorageController.php.
[ View ]
#325 335411_325.patch68.9 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/lib/Drupal/user/UserStorageController.php.
[ View ]
#321 disable-invalidate.patch1.27 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch disable-invalidate.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#306 session-335411-306.patch69.07 KBpounard
FAILED: [[SimpleTest]]: [MySQL] 47,955 pass(es), 48 fail(s), and 1 exception(s).
[ View ]
#304 session-335411-304.patch69.07 KBpounard
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#302 session-335411-302.patch69.06 KBpounard
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch session-335411-302.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#296 session-335411-296.patch68.01 KBsteinmb
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#290 session-335411-290.patch68.05 KBpounard
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#283 session-335411-283-interdiff.txt2.07 KBBerdir
#277 session-335411-277.patch67.68 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 47,724 pass(es), 48 fail(s), and 2 exception(s).
[ View ]
#277 session-335411-277-interdiff.txt600 bytesBerdir
#269 session-335411-269.patch67.7 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 47,658 pass(es), 75 fail(s), and 2 exception(s).
[ View ]
#269 session-335411-269-interdiff.txt5.57 KBBerdir
#263 session-335411-263.patch64.92 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#263 session-335411-263-interdiff.txt634 bytesBerdir
#257 335411-257.patch65.43 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 47,046 pass(es), 48 fail(s), and 18 exception(s).
[ View ]
#255 335411-255.patch65.43 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#253 335411_252.patch65.43 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 335411_252.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#251 335411_251.patch66.16 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 335411_251.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#247 335411_246.patch64.46 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#246 335411_246-do-not-test.patch64.46 KBcosmicdreams
#243 full_patch_335411_239-do-not-test.patch90.74 KBxjm
#243 335411_239-intediff.txt1.73 KBxjm
#228 335411-228.patch62.83 KBpounard
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#223 335411-WIP.patch47.17 KBpounard
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#218 335411-218.patch50.71 KBpounard
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#202 335411.patch47.19 KBRobLoach
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#195 sf_sesion-335411-193.patch32.84 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sf_sesion-335411-193.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#173 sf_sesion-335411-173.patch35.94 KBGaelan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sf_sesion-335411-173.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#166 335411-166-sf_session.patch49.2 KBpounard
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#163 335411-156-sf_session.patch50.74 KBpounard
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#143 335411-modules-node-user-do-not-patch.patch17.25 KBPaul Simard
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 335411-modules-node-user-do-not-patch.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#127 335411_127_session.patch82.39 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 335411_127_session.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#124 335411_124_session.patch62.19 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 335411_124_session.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#120 355411_120_session.patch48.26 KBtnightingale
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 355411_120_session.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#114 335411_114_session.patch58.9 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#110 335411-symfony-session-sf2.1-1.patch58.89 KBpounard
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#102 335411-symfony-session-1.patch53.31 KBpounard
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 335411-symfony-session-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#94 335411-symfony-session-0.patch35.55 KBpounard
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 335411-symfony-session-0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#25 335411-refactor-session-handling.patch72.89 KBDamien Tournoud
Failed: Failed to install HEAD.
[ View ]
#22 335411-refactor-session-handling.patch71.9 KBDamien Tournoud
Failed: Failed to install HEAD.
[ View ]
#16 session_singleton_335411-16.patch69.96 KBsdboyer
Failed: Failed to apply patch.
[ View ]
#15 session_15.patch61.17 KBsdboyer
Failed: Failed to apply patch.
[ View ]
#10 session_singleton.patch19.55 KBsdboyer
Failed: Failed to run tests.
[ View ]
session_uid.patch4.58 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch session_uid_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
session_uid.patch3.46 KBchx
Unable to apply patch session_uid.patch
[ View ]

Comments

I agree that it would be a good idea to refactor session management. Two remarks I made on the IRC:

- why not refusing to save the session altogether if session_uid != user->uid ?
- -1 for yet another global variable

And now that I think about it, I guess it would be nice to make that session management self-contained.

well then I either make a static get/set, which i would not like or i simply make sessions into a singleton object and move user_authenticate inside so that you have absolutely no ways to change the session uid aside from providing the proper name and password together. That would be bliss. Coming down the line, we could also provide a drupal_session()->user() instead of global $user.

It is sometimes necessary to log a user in programmatically. Login tickets or URLs such as our current password reminder system come to mind. A dedicated setter would allow this, while making it much harder to accidentally login a user.

Further decision with Heine (and summing up the issue): we will

  1. Make it easy to change the user. Just make drupal_session()->user public. Or we really want, we can make a get-set but hardly necessary.
  2. We wil keep a drupal_session()->uid, initialized in sess_read.
  3. Refuse to write the Edit:usersession if self::$uid != self::$user->uid so that if you want to save the user, then you need to explicitly set the uid as well. If you accidentally change the user object, it wont be saved.

The next step would be to create a "Drupal" object with static methods (ie. the equivalent of a singleton) that will:

- have a Drupal::user member variable
- handle sessions
- handle bootstrap and configuration
- store pointers to other subsystems (Theme, Menu, etc.) or, as a transition, store variables for other subsystems (Drupal::theme, Drupal::theme_key, etc.)

yes for Drupal:: static class instead of a singleton! and we can start with just the user/session stuff. Edit: Maybe Session:: instead? or Drupal_Session:: ? Dont forget, you cant write a class in two includes (unless you include one from the other, meh).

I'd been working at this thus far as a singleton, but I can see an argument for that being overkill. I'll shift my efforts towards just making it a static class, and only use a singleton there's some real reason to do so.

Errr...scratch that. I misread, and I'm tired. I can't see how a purely static class helps us here, so I'm going singleton.

Sorry. Need sleep.

StatusFileSize
new19.55 KB
Failed: Failed to run tests.
[ View ]

In working with this, I made a nice little discovery - not only can you point session_set_save_handler() to methods, but you can point it to PRIVATE methods and it (can) still work just fine. Which means our session handling can enter a whole new level of locked down. Calling something like session_destroy() from userland still gets hit with method visibility, as it should, but it means for some of the session handling functions that were previously marked in their docblocks as "Don't call these directly," it's now impossible to call them directly.

So I've taken the step of moving a fair bit into that session singleton, and have done my best to adhere to the general principles laid out already in this thread. There are obviously some hacky bits in there - we still need a global $user, and I've commented out the direct call to session_write_close in drupal_goto for the userland reason above. And those hacky bits, among others, are probably responsible for the few hundred failed tests I saw on a full test run I ran locally. And it errored out fatally since the file test called drupal_save_session() which I left out as chx did in his original patch. And we need some kind of other approach to doing that, if we use the singleton.

But I didn't want to do any more until I posted here and got some feedback on the general direction.

Status:Needs review» Needs work

The last submitted patch failed testing.

Honestly I'd rather go with an instantiated singleton than a static class. Static classes are touchy in PHP 5.2, because of the lack of late static binding. An all-static class is also in practice little more than a clumsy namespace. I know we are using one in DBTNG, but I'm actually not too happy with that either and there are legit reasons for it. (Mostly relating to it removing about 300 function calls that way, and making code simpler.)

The best of both worlds would require lazy-instantiation of static class properties, but sadly that didn't make it into PHP 5.3. :-(

Oh yeah, and subscribe. :-) I'll try to review this later.

and we hardly need more than a clumsy namespace :) but I am fine with the singleton just the static class looked like fast, saving a function call here and there... Session::$user has no function calls, drupal_session()->$user has one, and the worst is drupal_session()->get_user() which is two so let's avoid the OOP zealot get/set :P

Cool, I'll reroll later tonight, and start to put an eye to some of those test failures. I don't think we're in for any big surprises, but there is the matter of a few hundred failing tests that needs to be addressed...

StatusFileSize
new61.17 KB
Failed: Failed to apply patch.
[ View ]

OK, here's the next crack at this. Some quick notes:

  1. This patch is actually pretty close to working, now, whereas the last one was more like a skeleton/suggestion. The only outstanding issue I'm aware of right now is that the session persistence test fails. I do want to get the testbot's feedback though, b/c I have other things to do with my system :)
  2. Although there's clearly a lot of different logic, this patch mostly abides by the same flow that the old procedural approach did. We may want to refactor that, but I figure, one step at a time.
  3. There are some private properties in there that indicate the state of the session managing singleton. They're not really serving a purpose right now, but could, if some refactoring is done.
  4. I did a quick grep+sed to replace all the instances of global $user in core. I'd have left it until later, but it turns out a problem I banged my head against for four hours was because I was trying to leave 'legacy' support in there for global $user. So I stopped trying and it went away :)
  5. There are interesting possibilities with making the Session class abstracted, pluggable, etc...but as I discussed with chx, that's down the road and we should get this bit licked first.

Yar!

StatusFileSize
new69.96 KB
Failed: Failed to apply patch.
[ View ]

Another iteration, here. Added a bitmask to indicate session state, cleaned up some variables, stripped out some global $user and $GLOBALS['user'] calls that I missed, etc. Paved the way a bit more for making this pluggable...or at least, it feels like I did in my head :) Can't really tell if it's reflected in the code. Incorporated the different function ordering introduced by the commit of #280934: Use httponly cookie support when available. Still getting the session propagation error on running the session tests, and though I haven't really tried to understand it, it's leaving me befuddled on a couple levels.

We're gettin closer!

chx, sorry, but I don't see a compelling argument for making this a static class. drupal_session()->get_user() is gone (rightfully so), but I have difficulty believing that the speed difference would be non-negligible. I'm certainly open to some proof to the contrary. But, while I do agree with Crell - it's a class, not a namespace, and I don't see what we're gaining by pretending otherwise - I also just don't care that much. So basically, I'm going the path of least resistance, and will continue on with other aspects of this patch while more enthusiastic folks fight that one out.

Do not use Private. Use Protected. That allows us to still have an "out" by extending the Session class and mucking with stuff. "Private" access modifiers should never be used.

Docblocks are a bit off in places. Even for methods and class properties, the first line is limited to a single line sentence, and should be separated from any further comments by a blank line.

If we have a singleton object, I don't quite get why we need to prime it in BOOTSTRAP_SESSION, and mark it as such. Part of the benefit of a singleton is that it can be lazy-instantiated when it's first used, provided all access to it goes through the access method.

I think I'd rather use instance() or getInstance() for the singleton access method. get() just feels too generic to me, and may cause confusion latet with HTTP GET or __get() or any number of other things. In my own code I tend to use instance() I think.

Good stuff using @see. However, I'm pretty sure it has to be on its own line as a new docblock item, not inline with the rest of the text. (I may be wrong on that.)

The docblock for sessionClose() is incomplete and ends in mid sentence. :-)

I'm not sure why you're bothering with the bitflags and direct bitwise comparisons. The more common convention is to expose not a property (which someone else can then modify at any time) but a method, either a isOpen() that returns boolean or a checkState($flag) to which you pass the bitflag to check for. That lets you make $session->state protected, and safe from tampering from other parts of the code.

Great, thanks for the review. Item by item:

Do not use Private. Use Protected. That allows us to still have an "out" by extending the Session class and mucking with stuff. "Private" access modifiers should never be used.

Yup, I stripped 'final' from the class declaration right before I generated the patch (the original parameters presented to me had it being final), but clearly neglected to switch over to protected. Particularly important if we're to make this subsystem pluggable; it's becoming increasingly clear as I work on this more that inheritance will be useful if we go that direction.

Docblocks are a bit off in places. Even for methods and class properties, the first line is limited to a single line sentence, and should be separated from any further comments by a blank line.

I've been playing a little loosey-goosey with the docblocks since I'm not 100% on some of what's in there; for those things I'm not sure of, I'm keeping the docblocks loosely informational. I'll improve em as I get more feedback.

If we have a singleton object, I don't quite get why we need to prime it in BOOTSTRAP_SESSION, and mark it as such. Part of the benefit of a singleton is that it can be lazy-instantiated when it's first used, provided all access to it goes through the access method.

Y'know, I started thinking of this while I was browsing around the other session-related patches, especially #201122: Drupal should support disabling anonymous sessions, and it's true, there's really no reason to initiate session operations unless something explicitly calls drupal_session(). However, as it's written right now, initiating sessions is a necessary precondition for having a current user right now. I'll refactor accordingly; I think there are some interesting possibilities, here...

I think I'd rather use instance() or getInstance() for the singleton access method. get() just feels too generic to me, and may cause confusion latet with HTTP GET or __get() or any number of other things. In my own code I tend to use instance() I think.

Works for me - I had getInstance() initially, I'll add it back.

Good stuff using @see. However, I'm pretty sure it has to be on its own line as a new docblock item, not inline with the rest of the text. (I may be wrong on that.)

Thanks! Yeah, I'm a fan of, y'know, using links. I hear it's what this interweb thing is good at. :)

The usage of @see in this patch is (very nearly) the one-and-only way that I personally think it's acceptable to be used quasi in-line. Doxygen defines @see as taking a 'paragraph' parameter - that is, it takes everything from the termination of the tag up to the next carriage return, and will parse ONLY the first argument provided to it as a function. For example, search for @see tags in the Panels css filter; from them, doxygen produces the fugly output you see in the docs.

Problem is, I forgot that you can't do this properly when using them in a doxygen list, because both look for a blank line to terminate the current item, so either you get too much in the @see section, or you screw up your list. Since you want to provide context in this case - that is, have the link be close to the list item it refers to, it's best to just do it without the @see tag there since doxygen and api.module both automagically transform function names into links anyway.

The docblock for sessionClose() is incomplete and ends in mid sentence. :-)

Yeah. See earlier comment :)

I'm not sure why you're bothering with the bitflags and direct bitwise comparisons. The more common convention is to expose not a property (which someone else can then modify at any time) but a method, either a isOpen() that returns boolean or a checkState($flag) to which you pass the bitflag to check for. That lets you make $session->state protected, and safe from tampering from other parts of the code.

These are two separate issues, one being whether or not bitwise operations are appropriate for internal use in the class, and another for how the state bitmask is presented to the user. For the former, it's clearly a yes: this is a classic case where there are multiple separate components to the overall state, where the state of each flag has varied relationships with the state of the others. Now, I can't think of a use case where anybody's going to really, really need to use these - but I'd rather have them there and be crystal-clear accurate for when someone does come up with something.

I agree, in part, about how to access it. We should take all reasonable steps possible to avoid requiring devs to learn bitwise operations in order to interact with any core subsystem. So some methods to that effect would be a good thing. But I don't know enough about how visibility works (and don't have time to check right now) to verify that what you're saying is accurate about protecting the var. I think the relevant question is: sure, the var is returned directly. Does that mean that if the caller asks for it by reference, they can change it and it will change the value in the class? Or is protection implemented in such a way where that won't happen - pass by ref on internal class properties just doesn't work?

Variables are only returned by reference from a function/method if both the function is defined to do so AND the caller assigns by reference. However, there were places in the patch where you were doing:

<?php
if ($session->state & SOME_CONSTANT) { ... }
?>

I'm saying that's bad because you're directly accessing state, which means state must be public, which means anyone can mess with it. Instead it should be protected and you should have the following method:

<?php
class Session {
  public function
checkState($state) {
    return
$this->state & $state;
  }
 
// ...
}
?>

Make sense?

Sure, it makes sense, but at no place in this patch do I actually DO what you're describing. There are several bitwise operations which are written in there, but they're all done within the class context. If there's an exception to that, it would be of the form:

<?php
$state
= drupal_session()->sessionState();
if (
SOME_BIT & $state) { ... }
?>

or maybe

<?php
if (SOME_BIT & drupal_session()->sessionState()) { ... }
?>

I don't think the latter form would be there because I've only just thought of doing it that way, but if you look at the method definition of Session::sessionState(), the reason for my concern cited in #18 should be clear. In any case, though, there should never be a case where the member is being directly accessed in global context. If there is, it's a typo - c'mon now, give me some credit! ;)

Regardless, the checkState() method you've suggested there is clearly the way to go, and the way I'll do it in the next iteration.

Title:Security: do not allow uid changes accidentallyRefactor session handling (and Security: do not allow uid changes accidentally)
Assigned:chx» Unassigned
Status:Needs work» Needs review
StatusFileSize
new71.9 KB
Failed: Failed to install HEAD.
[ View ]

It's far better to simply wrap $_SESSION into our own object so that we can do:

<?php
$_SESSION
->user; // was $GLOBALS['user']
$_SESSION['myvar'] = 'myvalue'; // stays the same
$_SESSION->close(); // was session_write_close();
$_SESSION->regenerate(); // was drupal_session_regenerate();
// etc.
?>

After all, it makes little sense to have a drupal_session() singleton *and* a $_SESSION singleton at the same time.

As an added benefit, because $_SESSION is an ArrayIterator, we could manage to only save the session if it really was modified, thus completely avoiding a costly MERGE query during most page request.

Status:Needs review» Needs work

The last submitted patch failed testing.

Now THAT'S a great approach. Assuming it actually lets us wrap a superglobal in that way - I dunno if the test failure has anything to do with that. From a brief glance, the only drawbacks to the way you've written it now are that a) allowing different session handling classes via some factory approach isn't possible, aside from a pluggable session.inc (which I believe fails the definition of 'pluggable'). That ought to be an easy fix, though, if we want it. b) we do lose the capacity for lazy instanciation of the session object.

Although...hmm. I haven't played with ArrayIterator and ArrayObject enough yet, just the generalized iterators. But. Oughtn't it extend ArrayObject, then allow iteration via wrapping its contents in an ArrayIterator via IteratorAggregate::getIterator()? In any case, though, I really like the direction here, and am glad you posted it Damien, because I have NO time to do anything for the next two weeks, minimum :)

Status:Needs work» Needs review
StatusFileSize
new72.89 KB
Failed: Failed to install HEAD.
[ View ]

That should solve the installation issue (during install, drupal_init_language() is apparently called before session initialization).

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

I would like to see another round with the testingbot to make sure the patch broke the install.

Status:Needs review» Needs work

The last submitted patch failed testing.

I am shocked that replacing $_SESSION with an object would actually work. To be honest I'm not even convinced yet. :-) What exactly is wrong with session()->whatev? Overriding $_SESSION breaks a PHP dev's expectation of what the superglobals do, which is bad DX unless there's a very good reason.

As sdboyer pointed out, a factory function makes it much easier to swap out implementations. Swapping out session.inc is a bad mechanism that breaks the registry.

Which leads me to my next comment: This should be implemented as DrupalSessionInterface and core provides a DrupalSessionDatabase that implements that interface. Other code can then implement alternate implementations of DrupalSessionInterface, possibly by subclassing DrupalSessionDatabase or possibly just implementing the same interface.

That, in turn, necessitates using protected rather than public for things like $session->uid, so that subclasses can tweak them if necessary. That is a requirement. Also, if $session->uid is the uid that originally logged in, not a manipulatable object like $user (which I presume is the reason for not just using $session->user->uid), then it should be named accordingly as $loginUid or something like that.

Similarly, $preventWrite should not be a bare property. Instead, we should have a property protected $writeOnClose = TRUE that we check for in the save routine, and then either expose a method to manipulate it or, even better, have a testing class that extends DruaplSessionDatabase and does nothing but change the value of that property. Then you can just swap into a testing session implementation in a config when you need to. Or, there's no reason we can't do both. :-)

Arguably we could also have a $session->currentUser() method rather than accessing the bare property, too. We could then pass in a param to say whether to clone the object first, allowing us to get a copy of the global $user object that we can mess with without breaking the current login status.

If we do allow ArrayInterface access to the session object, do we want to consider making it "unset safe", so that you can't generate notices by accessing an unset property? Instead, return NULL. That would probably make things break harder, which is a good thing. :-) If you want a default, then we have a $session->value($key, $default) method a la variable_get().

I am shocked that replacing $_SESSION with an object would actually work. To be honest I'm not even convinced yet. :-) What exactly is wrong with session()->whatev? Overriding $_SESSION breaks a PHP dev's expectation of what the superglobals do, which is bad DX unless there's a very good reason.

I don't see why. Accessing $_SESSION as an array works just like before. And because that

As sdboyer pointed out, a factory function makes it much easier to swap out implementations. Swapping out session.inc is a bad mechanism that breaks the registry.

We still have a explicit factory, that is called DrupalSession::init(). Easy to swap out implementations from there.

Which leads me to my next comment: This should be implemented as DrupalSessionInterface and core provides a DrupalSessionDatabase that implements that interface. Other code can then implement alternate implementations of DrupalSessionInterface, possibly by subclassing DrupalSessionDatabase or possibly just implementing the same interface.

Our session implementation is currently non standard. The way to go (if we want to implement that properly) would be to respect PHP settings for sessions. And to allow Drupal and other frameworks (CakePHP, etc.) to share the same session handling. This would require to unbundle our current session and user management... looking at the current implementation of _sess_read() is sufficient to be convinced that we are doing things wrong now.

Similarly, $preventWrite should not be a bare property. Instead, we should have a property protected $writeOnClose = TRUE that we check for in the save routine, and then either expose a method to manipulate it or, even better, have a testing class that extends DruaplSessionDatabase and does nothing but change the value of that property. Then you can just swap into a testing session implementation in a config when you need to. Or, there's no reason we can't do both. :-)

There is no reason to make $preventWrite anything else than a bare property. It is FALSE by default, you can set it to anything non FALSE to prevent session writing. It's as simple as that. There is no validation, no access checking to be done. No need for any cruft here.

Arguably we could also have a $session->currentUser() method rather than accessing the bare property, too. We could then pass in a param to say whether to clone the object first, allowing us to get a copy of the global $user object that we can mess with without breaking the current login status.

That's actually a great idea.

If we do allow ArrayInterface access to the session object, do we want to consider making it "unset safe", so that you can't generate notices by accessing an unset property? Instead, return NULL. That would probably make things break harder, which is a good thing. :-) If you want a default, then we have a $session->value($key, $default) method a la variable_get().

I'm with chx on the "don't babysit broken code" rule. Hiding errors is the best way to allow hidden breakage. I would even argue that we should change it to throw an Exception in that case.

I don't see why. Accessing $_SESSION as an array works just like before.

Yeah, I can't think of a real reason why this would be a problem, either. As I said, I think it's a great way to reduce some of the complexity here; my concern with it really boils down to knowing the PHP does weird, non-obvious-in-userland stuff with its sessions AND with its superglobals. So the combination of the two makes me itch, notwithstanding my excitement.

We still have a explicit factory, that is called DrupalSession::init(). Easy to swap out implementations from there.

I'm loathe to question, because that's worked out so well for me in the recent past, but...it's not a factory if it's a method attached to the same class we'd want to be switching out. The only way to make it pluggable as it exists right now is to switch out session.inc, or go in and change the actual code. And the latter option doesn't qualify as pluggability.

The way to go (if we want to implement that properly) would be to respect PHP settings for sessions. And to allow Drupal and other frameworks (CakePHP, etc.) to share the same session handling. This would require to unbundle our current session and user management... looking at the current implementation of _sess_read() is sufficient to be convinced that we are doing things wrong now.

I've had the thought a few times in working on this that loosening the coupling between users and sessions could be beneficial, but was trying to avoid scope creep...I guess that's less relevant now. Anyway, though, I don't quite get what you mean by this - "respect PHP settings for sessions?" As in, use PHP's default file-based implementation? I don't understand what you're suggesting this interoperability would entail.

There is no reason to make $preventWrite anything else than a bare property. It is FALSE by default, you can set it to anything non FALSE to prevent session writing. It's as simple as that. There is no validation, no access checking to be done. No need for any cruft here.

I agree. Implementing that level of defensiveness is over the top.

Arguably we could also have a $session->currentUser() method...

I also agree - nifty idea, something actually usable for modules, I think. It would add some substantive purpose to the similarly-oriented function I had in earlier iterations.

I'm with chx on the "don't babysit broken code" rule. Hiding errors is the best way to allow hidden breakage. I would even argue that we should change it to throw an Exception in that case.

+1, and +1. Drupal doesn't do backwards-compatibility; in my mind, that goes hand-in-hand not babysitting broken code. If we do, then it just makes it harder to understand why an API change broke something. Same reason I love type hinting!

I don't dispute that doing bizarre things to $_SESSION is cool. I dispute that there's sufficient value in doing so to justify changing the behavior of a standard PHP superglobal rather than the common and self-obvious session() wrapper function. "Because it's a nifty trick" is not a sufficient reason.

Also, while I see from the last patch that we're pulling the array back out to save it, what happens if PHP tries to end the session without that explicit step? Does it still work because the session object is a pseudo-array, or does it die in a twisted pile of molten metal?

In general, I am of the mind that we should be layering functionality, not changing language functionality, unless there's a very good reason to change it. Sometimes there are; I haven't been convinced of that here yet.

I also don't follow the "we're already doing non-standard stuff" claim. Please elaborate.

Regarding $preventWrite, negative properties are non-intuitive. Most properties are affirmative properties, so throwing in a periodic "but don't do this" property is bad for DX. That's why simple setters are more self-documenting. They're an affirmative verb with a negator if appropriate.

Fair enough. I want to say that its appeal is more than mere nifty trickery, but when I really think about it, the best I've got is that "it allows other code to transparently use $_SESSION in the same way they used to, but allows us to effectively know and control its contents." In other words, it itself could end up being a bit of a babysitter for broken code. But...well, I dunno, I'm really waffling there, and I think it's because I just don't have enough experience to have a wise opinion on that one. So I'll defer to whatever y'all figure out.

what happens if PHP tries to end the session without that explicit step?

Which explicit step? Not 100% sure what you mean here, but...

Session closure ordinarily occurs during the PHP shutdown process after object destruction, which is why we used to use the call to register_shutdown_function('session_write_close'); it moved session closure prior to object destruction. Now, the way that session closure is handled is through the DrupalSession::closeHandler() and DrupalSession::writeHandler() methods - both of which are protected, which means that PHP can 'directly' invoke them if a call to session_write_close() (or somethin like it) is made from inside the object. The object destruction problem is handled by adding that call to the destructor, such that object destruction itself triggers session closure. Bottom line, though, is that PHP can't end the session without going through those handlers; that's the whole idea behind session_set_save_handler(). If someone tries to end it from outside class scope, they get a visibility fatal error; we provide methods for them to do it indirectly. So...I get the feeling this doesn't address your question, but it's probably a good explanation to have up anyway :)

Yeah, good point about negative properties. I originally wrote it as Session::allowWrite; chx asked me to take it out because its original use case (we were doing funky stuff with the user's uid and wanted to prevent corrupting session data) was obviated by the other changes in this patch. I think leaving it in for test purposes, though, is perfectly reasonable - but it should be changed back to a positive property.

why would you ever want to have a property for telling yourself that you've been written to that you have to explicitly update?

why not just do this instead:

<?php
class DrupalSession {
  protected
$updated = FALSE;
  public function
__get($name) {
    return
$this->$name;
  }
  public
funtion __set($name, $value) {
   
$this->updated = TRUE;
   
$this->$name = $value;
  }
  private function
__destruct() {
    if (
$this->updated === TRUE) {
     
$this->_writeClose();
    }
  }
 
/* ... */
}
?>

I guess this is "the" issue. Sadly, this conversation sort of ended here... what could we do to make the changes simple enough so that they could get accepted on Saturday?

Now when we more or less have decided to use the HTTP Foundation from Symfony2, I think it makes total sense to use the session handling that comes with the HTTP Foundation from Symfony2. We are already carrying that code weight, so to speak.

It's really solid, comes with a well defined storage interface and a few different implementations, like a PDO implementation that we could use as a base.

Code is here: https://github.com/symfony/HttpFoundation/blob/master/Session.php
Storage implementations are here: https://github.com/symfony/HttpFoundation/tree/master/SessionStorage

This is also mentioned here: #1263478: Identify pieces of Drupal that could be replaced by Symfony code

Version:7.x-dev» 8.x-dev

Missed this first time around.

The Symfony session storage interface looks decent to me, had been thinking of shipping core with a second implementation that does not depend on the database possibly in Drupal 8, if there's a base there already (native and file) that's handy.

I'm less keen on the actual session class though at first look. What is this 'flash' stuff? Is that the rough equivalent of drupal_set_message()?

Also need to figure out what we do with $GLOBALS['user'] if attempting this, that is not a bad thing but it's not a straight conversion at all.

I agree that we definitely need to look closer on both the session class and it's storage implementations. I'd recommend we do that before we start any other refactoring of the session system. I'd happily give this a shot.

Regarding $GLOBALS['user'] it's to my understanding that the issues raised here will be solved by the context system providing $context['user'] instead. The context will always be locked, so changing the user object should be impossible, if I understand things right.

There'll need to be a context provider for the current user, but context system itself is not going to be doing that work.

If we don't want the session system to do it (which seems fine - feels like session should just tell us if there's a session at all and the uid if it's an auth user), then it needs to happen somewhere else. If this patch lands before the context system it'll at least need to move the creation of $GLOBALS['user'] somewhere else.

Correct, there will be a handler that is responsible for $context['user'] that, I suppose, initializes the session in the background. I'm not sure yet on the particulars of that.

I haven't looked at the Symfony session handling at all yet, so I have no opinion there, yet.

Since I worked on the original patch, and have done a fair bit of Symfony tinkering, I'll see if I can't try to rock out some patches. I do agree with @catch that the 'flash' stuff is really an odd thing to have in there...that really smells like something that they've been meaning to separate, but haven't. And just looking at that, it's enough for me to say "no." That's a really confusing interface to present to our devs, but expect them not to use.

That said, I do like their separation of Session from the SessionStorage, and I'd strongly support following that pattern in rolling ours. Of course, we're immediately gonna get into some pluggability pattern questions when we do that...but hey, that's a good thing to keep pushing on :)

Note that in the implementations we've done in patches so far, the session object registers itself (via session_set_save_handler() in its own constructor. That's poor architecture, and ruins testability. Whatever we do, we need to get rid of that.

And one note though, re: #22. Overriding $_SESSION makes me really, really squeamish now, given the challenges with php's objects-as-arrays situation as I documented in #402896-155: Introduce DrupalCacheArray and use it for drupal_get_schema().

And also...it's really just not an API or an interface. I think we should just dismiss that approach. Give folks a class, and document it. That's what the other frameworks are doing, we should feel fine doing it too.

We could possibly use our own Session interface and class but re-use SessionStorage or at least make things compatible, then file an upstream issue with Symfony about the flash stuff. I agree with sdboyer it feels like a showstopper at the moment.

Oh yeah, I could totally dig using their storage but our own Session class/interface.

Chatted with @dixon_ and @catch. We're pretty well agreed that we will discard the Session class and write our own, but utilize the storage. My own list there is:

  • SessionStorageInterface: +1
  • NativeSessionStorage: +1
  • ArraySessionStorage: meh
  • FilesystemSessionStorage: -1, this seems just like Native but done in userspace with a configurable path...unless I missed something?
  • PdoSessionStorage: -1, replace with our own DbtngSessionStorage(that's the idea, don't take the classname literally)

@catch pointed out that we could use NativeSessionStorage in the installer. Maybe even as the default everywhere, pending benchmarks.

We also agreed that we like the idea of simply deleting files from HttpFoundation that we aren't using, in order to reduce developer confusion with yet more APIs they're not supposed to touch.

Sooo...now we just need HttpFoundation and the autoloader to make it in :)

Just FYI, did you look at http://drupal.org/sandbox/pounard/1263216 it's a sandbox project, which makes you able to use either native PHP session, either one more complex with a storage engine. I even wrote a storage engine using any cache backend for storage (when using my Redis cache backends, it really roxxes, believe me). You should also look at the issues of this sandbox, most of them have been opened by a colleague of mine, admin sys and developer, specialized in the LAMP stack and ZF.

EDIT: That said, Symfony's one looks really good.

@#50 Native sessions in PHP are likely to be faster on a single box, single user, with a fast harddrive or in a tmpfs, but it also subject to big locks, and as soon as you will have a lot of AJAX and or many tabs for the same user it can be like hell for performances.

I talked with sdboyer a bit last night. I am on board with #50 with the exception of removing classes we're not using. If there's a class we could use but decided not to (Symfony's session class, for instance), we should document why but not remove it. It's still way too early in the cycle to be pruning code for the sake of pruning. We can revisit that question when Drupal 8 is in alpha or beta.

I also really like the idea of using PHP native session handling for the installer, and for other "degraded" states. That sort of strategy should help make Drupal much more robust, and have fewer wonky hacks for such conditions.

I don't think we should remove the classes immediately, but we should leave the option there. Either way it makes sense to document things like the decision to use or not use specific bits.

Issue tags:+symfony

Tagging this to make it easier to identify all of the Symfony-related issues.

I like the Symfony inclusion of session handling, but I'm afraid of how to efficiently lock session writing and session reading to avoid concurent access (multiple browser window or parallel AJAX request). Either you can big lock the entire session either you can lock on a per entry key basis: first will generate a "giant lock" (not elegant and really slow) but the other one may create dozens of them even if they do not concur themselves. One think that I really like with actual D7 session handling is the lazzy session read and write (writes only if modified) which greatly diminishes the potential race condition (but still leaves some).

@pounard any solution that loses the lazy session loading we added in D7 will be a non-starter, IMO.

I have no idea how significant that concurrency & contention issue is, but my gut says it's fairly unlikely? If so, the value of adopting an interface and some pluggability would be that an alternate session storage handler can be implemented that does key-level locking. Seems to me to be tough to do that with file-based or the structure of our current sessions table, and I don't think remedying it could be performant. A Redis-backed implementation could prolly manage it more easily, though.

I'm not pro key-level locking, as soon as the session backend is a remote server you will generate an awful lot of I/O. The session concurency situation is a real problem, see this use case: open 10 tabs quickly middle-cilcking 10 links on a news site: your browser loads in parallel 10 pages of the same site: you just generated the concurency situation.

@sdboyer's #50 would the file session storage be easier if say you were using NFS? I can't imagine wanting to ever run that in production but seems like a possible use case for it.

@#62 File storage can be used in tmpfs or ramfs, some web applications are configured this way, it makes sense.

I've opened #1277682: Move responsibility for global $user out of session as a possible interim step for this.

FYI the "FilesystemSessionStorage" is mostly used for functional testing.

@catch I mean, I guess...but yeah, by the time you've got NFS backing your prod, the latency inherent in being file-backed probably makes it a non-starter - to say nothing of the network overhead.

@lsmith77 ah ok, that makes a fair bit more sense.

@pounard haha, I suppose. And I do that on d.o all the time :) It's true, though, that writing to an external system does mean latency, unless maybe you could get by with async writes. But that has its own issues. Really though, this seems like it ought to be a hugely problematic issue, as that's common behavior...so maybe the question is, why isn't it a bigger complaint? Or is it a big known issue, and I've just missed it?

I think most people that uses file storage will use native session handling since PHP does it well since the begining (and uses file locks along with it too!).

re #58 and #60 and 'lazy session handling'. a big part of our lazy session handling in D7 is just not sending down a session cookie for anon users unless they have session data. there is no reason at all for this to be changed if we decide to go for consistent reads and writes for session data.

while not writing session data unless it changed helps us a lot with the race condition, it also doesn't need to change for data-consistency reasons. that's just a straight performance hack (we trade memory usage against writes to the session store backend), and should be assessed as such, regardless of any locking we may introduce.

that being said, i'm on the fence about whether we need to fix the race here - sites that really need consistency in session data can get it via swapping out the session handling code.

IMO, the existing data consistency issues with our variable_get|set|del code are a much bigger concern.

#68 While this discussion is open, I think that it should remains something to think about, any solution may come over time, let's keep this as a background detail but not forget it.

Status:Needs work» Postponed

Let's be formal about it - postponing this, pending the completion of #1178246: Add Symfony2 HttpFoundation library to core, since we're going to build on those libraries.

Issue summary:View changes

Adding an issue summary.

Project:Drupal core» WSCCI
Version:8.x-dev»
Component:base system» Code

IMO, there isn't a *huge* issue with having Session containing an API for session messages. After all, a message is always tied directly to a given session, and the API itself is rather light. As is, not being able to store message types is a problem for us.

There's a related issue for flash messages over in the Symfony issue queue: https://github.com/symfony/symfony/issues/1863. If we can get message types in their flash message implementation, we can turn drupal_set_message() into a thin wrapper around that. I've got a fork starting that work over here that I worked on with Greyside at the BADCamp sprint: https://github.com/deviantintegral/symfony/tree/1863-improve-flash

Title:Refactor session handling (and Security: do not allow uid changes accidentally)Switch to Symfony2-based session handling
Priority:Normal» Major
Status:Postponed» Active

The patch is in, this is back in business.

@#66 There is a lot of complaints, you just don't hear them:) Just kidding (not that much, admins often yells but no one hear them) but not a lot of people actually knows that when you use a native session, it will be locked from the moment you read it to the moment you actually write it, i.e. the full page generation time in most case. This means that parallel AJAX requests for the same user will be mutually excluded, this is really bad. There is only two way to break this, and both includes not using the native session: either don't lock the session and let the last one who writes it win, either provide a per key locking.

Project:WSCCI» Drupal core
Version:» 8.x-dev
Component:Code» base system

There is zero reason for this to happen in the sandbox. Refactoring of session handling can happen in core and ought to be a self contained patch apart from some user module evilness.

catch: No, we've concluded that since user and session are so tightly coupled right now (itself a problem) we can't really fix one without fixing the other. We tried in #1260864: Convert global $user to a context key. Making user a context key without also rejiggering session handling is more trouble than its worth. If those have to happen together, it's easier to do them in the sandbox.

@Crell - we have #1277682: Move responsibility for global $user out of session already, that could be done as an interim measure to unblock this for core. Or are you planning to submit the initial context patch including a full refactoring of session handling all at once?

I'd agree that session can refactored using Symfony's one outside of the sandbox, and then when the context API is ready we can provide the final glue between them. Session handling has its whole business stuff to do, it's has enough complexity to be treated in an isolated core issue. Then linking it to context is only a final piece of glue that will probably only provide the lazzy loading as new feature.

As catch says in #79 there is ways to separate concerns here, and this may help progressing faster on context API since we'd not have to worry about this (and the final glue code will be, IMHO, quite easy to write in the end).

someone please remove my name from the node and delete/unpublish #1 as you see fit, i find it a travesty and offending that my simple little issue was taken over by this and yet kept my name on it. Edit: did myself.

Issue summary:View changes

Reuse of $_SESSION

remove chx

Symfony 2.1 will have their Session handling overhauled. Changes are in this PR with the full source code here. These changes fix many issues including stale locks, unexpected flash message clearing, and properly implementing the native session lifecycle. In addition, SessionStorage implementations have been better abstracted, as well as many more handlers added. I'm told by lsmith these changes will drop near the end of March.

+1 to using Symfony SessionStorage as each class is built around session_set_save_handler() fulfilling the session life cycling with various storage engines.

As for the locking issue...is it an issue? As a PHP developer I've always been aware of the shortcoming of PHP sessions. That was the nature of the ease of use coming from using sessions. If I was doing a long lived request I would have to call session_write_close() to remove the lock and dump my changes to the storage.

just FYI .. last I heard https://github.com/symfony/symfony/pull/2853 will be merged but without the flash changes for now. There is an on going discussion about them on the mailing list and Fabien wants to think about it some more:
http://groups.google.com/group/symfony-devs/browse_frm/thread/732e30a240...

Symfony 2 session handling is becoming really impressive with this pull request, I like it. If that is compatible with our usage, we have no excuses not using it.

Has anyone had a chance to evaluate Symfony's support for HTTPS sessions and mixed HTTP/S sessions?

I haven't but was going to ask the same thing. I didn't see any sign of it last time I looked though.

grendzy: Are you talking about something similar to this?

bschussek: more directly, I'm asking if the Secure Pages module (which I maintain) will be compatible with Symfony2 sessions. This has been a bit of a fiasco in Drupal 7.

It's actually enough if the session handler completely ignores the protocol / URI scheme (at least optionally), and lets another module step in. The "secure attributes" you propose aren't really needed for this purpose.

Assigned:Unassigned» Damien Tournoud

There are three possible showstoppers that needs to be clarified before we move forward. I'm not actually too worried about the implementation part after that:

I'm going to try to study those three this week as time permit.

Note that if we need to improve the Symfony library, we can still do that with PRs on GitHub directly to Symfony. That's totally fine to do and very much encouraged.

I actually succeeded in making native storage work perfectly fine. The only thing I had to get rid of (for now) is manual session cookie name handling (secure and insecure) but I will fix that as soon as I succeeded in rewriting the database storage (I am doing it right now).

Assigning to me as it is work in progress now. I will need eyes as soon as the first non invasive patch lands.

Status:Active» Needs work
StatusFileSize
new35.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 335411-symfony-session-0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here is a first patch. It's actually working for end-users, but it contains some regressions. The session.inc file header contains a full description, some assumptions about how to solve those regressions, it also highlight various changes, and benefits of using this code.

Please, don't do any code standards comments, I won't read any post containing Dreditor diff's or syntax highlight, this is for pure design discussion only.

The tests won't pass, I didn't fixed it. Most of them do pass but some actually fails (there is no WSOD at all on my box, so that's a good start).

Actual patch works gracefully with native PHP session handling (using Symfony's NativeSessionStorage backend) and with the custom DatabaseSessionStorage backend (that can be found in Drupal\Core\Session namespace).

Please before commenting on the patch (its looks messy because it actually modifies the entire session.inc file) read fully the session.inc file header first and take into account every comments I made into that. Consider that all comments are tied together, and taking only one out of context will make no sense at all.

Damien's notes are also revelant to this issue.

  • Regarding #1131986: Session handling fails on multiple switches between http / https and #1050746: HTTPS sessions not working in all cases those will have to be addressed in a follow-up issue. While we need to encapsulate the session token provider (the cookie for now) into a single responsability object, we will address the dual HTTP/HTTPS session cookie problem as the test case for this: the design must allow this complex use case to work, I'm quite sure that once we have a design that allows to encapsulate the token provider in a single responsability object and that can allow a specific implementation to solve the dual problem session, it will be a huge win. As the feature is broken in stable core anyway, we can proceed with Symfony's session handling first without addressing this issue
  • Flash messages can be addressed later too, we are now relying on Symfony 2.0 and we have to work with it. Proper Symfony upgrade and potential Symfony features proposals can be done in parallel: we still have to get the core session handling work first
  • Lazy session initialization will be easy to proceed with. The actual patch design already does it by design: the only block is the drupal_session_initialiaze() method: once we'll get a proper session token discovery chain, we need to ensure that if each component can determine the presence of a session token without actually starting it, then the session will support lazy initialization by moving this initialization into the drupal_session_get() method.

EDIT: Side note, the lazy init is not a blocker, we can work without it (all it costs is a few SQL queries more) but it does not imply any functionnal regression. The fact that token providers (for now, the cookies) can determine if a session exists without starting it will imply that the site will work per default with lazy init. If we properly implement this, people would be able to implement their own session token provider, case in which, if those implementation don't support session discovery without start, we would have a graceful downgrade to auto session start per default.

I didn't read any code yet, but I read the comments at the start. From that description this sounds pretty encouraging. Also from a performance standpoint putting global $user; related stuff into $_SESSION seems good to me. I think something like that had crossed my mind looking for queries to remove from core but don't remember opening an issue for it.

Couple of things that I didn't see discussed:

The update of {users}.access is removed in the patch, but not put back anywhere, have a feeling there might not be a nice place to put it back either but that column does get used for things like user administration etc.

Related to this, the "who's online" block that user module provides is already broken if you use a different session storage implementation and it doesn't really work for anonymous users now we have lazy session creation anyway, but it's going to be completely broken by this I think. We may want to scale back that block to only use {users}.access or similar - opened a separate issue for this since it's really a Drupal 7 bug and if it's possible to get it out of the way quick it'd be one less thing to worry about in here #1488630: Who's online block doesn't work with swappable session backends and lazy session creation.

Oh right, good catch (no pun intended). Indeed, the who's online block feature is broken by design from the start, not only with this but with actual stable core too. This may need some tuning, like tracking users accesses in a specific table. This can linked to statistics, it might worth the shot making stats being storable in various backends (some might be extremely performant, with ASYNC support and stuff) then use that to compute the block. Dunno, I guess the issue you opened will solve that, anyway it needs to be decoupled from session. The best way would probably be to track user accesses with listeners (hooks or modern listener pattern) to update the {users}.access column.

Drupal.org actually runs a patch that puts users.access in its own table. Maybe core could do the same?

@webchick Time to switch to #1488630: Who's online block doesn't work with swappable session backends and lazy session creation for this discussion, but yes I think it can!

One blocker found: drupal_save_session() behavior: Symfony session storage backends set by themselves PHP session handler functions, thus we cannot hook them easily, Drupal needs the write handler to check drupal_save_session() before actually saving (and must avoid the save if the result is false): this statement disallows us to use the Symfony storage backends unmodified.

EDIT: And found a nice 3 lines workarround, next patch will reveal it. Nope.

Re-EDIT: Symfony 2.1 version can handle this with the SessionHandler proxies, but 2.0 does not allow to hook at the right moment in runtime to keep this feature.

Just go ahead and use 2.1. We are for the kernel patch. Don't waste time on 2.0. We'll be on 2.2 by the time Drupal 8 ships. :-)

StatusFileSize
new53.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 335411-symfony-session-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here is another patch, most efficient one. Note that this is only a proof of concept, while code seems stable it is for brain storming purpose only.

  • Moved the cookie handling into a component called the SessionTokenProvider. This component is injected into the Session object which will trigger the token send or destroy on various method calls such as Session::invalidate(), Session::save() etc...
  • The token provider class is now hardcoded, but in the future it could be a fully configurable provider chain (meaning that the session token could be passed by different manners, not only using cookies, I'm thinking about CLI environments or other SSO contextes).
  • Overrided the Symfony\Component\HttpFoundation\Session class as Drupal\Core\Session in order to achieve the upper statement. This should still remain easy to port to Symfony 2.1 I guess.
  • Fixed session unit tests, they now pass (except for the drupal_save_session() problem, which cannot be solved without Symfony 2.1). I didn't fixed any other part of core (except where WSOD would happen) this means that anonymous users cannot have a session if the code uses directly $_SESSION superglobal instead of using Session::(get|set|remove)()
  • Had the chance to briefly speak with some Symfony dev on IRC, you should see my comments updated in the header of the session.inc file which explains everything
  • I still didn't restore the HTTP/HTTPS cookie feature, but now that session token provider is isolated from the rest, any folks can do it without breaking the rest session handling, which is good.

I can say that Symfony session usage now works quite well, including the cookie removal for anonymous users when session is empty, as well as lazy session initialization. Regarding lazy session init, this cannot be done for authenticated users as long as we don't have lazy user initialization on first user access, this means we need to get right of the $user global in order to do this.

The overall session.inc code is now almost fat-free, and decoupled from storage and token provider. Then only hardcoded stuff that must move out is the user handling, which depends on the future component container (DIC) once that done, the file will be a candidate for removal and all loading will be done by autoloading and a few configuration access only.

Any review, comments, ideas, design suggestions, rant, etc is welcome, this is a PoC not a final solution, but I think it tends towards a good design.

@Crell oups cross-post, nice, then I should go for it :)

Category:task» feature

Also, why is this a major task? This sounds like a feature request. Our current session handling works fine, this would be a nice-to-have, no?

Or, if there's some reason this particular issue is worth holding up other Drupal 8 issues in the queue, that needs to be made a bit more obvious. (and the issue summary updated accordingly)

Our current session handling works, but is global, which conflicts with "works fine". :-) Also, we already have this Symfony code in the repo as part of HttpFoundation, so I see no compelling reason to not use it and reduce the amount of code in core. It's also a place that we are helping to push feedback and improvements back upstream to Symfony.

I don't have any strong feelings on how the issue is categorized. It's changed enough times as is.

Also, symfony sessions now supports our message system! The flash improvements where pulled during the Drupalcon code sprint. Go open source!

Nice, FTR I started working with 2.1 Symfony branch and I have fully working code (except one test case that freezes I couldn't manage to find out why), it includes only session refactoring, not flash messages yet. I will provide a patch soon.

This patch was rolled after the flash message improvements went in: #1497182: Update to latest Symfony 2.1 code. As soon as that lands this issue should become easier.

It landed, now I can get back to this issue. My latest modifications are now not applying anymore due to other core commits, so I'll have to rewrite them, coming back soon with an updated patch for discussion.

Status:Needs work» Needs review
StatusFileSize
new58.89 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Here is a preliminary patch, open for discussion. I run the tests over it just for fun, but I'm quite sure they won't pass, even if working with this patch on my development box actually works well.

Status:Needs review» Needs work

The last submitted patch, 335411-symfony-session-sf2.1-1.patch, failed testing.

Oupsie, it was working as long as I was working from an install without the patch, just had to drop two database fields, now the install doesn't seem to pass at all (WSOD with no error message). I will see this later, just take into account that this patch work as long as you patch a working D8 install and drop the {sessions}.uid and {sessions}.ssid fields.

If I remember well, the 2.0 version was actually able to install Drupal, so this is probably a minor bug, anyone with courage, run your xdebug:)

Issue tags:+API clean-up, +PSR-0

Tags change.

StatusFileSize
new58.9 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Thing I found it. You redeclared system_update_8006

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 335411_114_session.patch, failed testing.

Status:Needs work» Needs review

pounard: can you please provide an updated summary of what this patch does. I get a bit lost in the code.

The installation fails at the point where it is setting up the DB. It gets as far as setting up the variables table. I didn't get any error messages (I think). It just kicked me to the login screen (which won't work since there is no user table).

Status:Needs review» Needs work

As per #118 this needs work.

StatusFileSize
new48.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 355411_120_session.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

As per #112 & #118, installation with this patch applied is broken.
I have spent some time (character building??) stepping through install.core.inc with xdebug and have found that it's breaking during the execution of the batch process returned by the 'install_profile_modules' task.

This makes sense as the batch api is still using $_SESSION (form.inc & batch.inc). -- ftr a grep of the includes directory reveals that authorize.inc, database.inc and bootstrap.inc also still manipulate $_SESSION directly.

Attached is a patch which makes a start at replacing direct $_SESSION access with drupal_session_get() in form.inc and batch.inc.
Still getting errors, but I need to get some sleep, the following is a brain dump for the morning:

  • Error @ core/lib/Drupal/Core/Session/Handler/DatabaseSessionHandler.php:48 but they're obfuscated by the executing batch process.
  • Looks like the session write() is failing when trying to write to the database.

Patch also replaces usage of session_id() with drupal_session_get()->getId() in install.core.inc, not sure if that was actually causing problems yet, but likely will in future.

And here's the error:
Recoverable fatal error: Argument 1 passed to Drupal\Core\Database\Query\Merge::__construct() must be an instance of Drupal\Core\Database\Connection, instance of Drupal\Core\Database\Driver\mysql\Connection given.

Which is weird because the mysql driver Connection is a subclass of Drupal\Core\Database\Connection... :-\

Wow, fails have evolved. The original failure was only due to the fact that I needed to change the session table schema. The later failures are probably due to more recent commits.

Using the Native* Symfony classes, the $_SESSION direct access is not recommended, but still works partially because the internal attribute bags are referenced into the $_SESSION array which makes them being persisted with the session, this should not be a huge problem right now. We should focus on fixing the upgrade path first (really this is not easy).

EDIT: After some chats with Crell a few days ago, the right solution would be to force the installer and updater scripts to use the PHP native (file based) session handling for upgrades and install, to ensure that we actually can break stuff into core and still be able to access the update script.

For people asking for more explainations about how all of this work, you should first read the Symfony 2.1's Session handling documentation first (if there's any), my patch add only one feature: the abstracted session token handling (right now implemented using cookies as PHP native behavior emulation).

Symfony session documentation is here: http://symfony.com/doc/master/components/http_foundation/sessions.html

(Note: This is for a dev version so may not be 100% up to date, but I think it's very recently added so should be fairly up to date.)

StatusFileSize
new62.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 335411_124_session.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's an updated patch that gets to the installing modules part. Errors on language stuff.

It seems like it runs through the installation of modules and it's unclear where it fails but this may be the part that Crell suggested we stop and focus on re-implementing the new installer.

Something that brings up a red flag for me is that even with the patch in #124 there are 261 remaining references to $_SESSION. It seems like the goal of this patch is to always use the session as handled by the symfony library. If it is the case that we have that many changes to do in order for this issue to be complete, then we've got a lot of work to do.

Can someone check my logic here? Is this our next step? Remove all the $_SESSIONS?

StatusFileSize
new82.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 335411_127_session.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok.... so I followed this idea and ran into a few patterns that provided easy changes, many challenging exceptions, and general confusion if this is even worth the effort.

But still I pushed forward.

Here's a patch that attempts to replace all of the uses of $_SESSION with Symfony's fluid syntax for using the session. The main patterns I used are:

  • Transform isset($_SESSION['something']) to $session->has('something')
  • Transform $_SESSION['something'] = $thing to $session->set('something', $thing)
  • Transform $thing = $_SESSION['something'] to $thing = $session->get('something')
  • Transform $thing = isset($_SESSION['something']) ? $_SESSION['something'] : array(); to $thing = $session->get('something, array());

Please let me know if this is the right path to take. Because if it is, we will have to think about how we want to handle multi-level arrays. I'm not sure if I can do this:

$session->set('something', array('big', 'giant', 'array')

Also, I think I am calling the static session properly but reviewing that code shows me that we may need to adjust the default session storage type logic.

cosmicdreams: Your bullet points are a correct transformation.

Although Symfony Session Handlers provide the correct PHP functionality to still access the $_SESSION variable, the primary advantage of using the Symfony Session class is to provide clean unit testing. In unit tests you are able to replace the NativeSessionStorage with MockArraySessionStorage in the Session::__construct and run a full suite of tests using the same code/interface.

I agree with cboden. We should fully switch over to Symfony's session API, entirely, for testing ability. We do not need to do so all in this patch, though, if it's easier to leave that for follow-ups.

OK, created a follow up issue here: #1545680: Convert all uses of $_SESSION to symfony session syntax.

Will continue to effort to convert $_SESSION there, and continue to diagnose why #124 fails. I am "lost in the woods" on why the installation is failing and could use some help in finding the root causes.

Issue tags:+wscci-hitlist

Tagging

Nice stuff in here, review below :)

+++ b/core/includes/authorize.incundefined
@@ -26,11 +27,11 @@ function authorize_filetransfer_form($form, &$form_state) {
   // Get all the available ways to transfer files.
-  if (empty($_SESSION['authorize_filetransfer_info'])) {
+  if ($session->has('authorize_filetransfer_info')) {
     drupal_set_message(t('Unable to continue, no available methods of file transfer'), 'error');
     return array();

Looks like the check is the wrong way round here.

+++ b/core/includes/bootstrap.incundefined
@@ -921,6 +921,7 @@ function variable_del($name) {
   global $base_root;
   static $cache_hit = FALSE;
+  $session = drupal_session_get();
   if ($check_only) {

Looks like you don't use this anywhere in this function. the next chunk is already another one.

+++ b/core/includes/form.incundefined
@@ -4729,7 +4732,10 @@ function batch_process($redirect = NULL, $url = 'batch', $redirect_callback = 'd
       // Set the batch number in the session to guarantee that it will stay alive.
-      $_SESSION['batches'][$batch['id']] = TRUE;
+      $session = drupal_session_get();
+      $batches = $session->get('batches', array());
+      $batches[$batch['id']] = TRUE;
+      $session->set('batches', $batches);

Yes, it might make sense to think about flatter session variables :) Probably not in this issue though. Maybe revert the $_SESSION['batches'] changes for now? Not sure..

+++ b/core/includes/session.incundefined
@@ -4,511 +4,323 @@
- * Session handler assigned by session_set_save_handler().
+ * This file is the first Symfony session usage test. It works gracefully but
+ * some core features had to be removed in order to make it work:

As I understand it should code blocks describe and explain the current behavior. And not how it's different than the old system. By the time it is commited, the old system is no longer relevant. So parts of this might be better suited for a change record and other to be rewritten.

Correct me if I'm wrong here. Haven't read everything in here, there might be a reason for this.

+++ b/core/includes/session.incundefined
@@ -4,511 +4,323 @@
+ *  - The user fetch has been decoupled from Database session storage, thus it
+ *    make one extra SQL query per authenticated page run: we cannot avoid this
+ *    in order to decouple the storage from the user handling. May be in a late
+ *    future we could actually write the serialize user token data into the
+ *    session itself thus avoiding this extra SQL query (as Symfony does per
+ *    default in its Security component).

Just having read the comment, haven't looked at the code yet. I'd argue that this is actually a good thing and from what I can read below, the goal is to get a lazy loading user object (which should probably also allow to get a real user entity, not the crippled thing that global $user is now).

+++ b/core/includes/session.incundefined
@@ -4,511 +4,323 @@
- * @return
- *   The user's session, or an empty string if no session exists.
+ * @return Drupal\Core\Session\Session

Not sure but I think a description line for @return is still required?

+++ b/core/includes/session.incundefined
@@ -4,511 +4,323 @@
+    if (version_compare(phpversion(), '5.4.0', '<')) {
+      // FIXME: Path relative to my own environment
+      require_once DRUPAL_ROOT . '/core/vendor/Symfony/Component/HttpFoundation/Resources/stubs/SessionHandlerInterface.php';

That FIXME can probably be removed, doesn't look custom to me?

+++ b/core/includes/session.incundefined
@@ -4,511 +4,323 @@
+    $user = db_select('users', 'u')
+      ->fields('u')
+      ->condition('u.uid', $session->get('uid'))
+      ->execute()
+      ->fetch();

This shouldn't be a db_select() IMHO. db_select() is quite a bit slower than db_query(), especially when called early in the bootstrap.

+++ b/core/includes/session.incundefined
@@ -4,511 +4,323 @@
+    elseif ($user) {
+      // The user is anonymous or blocked.
+      return drupal_anonymous_user();
     }
-
-    // Likewise, do not update access time more than once per 180 seconds.
-    if ($user->uid && REQUEST_TIME - $user->access > variable_get('session_write_interval', 180)) {
-      db_update('users')
-        ->fields(array(
-          'access' => REQUEST_TIME
-        ))
-        ->condition('uid', $user->uid)
-        ->execute();
+    else {
+      // User does not exists anymore or session data has expired.
+      return drupal_anonymous_user();
     }
-
-    return TRUE;
   }
-  catch (Exception $exception) {
-    require_once DRUPAL_ROOT . '/core/includes/errors.inc';
-    // If we are displaying errors, then do so with no possibility of a further
-    // uncaught exception being thrown.
-    if (error_displayable()) {
-      print '<h1>Uncaught exception thrown in session handler.</h1>';
-      print '<p>' . _drupal_render_exception_safe($exception) . '</p><hr />';
-    }
-    return FALSE;
+  else {
+    // No session uid is set, meaning the session does not exists or the user
+    // is anonymous.
+    return drupal_anonymous_user();

Do we need these three special cases? Can't we just have a final return with somehow merged comments?

Sidenote: The patch is quite hard to read due to huge blocks of totally unrelated removed code.

+++ b/core/includes/session.incundefined
@@ -4,511 +4,323 @@
+  if (!$session->isSaveEnabled()) {
+    // In case business layer specifically asked for not saving the session, we
+    // need to unregister potential handlers the Symfony session storage
+    // component may have registered for us. Considering that this function is

I've never seen us use the term "business layer", not sure if something else should be used.

+++ b/core/includes/session.incundefined
@@ -4,511 +4,323 @@
+  if (empty($user->uid)) {
+    // Ensure there is no 'uid' set in session. Keeping an outdated or empty
+    // session 'uid' attributes would taint the Session::isEmpty() check and
+    // give potential false positives, thus forcing empty session to be saved.
+    $session->remove('uid');
   }
-
-  if (drupal_session_started()) {
-    $old_session_id = session_id();
+  else if (empty($user->uid)) {
+    // Ensure the uid is set into session, forcing it to reflect the user really
+    // being logged in and may prevent some security hijack attemps.
+    $session->set('uid', $user->uid);

The else if should be !empty, no? it's currently the same check as above.

Also, a simple else { should be enough, no? We have empty() and !empty(), there is nothing else left :)

+++ b/core/lib/Drupal/Core/Session/Handler/DatabaseSessionHandler.phpundefined
@@ -0,0 +1,64 @@
+ * Drupal database session handler, load and save sessions using the {sessions}
+ * table throught DBTng.

DBtng => the database or something like that. We don't use DBtng in official documentation, that's the secret code name :)

+++ b/core/lib/Drupal/Core/Session/Handler/DatabaseSessionHandler.phpundefined
@@ -0,0 +1,64 @@
+class DatabaseSessionHandler implements \SessionHandlerInterface {

I am not sure if it's officially defined but all code that I've seen and written so far did use use statements instead of absolute namespaces in the code (except it's dynamic in strings and so on).

+++ b/core/lib/Drupal/Core/Session/Handler/DatabaseSessionHandler.phpundefined
@@ -0,0 +1,64 @@
+  public function open($savePath, $sessionName) {
+    return TRUE;

Interface implementations should have a "Implements Interface::method()" docblock.

+++ b/core/lib/Drupal/Core/Session/Handler/DatabaseSessionHandler.phpundefined
@@ -0,0 +1,64 @@
+      db_delete('sessions')->condition('sid', $sessionId)->execute();

I think all chained method calls should be put on separate lines.

db_delete()
->condition()
->execute();

+++ b/core/lib/Drupal/Core/Session/Session.phpundefined
@@ -0,0 +1,156 @@
+ * This SessionInterface implementation add the SessionTokenProviderInterface
+ * support for delegating session token management to a specific injectable

addS and a trailing space.

Also, I think the first sentence should be on a single line and as short as possible. (Still not sure if it needs to be <80 characters or not, but this is probably too long).

+++ b/core/lib/Drupal/Core/Session/Session.phpundefined
@@ -0,0 +1,156 @@
+  /**
+   * @var \Drupal\Core\Session\TokenProvider\SessionTokenProviderInterface
+   */
+  protected $tokenProvider;

Should also have a description I think, not just the @var.

See http://drupal.org/node/1354#classes for coding standards on class docblocks.

+++ b/core/lib/Drupal/Core/Session/Session.phpundefined
@@ -0,0 +1,156 @@
+      throw new \LogicException("Cannot enable or disable storage when not using a NativeSessionStorage implementation");

Should be "Can not".

+++ b/core/lib/Drupal/Core/Session/Session.phpundefined
@@ -0,0 +1,156 @@
+   * Does this session is empty.
+   *
+   * FIXME: This is the most absurd implementation that could ever been written
+   * but there is no clean solution because bags can not be directly accessed
+   * via protected attributes, and they don't have either a count() or isEmpty()

Maybe "Returns TRUE if the session is empty".

Also, I think we use @todo and not FIXME.

+++ b/core/lib/Drupal/Core/Session/Session.phpundefined
@@ -0,0 +1,156 @@
+    // Lazzy send the authentication token to client, this will avoid to send
+    // the token at session start time, thus if session is empty this ensure

Is this short for Lazy Larry? :) -> should be Lazy.

+++ b/core/modules/system/tests/session.testundefined
@@ -131,7 +131,10 @@ class SessionTestCase extends DrupalWebTestCase {
-   */
+   *
+   * FIXME: Because we are moving out cookie handling, we cannot ensure this
+   * behavior until we restored it. Temporarily disabling this test.

Hm, not sure I understand this. We can't simply disable tests that don't work anymore, no?

Same for those below.

Powered by Dreditor.

Sorry Berdir, please disregard the patch in #127 and review the patch in #124 instead

I tried debugging #124 for a bit. There's actually nothing wrong with the installer; Drupal installs fine with the patch applied as long as you install via the command line. The problem only happens when you try installing via the UI, but nothing else works via the UI either :) Even once you have Drupal installed, you can't log in or anything else. It's just broken.

I got as far as discovering that the immediate cause of all this is that in drupal_session_commit(), $session->isSaveEnabled() seems to always be returning FALSE. Thus, no sessions are getting saved at all. I stopped looking into it after that, but hopefully it's a good starting point.

***

+++ b/core/lib/Drupal/Core/Session/Session.phpundefined
@@ -0,0 +1,156 @@
+      throw new \LogicException("Cannot enable or disable storage when not using a NativeSessionStorage implementation");

Should be "Can not".

FYI, "cannot" is actually the preferred usage.

@cosmicdreams

With one or two exceptions, my review is about the code in session.inc and Drupal\Core\Session. Those parts seem to be identical in both patches, so it doesnt really matter which one I reviewed. No?

I've been looking at bits of code being written implementing Symfony components and I noticed something...

Many of the 'use' statements in use are using '\' for a directory separator. While this normally presents no problem on the Windows platform, I recall seeing Linux kind of choking on it.

Is there a new policy on this matter?

Those actually aren't directory separators. Those are namespace separators. The syntax comes from PHP. http://us.php.net/namespaces

@Berdir Your review is exactly the kind of I tried to avoid in this issue. This code is far from being ready for production yet and the only important thing is general code design review, not Dreditor english grammar nazi kind of reviews.

Aside of the rest, your first three point are revelant!

I think what pounard meant to say was, "Thanks a lot, Berdir, for taking valuable time to carefully inspect my code. I plan to focus on the architectural elements of your review for now, and work on cleanup stuff after." Didn't you, pounard? :P

Exactly, but after saying it 10 times, that sounded clearer that way :)

@webchick: Do'ah! My Homer Simpson moment for April.

Thanks for the link, too. :)

Thanks David_Rothstein!

Now if you could kindly explain how you were able to discover that I would be extremely grateful. I tried really hard to find the core issue (spent like 4 hours on it) and couldn't find that root cause.

StatusFileSize
new17.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 335411-modules-node-user-do-not-patch.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

@cosmicdreams: Here's the patch I mentioned to you in IRC.

I'm not certain it'll work as advertised as is, however, the patterns are basically translated as suggested. I included references to 'Drupal\Core\Session\Session' where needed, as well as initialized $session = drupal_get_session() in each function where the new form is used.

Let me know if I need to do any further work on this, please. I left the issue as 'needs work' as I don't want the patch tested prior to your review.

Paul

I didn't get far and dreditor failed to let me select just this one line but the ternary operator pattern you're converting can be simplified with $session->get($name, $default)

$filter_data = $session->has('node_overview_filter') ? $session->get('node_overview_filter') : array();

$filter_data = $session->get('node_overview_filter', array());

#143 and #144 for now, the converting all session handling to symfony session is not in scope of this patch. Please continue your pursuits of this conversion in the follow up issue here:

#1545680: Convert all uses of $_SESSION to symfony session syntax

Good move

Thanks David_Rothstein!

Now if you could kindly explain how you were able to discover that I would be extremely grateful. I tried really hard to find the core issue (spent like 4 hours on it) and couldn't find that root cause.

Wish I had a simple answer, but it took me a while too.

Basically, the installer was failing when it tried to start the module installation batch, so I put some debug statements in the batch API in core/includes/batch.inc and discovered that the batch was being written to the database on one page request, but not being loaded on the next request because the token that it was using to load it didn't match what was in the database. The token comes from drupal_get_token() which uses session_id(), so that suggested a more general problem with the sessions. When I printed out the session ID on each page request, I saw that the two requests had different IDs.

At that point, it helped that I knew Drupal only creates a session for anonymous users if there is something to store in the session (and at that point in the installer you're still an anonymous user)... So I started to debug the code that actually writes the session, and saw that it was being called but that it was bailing out at the place I described above.

That's it :)

Issue tags:-PSR-0+kernel-followup

We'd obviously be using PSR-0 :-) .

Indeed. This not really a kernel-followup thought, but tagging with it will trigger more reviews! Please, people, need DESIGN reviews!

First directed questions:

  • Do you think that storing the user into the session instead of querying it each page hit sounds secure? What alternatives would we have if not? I'd really like to get rid of this SQL query.
  • Do you think the provider abstraction is right? Do you see you use cases of having this session provider elsewhere or should be using something else? In Symfony world, they would use the Security component instead, but it's way too complex compared to what we want to achieve, and it would forces us to adapt a lot more than just the session.
  • Now that the kernel has been commited, do you think we should provide session: 1) in the DIC 2) attached to some listener for the session token provider 3) both?

Can only answer about the user part.

The goal is to make global $user (or it's equivalent) a fully loaded, real user entity. So IMHO, we should only store the uid in the session and then do a lazyload through drupal_container(). So maybe there could be a helper function to just decide whether the user is logged in or not, so that we don't need to load it for that.

I don't think that's your concern though, there already is an issue where this is discussed. Not sure about storing the user data completely in the session. Imagine that you block a user. That means the user status needs to be reflected immediately, if it's stored in the session, then he might still have access (Not actually sure about that part).

The goal is to make global $user (or it's equivalent) a fully loaded, real user entity.

No, I don't think we want that. The complete user entity may not be required at all in a request, so for such requests this would be nothing else than a performance drain. There's already an issue for discussing this concrete detail in #361471: Global $user object should be a complete entity and the discussion should stay in there. Also note that there's a counter-proposal to that issue that suggests to completely eliminate the $user and replace it with §session only in #1549526: Change global $user into $session.

Hum, anyway since the DIC and kernel are commited, we need to refocus this patch, and as I proposed the first version, I'd like to re-orient the design now that core evolved.

First, I'd like all people interested in this issue to read briefly the security component code and try to understand its design, and then evaluate the following proposal.

First, I'd like to say I don't want to integrate the Security component, it's a good component, but a very complex one, and right now I don't think it fits with our own usage. So, we should start without, maybe in Drupal 9 access checks will be revamped using it, but now is not the time.

Here a proposed battle plan:

  • Remove the proposed "token provider" and replace it with a very early bootstrap listener: this listener has only one goal, settings the session_name() and session_id(): we decouple the session token provider from the session code (YAY!)
  • Move our custom cookie handling logic as its own component, that the above listener needs as a dependency (we can skip this step for early coding and use the PHP native one as a start): we isolate this code to the maximum, no dependency (YAY!)
  • Implement our own micro Security component like listener, following a simplified (really simplified) version of the Security component, we just need to make a passthrought with the user loading as a listener and setting it into the session: we decouple the user handling from the session (YAY!). Expose this component into the DIC for easy get/set by procedural, login, etc code: no invasive modification of the actual login and logout workflow (YAY!)
  • Remove all overrides I done for Drupal\Core\Session stuff, and use the native Symfony component, thanks to the listener the session_name() and session_id() will be set correctly before first session hit and we don't need what I added anymore: removes ugly custom code (YAY!)
  • Once this works, override the Symfony Session object in order to add the drupal_session_save() emulation. We'll have some weird work to do here, but it will be isolated from the rest, and we can make the above steps work before starting thinking about this point. We'll need to do this anyway, but consider it as a weird hack and not a real super top feature. But we need it for backward compatibility and avoiding invasive changes all over core.

Once all this done, we can really for good remove the session.inc file: yes, really, no procedural wrappers anymore, the only one we need is drupal_container (YAY!).

Any thoughts? I will try to implement this within the week.

That said, @Berdir, yes, user deactivation is the black spot in the picture, we need to do this extra SQL query if we want to be able to check that. Maybe later, we could start to think how to implement this in another way, but right now, if we want to use Symfony Session component, we'll add one extra and mandatory SQL query for staying iso functionning with actual core.

+1 to the general plan.

What do we gain from bypassing the token system and placing our logic in the event listener. It seems like since we have custom id logic, it should be there.

I played around with this some today and did some research. I though it would be interesting to see how Silex does it and I don't think its a exact match because of how Silex works but the event/listener idea looks fairly similar.
https://github.com/fabpot/Silex/blob/master/src/Silex/Provider/SessionSe...
It seems registers a service that provides session objects and then on a request uses that service to toss a session on the request object and on response takes care of the cookie logic. Very similar idea I'd say.

Ok, some news. I started reimplementing it from scratch, session.inc has now only three methods: drupal_session_initialize(), _drupal_load_user() and drupal_session_commit().

Because we need the user to be loaded early, we cannot really lazy initialize the session so here what happens: the drupal_session_initialize() only serves the purpose of injecting the Session object into the DIC, then call the _drupal_load_user() which loads the Session from the DIC (and not by parameter, the idea behind that is to set it into some listener later) and load the current user if a UID exists.

Already, we see that security (user load) should not be a global anymore but maybe provided by the request or a factory into the DIC: session would be loaded by dependency but won't be coupled to it anymore. If we do this, we can definitely remove the drupal_session_initialize() and _drupal_user_load() and remove the session.inc file definitely (yay!) the only last piece being the drupal_session_commit() which can live as a kernel listener (and being more consistent along new code).

After that, the drupal_session_commit() changed a bit, but it just checks if there's not UID AND session is empty, case in which it destroys it (enable the next hit to go throught page cache if user is anonymous and has no messages).

I removed the dual HTTP/HTTPS session cookie handling (this should be done into the DrupalProxy implementation I think, already existing into the previous patch) and I removed the custom identifier generation (if we don't handle cookies ourselves, we cannot generate the id ourselves, PHP native API is a bit all or nothing).

Symfony actual session API doesn't let us handle this by ourselves without extending all of those components altogether: Session, AbstractProxy, NativeSessionStorage, SessionHandler. This sounds a huge waste, because we could live with using most of SF's implementations (at least we could keep the SessionStorage, which unlike what it sounds is not about storing data on disk, but storing data into memory, SessionHandler is the persistant storage handler, the only one we really need to implement ourselves at this point).

So, we have two features regression, which are transparent for the rest of world but might cause security regression, so from this point, from what I said, I'd really like security guys to come and check and tell me if those custom ids and dual session cookie really are important, case in which we'll have some problems and might need to do some Symfony PR so that we can handle cookies ourselves back again.

Aside of that, it's almost fully working. I didn't run test, but tried the site upside down. There is some problems thought, sometime SF session handling when called late in drupal_session_commit() will cause some PHP warnings (headers already sent) for which we have to find some way to work around.

The biggest problem is the update, the new storage doesn't rely on the same database schema, so if you try to use it on a current D8 install, it will WSOD with some PDOException, which is totally normal, but the update.php script will too, and this is more a problem for us: solution I can propose here is to developer another SessionHandler implementation, called LegacySessionHandler that would use the older schema just for the update.php script (using a current system module schema version test) and then be forget once done (code will still be here, but never used). Crell seemed to tell it was an acceptable solution if it works flawlessly.

I will commit the code tomorow or the day after. Any notes, comments, ideas, suggestion from here are more than welcome.

Unless I'm mistaken, then

  1. the custom IDs are absolutely required for multi-site/sub-domain/subdirectory setups, so session leaking can be controlled and allowed or denied in a safe manner.
  2. the dual HTTP/HTTPS session cookie handling is essential for many sites, especially commerce sites.

sf should probably see a use-case for 2. but not necessarily for 1., since I'm not sure whether they're dealing with multi-site scenarios driven by the same code-base at all (haven't seen any code in sf to that extent yet).

The Dual HTTP/HTTPS functionality should be pushed upstream into Symfony. At this point it probably wouldn't make it into 2.1, but that's fine because we're still working with master.

I don't know enough about the custom IDs to know what to do about that.

I was afraid both features could be useful. @sun Could you give some more precisions about the session leaking problem? I guess it's a problem that would happen only when the different sites on the same install are in the same domain right?

@Crell That would be a massive change in Symfony's code, I don't know if they're going to accept such new concept it would require a lot of thinking around it. It would be easier as a first step to ask them to adapt a bit their API so we can attach our own implementation to it more easily.

Yes, my guess is along the lines of #159. But I'm not too familiar with the advanced features and security aspects being involved. In any case, I do not think there is any feature in our session handling that can be removed. People who know this code a lot better are probably: @chx, @Damien Tournoud, @Heine, @catch, @slantview, and potentially others.

Ok, thanks, I will try to take some time and ask those people for details.

StatusFileSize
new50.74 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Ok, here is a proof of concept patch.

Some additional notes, that adds up to the notes from #153:

  • As said before, we loose custom session id generation and HTTP/HTTPS custom session cookies handling. These need to be restored, and are two of the three most important points we need to focus on.
  • I didn't change any unit test, so all session unit tests will fail.
  • The update problem is the third most important point we need to focus on, I'll explain it below.
  • There is a lot of @todo, // FIXME and whitespace problems, please don't review that; at this stade they are not important, we need to find solutions to the other problems first.
  • I experienced two PHP notices: one is due to ob_flush() being called while there is no buffer to flush: my guess is that the kernel patch actually did something wrong, it should probably use ob_flush_end() on the onTerminate event instead of ob_flush(). The other is because drupal_session_commit() happens after headers have been sent: one of the operations in drupal_session_commit() attempt to destroy the session cookie in an edge condition, which makes this happen. We'll figure a nice way to avoid this later (it's not important, but if someone has a solution please speak).

You can test it, your site will work, just install a nice D8, apply the patch, but don't run the update.php and everything will work out.

The update problem is this: considering that Symfony does not handle dual session, I had to drop the 'ssid' field of the {sessions} table. This is not a problem, the new SessionHandlerInterface implementation doesn't care about this field, because it's nullable in the database (so it's ignored). Because the user handling is now done by storing a key into the session bag instead of being a field of the database (in order to be consitent with Symfony's session handling), the new session handler will have PDOExceptions being thrown on db_merge() calls because the 'uid' field is not nullable; this will happen until the {sessions} table has been updated by the update script.

We cannot leave empty and unneeded fields into the database, this wouldn't make any sense, so we need to update this table.

Consider this problem: we cannot use the new session handler, until the database has been update, but we cannot afford to do runtime checks after it's done, because it's useless and CPU/IO consuming. We have some kind of chicken and egg problem: in order to work, the new code needs the database to be updated, in order to be able to update, we need to have updated first.

I have a partial solution throught, if I remember well it's a Crell's idea: using a LegacySessionHandler implementation working gracefully on old table schema, until it's not being updated (actually, this will be what will run if you apply the patch so you can test the patch without scratching your head). This is theorically really good, we can then use it in update.php. But consider this other problem: in order to run updates, the root user must login first, which he cannot do since the rest of the site can't work until update is done.

I thought other solutions, but I don't like most of them:

  • Using a variable that tells the handler to use, and change it on update, but this means that if the user accidentally removes it on a working site, the default implementation would go back to the legacy implementation and throw WSOD/exceptions.
  • Using a global $conf key that tells the implementation to use, defaulting it to the legacy session handling, and write the settings.php file while updating in order to comment out the line or change the value to the default implementation. This probably would work, but it would need to be done at install time, and this kind of ugly to deliver a non working default configuration that needs to be changed at install.

Any ideas about this problem are welcome. We can solve it very easily by requiring the user to set $update_free_access to TRUE for the first update.php call, but aspilicious stand against for security reasons, because during the update.php run, anonymous attackers that can access the web server could write anything into settings.php. This is a non viable option if we want to support mutualized environments.

Time to scratch your heads!

Hum after reading what I wrote, there is a quick and easy solution: make the {sessions}.uid field having a default value of 0 (anonymous) in the latest D7 version. The upgrade to D8 will require people to update their D7 before doing that, which means that the new session handler will be able to transparently use the old database schema ignoring the columns with default values. No problem to solve anymore, and because D8 is not stable anymore, we can afford to break D8 to D8 update until we aren't at least in a beta stage.

Got a solution for HTTPS cookie handling too. FYI I'm working on restoring the custom cookie handling into the DrupalProxy implementation, it works fine. I have restored the lazy session cookie send feature that allows finer tuning with reverse proxy caches and stuff. Now, the last bit of this problem is that D7 keeps two session identifiers, one for HTTP and the other for HTTPS, and is able to load the session with either of them. Using SF's session handler, we can work with only one identifier to load the session being, so we need a way to be able to guess either one of the identifiers using the other (crypting the HTTP session identifier using the drupal private key in order to be able to restore it and load the session would work, but is it secure?). Any ideas welcome.

Status:Needs work» Needs review
StatusFileSize
new49.2 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

New patch attached, almost the same as the one above plus:

  • Lazy cookie sending to client restored, this means that cached pages for anonymous users will never set a cookie.
  • Restored custom session cookie handling, but only for HTTPS, see comment #165.
  • Removed update.php changes, we won't need it at all if we apply method from #164

Install should work just fine, PHP warnings seem to have disapear thanks to better cookie handling, only update will fail.

Still missing the HTTPS cookie handling, and once done that, D7 features will be fully restored, but on top of Symfony's code.

Switching to needs review for cursiosity, but this won't pass.

EDIT: And no, install won't pass. Batch seems broken, I should have fixed them. For people who want to test: install a clean D8 from git, drop manually the 'uid' database field from {sessions} table, and apply the patch.

The last submitted patch, 335411-166-sf_session.patch, failed testing.

Status:Needs review» Needs work

It looks like there's discussion of HTTP/HTTPS discussion in Symfony already: https://github.com/symfony/symfony/issues/3247

If we can get that upstream, that's a win for everyone. (I'll comment over there shortly to complete the loop.)

Oh nice, that would solve a huge part of the problem for us and make it really easy to integrate.

Moved to Crell

Status:Needs work» Needs review
Issue tags:-API clean-up, -symfony, -wscci-hitlist, -kernel-followup

#166: 335411-166-sf_session.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+API clean-up, +symfony, +wscci-hitlist, +kernel-followup

The last submitted patch, 335411-166-sf_session.patch, failed testing.

Status:Needs work» Needs review

The test bot will revert the issue status as needs work as soon as the test has failed.

Any reviews of patch in #166 and #163, #165 considerations are more than welcome. We almost have a clean path for this, but I don't want to leave any design mistake in there that could have negative impact later.

Issue summary:View changes

Add deep link to first useful comment.

StatusFileSize
new35.94 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sf_sesion-335411-173.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reroll. Still doesn't work because of something seemingly unrelated(can't find entity_load_multiple in user.module).

Status:Needs review» Needs work

The last submitted patch, sf_sesion-335411-173.patch, failed testing.

Posted #1672804: Add a default 0 value to the {sessions}.uid database column I hope Drupal 7 maintainers will be OK with that. If this patch passes, the upgrade path problem will be solved without any code to do on our side (except dropping the field in an update function).

Ok I'm teasing a bit but I have been fixing it on my side, I will post a patch soon. I'm working on tests, and actually I have 147 passes and 19 fails on the Session test, and everything else seem to be OK. As soon as I narrow down this to one or two, I will comment post the patch. I also removed session.inc because its code is now about 100 lines (more or less) and moved it to bootstrap.inc, it makes the patch cleaner to review/read.

Status:Needs work» Needs review
Issue tags:-API clean-up, -symfony, -wscci-hitlist, -kernel-followup

#173: sf_sesion-335411-173.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+API clean-up, +symfony, +wscci-hitlist, +kernel-followup

The last submitted patch, sf_sesion-335411-173.patch, failed testing.

I need to get back to this patch, especially during DC so people can take some time to discuss @Munich.

pounard: in #176 you say that you're patch is more advanced than #173 and is about 100 lines of code. Can you post your current patch or provide guidance on how you've accomplished that?

@#180 I have to rebase with current core, the method is still pretty much the same as older patches, but with the recent changes (DIC and kernel) the patch now would be radically different. I have to rewrite it from scratch I think. I still need reviews about the method, not the details, which I all explained upper, in the "proposed plan" in the issue summary.

Regarding the upgrade path, what exactly needs to be upgraded. The differences of Symfony's handling of session is merely syntactical. Do we need to modify the session table dramatically. Can't the upgrade path merely be the modification of the session table from one schema to another?

No you can't, here's why:

If you update the code before upgrading the table, the update script will WSOD because it uses the existing logged in user. The non set default value of the uid column will make the backend throw PDO exceptions as soon as the code is updated and the user refresh the page with the Symfony session handling in place of the legacy session handling.

The other way arround, updating the database first is not possible, because in order to update, you must be logged in.

One intermediate solution is to implement the Symfony session handler in order to use this uid field, but sad as it is, the Session component is not supposed to know what is a uid and where to fetch it, so it would be sad hack. I don't want to explore this path because we would have update related compatibility code into the normal runtime component, which is not something I'd do because it's pure evil and ugly and against simplicity.

Another intermediate solution is to test if the table has been modified, then use the backend accordingly (either the new one if up to date, or an intermediate one that take care of this uid field with a foo value if not) but this means that we would have to do a full set of install/update specific components for session handling, and do SQL query at runtime to determine the current schema, I think this is a no go too.

All the intermediate solution represent, in term of amount of code, a lot more than just switching the 'uid' field default value to 0 in D7: because the update to D8 will require the latest D7 version, this would be almost transparent and very efficient, and would avoid D8 code to care about doing ugly compatibility runtime checks while it only needs to do it once in the whole site lifetime.

Requiring the latest D7 in order to upgrade to D8 is completely reasonable. I think requiring setting $update_access to TRUE for a major version update is also acceptable. In fact, there's even discussion of not having an update process for D8 but just a migration path, but we can't bank on that just yet. ;-)

I do not see any process problems mentioned here. While so far we only picked Symfony components that's kind of hard to have a sechole in them (though not impossible but I would say there are extremely few places where there even could be a sechole, maybe where it sends the headers? anyways, not many) this issue is a cornerstone of security (obviously). I would be very very hesitant to do this in Drupal 8. Let our release practices mature I would say.

Here's the thing: if there's a sechole in the Symfony session component then all the projects using Symfony as a library need to coordinate a release and there are more and more of them.

chx: How are security releases of Symfony at all related to this issue? The hitch here is the internal upgrade process so that your site doesn't break while trying to upgrade from D7/our session handling to D8/Symfony session handling. Security release schedules have nothing to do with that.

This is a new concern I have brought up.

It's unrelated to this issue. Security update cycles are already being discussed in #1451056: [policy] How to handle unforeseen diversion of Symfony code in stable/API-locked Drupal core?. The code in question (Symfony's session handling) is already in core as part of HttpFoundation, which we are already using. Update cycles have no relevance to this issue at all. Please stay on topic.

Status:Postponed» Needs work
Issue tags:+security

I agree that this security policy should be arrived at before this work can be committed. I don't think an unfinished policy for determining if an external library can be used based on security implication should be the basis for blocking people from doing good work! :) Thank you for bringing this up, chx. Tagging with security.

I'd also like to continue working on this issue, even if it is not a candidate for 8.x, it still might be depending on how things goes and @cosmicdreams seems really eager to work on this issue, so let's continue development.

Issue summary:View changes

New summary

I do not see this as the best place to spend our time, but, obviously, feel free. I added a note to the issue summary about my concerns and that's it.

Issue tags:-security

No. The Symfony Session code is already in core as part of HttpFoundation. We're already using it extensively. Any update-cycle security issues there are already there, and this issue does not affect them. Repeat: There is no new external library being introduced by this patch. Any concerns about 3rd-party-library security are already being dealt with elsewhere.

Bringing that topic into this issue is FUD. chx, q0rban, please do not torpedo a 190+ comment issue with FUD.

This issue is still targeted at Drupal 8, because eliminating superglobal use is necessary to segment the page properly for master-request/sub-request logic, which is part of WSCCI.

Crell,

I was in no way trying to torpedo a 190+ comment issue, but more importantly, I see no value in trying to attack and accuse in this way. If I misspoke or made a blunder, I apologize. I know very little about this new work, but I am supportive of it, and thought that moving it back to NW and adding the security tag was appropriate. Maybe I don't know what it's supposed to be used for, but I thought that tag would flag the security team that this issue has security implications. If you feel the issue doesn't have security implications, I would love to know why! :)

Thanks for all your hard work on these issues.

Status:Needs work» Needs review
StatusFileSize
new32.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sf_sesion-335411-193.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Tried to reroll this today. Probably broken.

Status:Needs review» Needs work

The last submitted patch, sf_sesion-335411-193.patch, failed testing.

q0rban: The security "issue" is a process question. "What do we do if there's an upstream sec hole in this code, OMG?" It's not a claim that this code is insecure. It's general fear about relying on 3rd party components.

That concern is not invalid. It is, however, off topic. It would be relevant here if and only if this was the only security-relevant code we were pulling in from Symfony. Contrary to chx's claim above, there are plenty of other pieces of Symfony now that we're using that could, in theory, have security holes. (Of course, Symfony's also had a security audit so I don't expect any gaping holes, just as I don't expect gaping holes in Drupal, either.) Using the session parts of HttpFoundation does not appreciably increase our security process risk any higher than it already is by using the request handling parts of HttpFoundation, using the YAML file parser (that's external data, remember), using the ESI handling out of HttpKernel, etc. The security process risk is valid, but there's another issue where it's being discussed already.

Meanwhile, we have been trying desperately for literally months to get any attention on this issue at all for reviews. When we finally, finally have a hint of motion shutting it down with "postpone to Drupal 9" is going drive away any inkling of attention we are getting.

Meanwhile, having looked deeper into the HttpKernel subrequest handling, ESI logic, and so forth, I am not sure that we could get it to work properly without converting to Symfony sessions. (I don't know for sure that's the case, but I think it's likely.) So if we want the "fancy sub-request auto-block-caching magic" (which is one of the main reasons we're doing all of this refactoring), I'm growing more convinced that this issue will be a requirement for it. Even if it's not, it's horribly ugly to convert almost all of our request handling over to a clean OO system that's nicely integrated but leave session handling as a PHP 4-era superglobal that's hard coded to an SQL database and a particularly ugly hack to create a pseudo-user. Really, module developers deserve better than that.

Hence why I started calling FUD. Perhaps a bit strong, and perhaps not intentional, but still the case. We need this patch to move forward, and mixing it up with separate discussions that will only drive away reviewers is directly harmful to WSCCI.

@cosmicdreams Thanks, I will try a review. Did you encounter any problems?

@cosmicdreams New core files (core/Session/*) are missing in your patch.

Thank you for your explanation, Crell.

@pounard, yes many. And as you noticed I missed some major issues. So I'll check in on this again and let you know of specific ones.

StatusFileSize
new47.19 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

This is a re-roll with the addition of a bit of clean up, and the declaration of the "session" service within the DIC. Not quite sure I'm registering the service correctly, mind taking a look?

Status:Needs work» Needs review

status

Status:Needs review» Needs work

The last submitted patch, 335411.patch, failed testing.

@#202 The registration looks good to me, but I'm not a SF2 expert. The "Drupal installation failed" is probably due to the fact that SimpleTests does black magic with session, but I might be wrong.

Did you manually run the session tests only? I think this is the goal to achieve before pinging simpletests gurus for potentially fixing this.

If the compiler does the registration itself, we need to get rid definitively of session_initialize() (or most of its content) including the 'session_storage_backend' variable part, the only pieces we'd want to keep there are: the SessionHandler interface ugly hack (if not moved into a specific autoloader) and the user instance registration.

Thanks for the patch, I'll try to move forward as soon as I have a couple of hours of free time.

Reviewed this. Let's do it. I hope dearly I won't need to eat my words later, but let's do it.

As pounard predicted, after applying this patch to a working D8 site I was logged out and couldn't log in (since session is so changed). I was then able to drop by database tables and reinstall Drupal 8 without issue (with the patch still in place).

After that, I still couldn't log in. Trying to get proper error messages now...

ok, one problem in session.inc is that is_https used to be a global but isn't anymore. That means when it is used on line 256 it is undefined.

Should we code up the equivalent?

Also on line 293 the use of $sid is broken. $sid does not have a value at that point. Time for a more extensive review.

@cosmicdreams Can you post the most recent patch you can? Or is there a sandbox/git branch where you are working on? I will have some spare time tonight so I'd like to be able to review your work/work on it myself.

Sure, I'm currently working on HEAD + the patch provided in #202 by Rob Loach. What you said about needing to manually modify the schema for the session table is really driving home now. Appling the patch will show you want I'm talking about.

I'm deep into work stuff at the moment, but I'll come back to this tonight and check into this issue a few times today.

Issue summary:View changes

Added security note

Yo folks, looking into this again this evening. Looks like session is never comitted because the isSaveEnabled is returning function is returning FALSE.

Other questions / comments. I'm asking these because I'm trying to find out more about this system. Sorry if I'm nit-picking.

+++ b/core/includes/session.incundefined
@@ -4,329 +4,307 @@
+ *  - We cannot delete session by uid, this regression may be the worse. We can

Symfony 2.1 provides a way of retriving the ID of session. Does that help?

http://api.symfony.com/master/Symfony/Component/HttpFoundation/Session/S...()

+++ b/core/includes/session.incundefined
@@ -4,329 +4,307 @@
+ *  - As written upper, the cookie handling is decoupled from core session
+ *    handling and storage.

Should be revised to "Cookie handling is decoupled from core session handling and storage" and reference function that does cookie handling

+++ b/core/includes/session.incundefined
@@ -4,329 +4,307 @@
+ *    This means that session will almost always be started and components put

Should we remove checks test whether a session is started then?

+++ b/core/includes/session.incundefined
@@ -4,329 +4,307 @@
+ *  - Right now, flash messages are not being used, they will be in the future
+ *    but 2.0 Symfony's HttpFoundation component can not allow us to do that

Outdated, we're using Symfony 2.1-dev now.

+++ b/core/includes/session.incundefined
@@ -4,329 +4,307 @@
+ *  - The Symfony's session handling does not allow a storage direct access per
+ *    design, except if we keep the storage reference somewhere: this means that

direct access to what? be clear in this comment.

+++ b/core/includes/session.incundefined
@@ -4,329 +4,307 @@
+ *  - Regarding the above statement, Symfony's session handling design also
+ *    disallow us to use the $_SESSION super global directly. While this is a

Perhaps disallow is the wrong word to use here. There is nothing that actually prevents a developer from accessing and manipulating $_SESSION directly. Some of the comments above this one directly informs the reader that accessing the $_SESSION super global is a tactic that future code will pursue.

+++ b/core/includes/session.incundefined
@@ -4,329 +4,307 @@
+ *  - If we switch to 2.1 version of Symfony, we will have to port some specific
+ *    stuff, such as the DatabaseSessionStorage. Aside of that nothing should
+ *    change for us. The only exception seems to be for Flash messages, but we
+ *    will port Drupal messages to Symfony Flash messages only once the core

2.1 is our base, this sounds like a necessary @todo in order to get this patch working.

+++ b/core/includes/session.incundefined
@@ -4,329 +4,307 @@
+  // @todo: See if it fits with the new plugin API once commited.
+  // @todo: In all case, see if it fits with CMI too (early init).

Is this really a job for the plugin system, or would getting this class from the DIC be better?

+++ b/core/includes/session.incundefined
@@ -4,329 +4,307 @@
+  // @todo Replace this with container definitions instead, for lazy init
+  // and compilation purposes.
+  $storage = new DrupalSessionStorage(array(), new CookieOverrideProxy($handler));

as a result of registering session services in the bootstrap, do we still need to do this?

+++ b/core/includes/session.incundefined
@@ -4,329 +4,307 @@
+  $session = drupal_container()->set('session', $session);

Is this redoing the work bootstrap is already doing (when it registers session stuff)?

+++ b/core/includes/session.incundefined
@@ -4,329 +4,307 @@
-  global $user, $is_https;

why is the $is_https global removed here but not in other functions?

+++ b/core/includes/session.incundefined
@@ -4,329 +4,307 @@
+  if (!$session->isSaveEnabled()) {
+    // In case business layer specifically asked for not saving the session, we
+    // need to unregister potential handlers the Symfony session storage
+    // component may have registered for us. Considering that this function is
+    // only run when Drupal is doing its proper shutdown, we can safely assume
+    // the session has not been automatically saved by PHP at shutdown.
+    // Notice that this check is duplicated into the Session::save() method in
+    // order to avoid accidental save. This check here only exists for minor
+    // performance reasons.
+    return;

After install, this test always passes. Then the function immediately returns.

That leads us to question. Is the session ever saved enabled?

also _drupal_session_load_user() states that it should Replace this with a container lazy initialization function instead.

How is that done?

Also, it appears that the following functions aren't used in core anymore (other than tests):
_drupal_session_destroy()
drupal_session_destroy_uid()
drupal_session_regenerate()

which would mean that except from where it is used in drupal_session_commit(), drupal_save_session() isn't used in any other function.

And since drupal_save_session() reads like it's supposed to do the save thing as $session->isSaveEnabled() I don't know why we are keeping it.

in drupal_session_initialize() i tested my hypothesis that the code that is setting the session within that function is unneeded and was proved correct. After removing that code I still have a proper session object once drupal_session_commit is invoked.

Looked at user_authenticate and found that I was able to properly authenticate it's just that the session was not annotated appropriately after the successful login attempt.

It's pretty clear that sessions are left in a "started" and the session storage handler is not left in an "active" state. That is why $session->isSaveEnabled is always FALSE.

You're working in HEAD of which sandbox? Can I access it? Can I see the code in the actual state?

Some answers to #202:

We cannot delete session by uid, this regression may be the worse. We can
Symfony 2.1 provides a way of retriving the ID of session. Does that help?

In case the user is logged in, destroying a session is just calling session_destroy(), case in which we don't need to destroy a session by UID, for all other cases of Drupal core destroying a session by UID needs to be able to fetch another session than the current one by UID, this is by design just not possible with SF2. So retrieving the current session id doesn't help for the use case of destroying other sessions. The only thing we can do here is relying on session lifetime and hopping that it will be garbage collected one way or another by the session handler: this is not our thing to do in application level anymore.

As written upper, the cookie handling is decoupled from core session handling and storage.
Should be revised to "Cookie handling is decoupled from core session handling and storage" and reference function that does cookie handling

Wait a minute, are you trying to improve the file docblock? This whole piece of documentaiton is here only for the development phase, I hope that we will write a completely new documentation once the patch is working fine. Before that, we need to get to this point first.

This means that session will almost always be started and components put
Should we remove checks test whether a session is started then?

Nope, the existing tests in code are important because the session is still lazy started, but at component init.

Right now, flash messages are not being used, they will be in the future but 2.0 Symfony's HttpFoundation component can not allow us to do that
Outdated, we're using Symfony 2.1-dev now.

Not completely outdated, we're still not using flash messages, and that's the important point. If I remember correctly, there's an issue for that.

The Symfony's session handling does not allow a storage direct access per design, except if we keep the storage reference somewhere: this means that
direct access to what? be clear in this comment.

There is three important components in SF2 session handling: the session "storage", the session "handler" and the session object (some kind of factory). The session handler is what makes it persistant (database, PHP native files), the session storage is what holds data into memory (the $_SESSION array). By design, session data must be accessed throught the session factory object which compose the other two elements. By design, because of this composition, the end user of the session is not supposed to know what storage it holds, and therefore cannot access is interface directly. That's why per design we should never ever use either one of the storage nor the $_SESSION array directly, because it violates the encapsulation and interfaces, and will make everything explode the moment a site admin will configure his own Drupal to use a different storage and or handler.

Regarding the above statement, Symfony's session handling design also disallow us to use the $_SESSION super global directly. While this is a
Perhaps disallow is the wrong word to use here. There is nothing that actually prevents a developer from accessing and manipulating $_SESSION directly. Some of the comments above this one directly informs the reader that accessing the $_SESSION super global is a tactic that future code will pursue.

Indeed, there is nothing that technically prevent a developer to use it.

If we switch to 2.1 version of Symfony, we will have to port some specific stuff, such as the DatabaseSessionStorage. Aside of that nothing should change for us. The only exception seems to be for Flash messages, but we will port Drupal messages to Symfony Flash messages only once the core
2.1 is our base, this sounds like a necessary @todo in order to get this patch working.

It seems that I started working with a 2.1 copy for my patch on Crell's advice, so I think this comment is outdated and should be removed. The flash messages stuff is already comment upper and should be dropped here too.

@todo: See if it fits with the new plugin API once commited. @todo: In all case, see if it fits with CMI too (early init).
Is this really a job for the plugin system, or would getting this class from the DIC be better?

I think putting this into the DIC is a better approach for us. Plugin system is highly devoted to making UI and listing plugins, in our specific case session handler and storage is not supposed to be configured in UI: it's not supposed to change over time in a single site. The DIC as long as it is overridable using a custom module bundle (not already doable, but is a target feature of D8) is enough here. CMI is out of scope this too early in bootstrap, and some other components might depend on it.

// @todo Replace this with container definitions instead, for lazy init and compilation purposes.
+ $storage = new DrupalSessionStorage(array(), new CookieOverrideProxy($handler));

as a result of registering session services in the bootstrap, do we still need to do this?

This is exactly the kind of code that must be replaced by DIC registration/compilation in core Bundle.

$session = drupal_container()->set('session', $session);
Is this redoing the work bootstrap is already doing (when it registers session stuff)?

It is registering the Session object in the container with the session key. This is exactly, with the point upper, what must be replace by DIC registration/compilation in core Bundle.

global $user, $is_https;
why is the $is_https global removed here but not in other functions?

All globals must die, if I remember well this one is not used anywhere else in core. As it is highly coupled with core session handling, this must be removed with it and evolve in the same patch.


+ if (!$session->isSaveEnabled()) {
+ // In case business layer specifically asked for not saving the session, we
+ // need to unregister potential handlers the Symfony session storage
+ // component may have registered for us. Considering that this function is
+ // only run when Drupal is doing its proper shutdown, we can safely assume
+ // the session has not been automatically saved by PHP at shutdown.
+ // Notice that this check is duplicated into the Session::save() method in
+ // order to avoid accidental save. This check here only exists for minor
+ // performance reasons.
+ return;

After install, this test always passes. Then the function immediately returns.
That leads us to question. Is the session ever saved enabled?

There is a few use case where this is being used, in cron an some other edge cases. This probably needs to be kept for a while until those are figured out.

Status:Needs work» Needs review
StatusFileSize
new50.71 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
  • Removed extra useless code from session init, RobLoach's DIC patch is sufficient by itself to fully build the session object.
  • Added header_sent() check into the cookie proxy implementation, to avoid some weird PHP WSOD
  • Did some minor cleanup, removed useless code, simplified some pieces

It's working on my box now. The main problem I have is I got PHP notices due to double session_start() calls. This is due to the fact that the early bootstrap container needed for config is actually not merged to the kernel container, and session objects are instanciated twice. This is a huge problem and config guys should revisit their way of working this stuff out because it causes serious trouble here.

In order to use the patch, install a d8 from git, apply the patch once installed, and type (if using mysql) in you sql console:

alter table sessions modify uid int(10) unsigned default 0;
delete from sessions;

Then try to login and login, everything should be working fine.

Status:Needs review» Needs work
Issue tags:-symfony, -wscci-hitlist, -kernel-followup

The last submitted patch, 335411-218.patch, failed testing.

Oups, I don't know what happened here! I'll provide a new one tomorrow or this weekend.

EDIT: Again, it is usable, just use the procedure I described above.

Issue tags:+WSCCI, +symfony

Bad bot!

Category:feature» task

Don't see how this can possibly be described as a feature.

StatusFileSize
new47.17 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Here is a version where the installer actually works.

Status:Needs work» Needs review

Then let's test it :)

Status:Needs review» Needs work

The last submitted patch, 335411-WIP.patch, failed testing.

It was on purpose I didn't tested it, tests themselves needs fixing! EDIT: But it's interesting to see the result, thanks.

Ok, random notes about current status, patch to be posted here soon:

  • Ok, some tests are passing, some other are still not fixed and will cause some PDOExecptions.
  • Installer works!
  • Update won't work (needs Drupal 7 patch to be commited).
  • Fixed some problems in the Session::isEmpty() method that would prevent anonymous users to have flash messages in some cases.
  • Notices everywhere due a chicken and egg problem: session needs to be in the bootstrap container because session is initialized before the kernel. Session is initialized soon because of page cache. Compiled kernel will override the currently registered session service and instanciate a second once, causing PHP notices due to a second session_start() call. Conclusion: page cache should happen after kernel initialization: this will cause performance regression in page cache scenario, but would actually eliminate the chicken and egg problem allowing us to define the session service only into the CoreBundle. Any ideas?
  • The upper change would elminate most of errors and failures of the test bot, making this patch almost RTBC, but this is a quite important architectural change which need not to be taken lightly.

EDIT: Another solution can be to register the session a synthetic service, just like database is registered, but I'm not fan of this solution because it makes it impossible to be overriden. I'm gonna try this implementation for the sake of making the test bot happy: we can chat about this later in follow ups.

Status:Needs work» Needs review
StatusFileSize
new62.83 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

This should start to be encouraging test wise.

Status:Needs review» Needs work

The last submitted patch, 335411-228.patch, failed testing.

Status:Needs work» Needs review

Test log actually shows a lot of things actually passes, but some fatals make it go crazy.

Status:Needs review» Needs work

I applied the patch and was able to discover that drupal_save_session is now only used in one place: UpgradePathTestBase.php (in the system module's tests).

Going to manually test now.

I have a patch that I am testing locally that removes those drupal_save_session calls. I'll post the patch before I leave for home tonight.

Thanks, good to ear! From now on I will post interdiff to all my patches, it starts to stabilize (I hope).

According to the comments above I thought this patch was almost ready so I started a serious review. But it is in fact still very rough. It contains a ton of @todo's. I don't think all of them are still relevant. In some place the docs are in bad shape. I suggest that pounard or someone familiar with this patch goes through every line and clean it up as good as possible. (when the fatals are gone)

That way others can dive in much faster. I don't feel like being able to participate at this moment due to the high numbers of @todo's in the patch

+++ b/core/includes/session.incundefined
@@ -4,530 +4,149 @@
+ * @todo Move this into a lazy user loading once Drupal will got a fully
+ * featured component registry (aKa DIC).
  */

We have a DIC?

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -64,6 +64,22 @@ public function build(ContainerBuilder $container) {
+    /*
+     * @todo This needs to be uncommened as soon as the bootstrap container is
+     * tackled out and real container inits before the session.

Is there an issue for this?

+++ b/core/lib/Drupal/Core/Session/Handler/DatabaseSessionHandler.phpundefined
@@ -0,0 +1,87 @@
+class DatabaseSessionHandler implements \SessionHandlerInterface {

Don't write stuff like "\SessionHandlerInterface". "use" it on top of the file

+++ b/core/lib/Drupal/Core/Session/Handler/DatabaseSessionHandler.phpundefined
@@ -0,0 +1,87 @@
+   * @Implements SessionHandlerInterface::open().

I think it is @implements without the capital and I think we need the full path \Drupal\Core\Session\Handler\SessionHandlerInterface

+++ b/core/lib/Drupal/Core/Session/Proxy/CookieOverrideProxy.phpundefined
@@ -0,0 +1,153 @@
+class CookieOverrideProxy extends SessionHandlerProxy {
...
+   * Defautl Constructor.

Constructs a CookieOverrideProxy object.

+++ b/core/lib/Drupal/Core/Session/Proxy/CookieOverrideProxy.phpundefined
@@ -0,0 +1,153 @@
+   * Get current session identifier from cookie, if any.

GetS

+++ b/core/lib/Drupal/Core/Session/Session.phpundefined
@@ -0,0 +1,96 @@
+   * Disable session save, at commit time session save will be skiped and
+   * session token will not be sent to client.
+   *

80 chars max for a summary

+++ b/core/lib/Drupal/Core/Session/StaticSessionFactory.phpundefined
@@ -0,0 +1,61 @@
+      /*
+       * @todo Keeping this code as container registration code that should be
+       * used instead when the bootstrap container disapears
+       * ¶
+      // Register the session service.
+      $container->register('session.storage.backend', 'Drupal\Core\Session\Handler\DatabaseSessionHandler');
+      $container->register('session.storage.proxy', 'Drupal\Core\Session\Proxy\CookieOverrideProxy')
+        ->addArgument(new Reference('session.storage.backend'));

$container isn't defined in this class. This can't work... or can it?

+++ b/core/modules/system/system.installundefined
@@ -2215,6 +2184,23 @@ function system_update_8032() {
/**
+ * Make changes on the {sessions} table accordingly to new Symfony session
+ * handling usage.
+ *
+ * @todo This will fail, always. The only way we can ensure update will work
+ * seamlessly is by defaulting the {sessions}.uid column to 0 in Drupal 7, thus
+ * ensuring the new database storage handler will ignore this field when doing
+ * the session db_merge().
+ *
+ * @see http://drupal.org/node/335411

If you're posting a node id use the node id pointing to the correct issue that is fixing this for D7.

Reviewing the logs to make it easier to remove the fatals:

PHP Fatal error:  Call to a member function get() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php on line 1020
Fatal error: Call to a member function get() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php on line 1020
FATAL Drupal\forum\Tests\ForumTest: test runner returned a non-zero error code (255).

and

Fatal error: Call to undefined function Drupal\system\Tests\Upgrade\drupal_save_session() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/Upgrade/UpgradePathTestBase.php on line 126
Fatal error: Exception thrown without a stack frame in Unknown on line 0
FATAL Drupal\system\Tests\Upgrade\BareMinimalUpgradePathTest: test runner returned a non-zero error code (255).

fatals, some page caching and poll vote tests are failing but that will be visible once the fatals are gone

@aspilicious Thanks for the review, as you noticed, it's still not ready standard wise, first step is to pass tests! Although, you indeed raise revelant notices. Here is some random answers:

  • Outdated comments about the DiC, indeed, we have it and we use it
  • There's a plan for tackling out the bootstrap container, and I think there is related issues, Crell &co (WSCCI) will be a better source of information for this. For what this patch is concerned, it's not ours to do, but there is bootstrap container specific related workarrounds in it that needs to disapear if it ends up takled out. Those comments must remain for those who fix that to understand why this code is here
  • About he commented code, yes, there is no $container in this class, it's only sample code kept here to show what it replaces. I kept it here for better understanding of why (and it is documented right upper). This comment will disapear later once this is fully working I guess
  • You are right about the node link, it must be fixed!
  • Thanks for all the coding standard reviews, but the patch isn't ready yet and will move quite a bit: I will try to take those into account while working on it.
  • About the fatals you're right, and that's what cosmicdreams is trying to fix while we're speaking.

I went to bed last night after waiting on the tests to finish for about an hour on my workstation. I'll see if I can get that patch up but it makes some guesses on how to properly reproduce what the current code is doing in the UpgradeTest.

@cosmicdreams: It think its better to use: -do-not-test.patch as extension instead of relying on issue status ...

Especially if it can break a test bot ...

Okay because of the testbot backlog I will repost cosmicdreams' comments with properly named attachments. Please don't set this issue to NR until I'm done.

Original post by @cosmicdreams:

Here is that patch, I tried to test this locally but it appeared to hang simpletest so please don't send this test to testbot.

This patch removes the final calls to drupal_save_session. I'm likely doing something astondingly stupid in my replacement for _drupal_session_write. Please review that closely.

Reposted by xjm

revisiting this for about an hour. Going to try to address #236 and others.

In tests, I get proper failures now. What I see in those failures is confusing. I get an this error:

Call to undefined function Drupal\system\Tests\Upgrade\drupal_save_session()

on line 127 of my UpgradePathTestBase.php test. But I've eliminated all calls to drupal_save_session and line 127 of UpgradePathTestBase.php doesn't contain any code, it's just an empty line.

I don't understand what's going on.

StatusFileSize
new64.46 KB

Here's a patch (properly named so that it isn't tested) that shows what I'm talking about. Don't laugh at the large amount of whitespace I added to prove my point.

Status:Needs work» Needs review
StatusFileSize
new64.46 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

I've applied the patch, did a new install and was able to successfully execute a random test, the tests even passed, the only problem that I had was that I got the "No active batch" error *after* I saw the green test execution summary.

I also run the Session tests with run-tests.sh and it worked and I got "Session tests 133 passes, 8 fails, and 0 exceptions".

So I'm going to be bold and re-upload the patch from #246 (wow) to see what happens with the testbot. Our test queue is currently empty, so we'll survive if the testbot decided to throw up.

Status:Needs review» Needs work

The last submitted patch, 335411_246.patch, failed testing.

Ok, a handful tests threw up but most of them seem to have passed.

That doesn't look too bad :)

The test results from 246 are likely dirty because the patch needs to be rerolled.

Status:Needs work» Needs review
StatusFileSize
new66.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 335411_251.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled against 8.x and, together with Berdir, tried to fix the lost session.

#251 I'd be interresting to have an interdiff please.

StatusFileSize
new65.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 335411_252.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Damn. I forgot to remove a debugging line. Here's the cleaned patch.

Status:Needs review» Needs work

The last submitted patch, 335411_252.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new65.43 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

I tried applying this patch and it applied fine. So I'll reupload the diff

Status:Needs review» Needs work

The last submitted patch, 335411-255.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new65.43 KB
FAILED: [[SimpleTest]]: [MySQL] 47,046 pass(es), 48 fail(s), and 18 exception(s).
[ View ]

Again, but with an updated system.install function

Working on the active batch problem, I think I know what the problem is: the session regenerate does not respect the disable/enable Save flag. That's why it currently works without this patch I think. Then we don't need to keep the session because it doesn't change :)

Status:Needs review» Needs work

The last submitted patch, 335411-257.patch, failed testing.

48 fails and 18 exceptions is a really good score! Thanks cosmicdreams. Now all that remains are figuring out the testing batch problem and make the dual session cookie (https) working again.

We should also have some strong code reviews, Crell has volunteered to do one once we make this patch green. I'll have a chance to look at this patch tomorrow night and push it forward.

Ok, sounds like a plan, we need to get this green.

Status:Needs work» Needs review
StatusFileSize
new634 bytes
new64.92 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Yes!

Testing batch works now, this might also fix a few test (e.g. those that test a test within a test ;)). I had to overwrite the migrate() method and check isSaveEnabled(). The functionality of that is consistent with with current core behavior, not sure if there's a better place for an actual fix. See attached interdiff. There might have been some smaller changes as well but that's the only relevant change I think.

@Berdir, oh that was just that then, very nice!

Status:Needs review» Needs work

The last submitted patch, session-335411-263.patch, failed testing.

Ah the ghost drupal_save_session errors. I kept running into those. Let me apply the patch locally and see if a drupal_save_session snuck in. That function no longer exists after applying the patch and even though I could prove that I couldn't find that function being used anywhere I kept running into that error message.

I never could resolve that, but surmised that it was the result of not cleanly re-installing Drupal. Perhaps we can get to the bottom of this now.

Update tests, LOB 15 passes, 0 fails, and 0 exceptions
Fatal error: Call to undefined function Drupal\system\Tests\Datetime\drupal_save_session() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/Datetime/DrupalDateTimeTest.php on line 87
FATAL Drupal\system\Tests\Datetime\DrupalDateTimeTest: test runner returned a non-zero error code (255).

The DateTime component was commited *today*. I assume there there actually is a call to that function left.

Also, some of those test failures are because I'm currently unconditionally calling $this->container->get('session') in TestBase and some unit tests explicitly define a new container that doesn't contain session. After thinking about it, I guess we can simply remove that line because we throw away that container and session class right below that line, so the next call to drupal_container()->get('session') will get the original one that will do a save because we never disabled saving on it, just on the test container.

@cosmicdreams: Are you currently working on this? I might get back to this and try to fix some of these tests in an hour or two but now I need a break (been working on core for almost 12h now...)

Status:Needs work» Needs review
StatusFileSize
new5.57 KB
new67.7 KB
FAILED: [[SimpleTest]]: [MySQL] 47,658 pass(es), 75 fail(s), and 2 exception(s).
[ View ]

Here's an update.

- Removed drupal_session_save() form the DrupalDateTime tests, this is not necessary as that is already disabled for tests anyway.
- Removed the enableSave() call, as explained above, follow-up calls should actually use the real container again, which never had save disabled.
- Looked at the upgrade path. The uid problem is easy to solve, that's why we have early-bootstrap prepare/fix functions. I also made an attempt at converting the session data to allow existing sessions to work. This is required to get our tests working and being able to test the uid column at the same time. Seems to work, but manually messing with session data is fragile. We probably need to make some more checks (e.g. that the encoding function is actually the one that we expect it to be) but should be able to make a best effort attempt to make it work. If it doesn't work for some sites then that's a non-issue, they can just set the free access flag. This should fix some upgrade tests, somehow logout currently doesn't work on an upgraded site yet, which causes some fails.

Also had a look at some of the remaining test failures:
- Date time setting based on default/user settings doesn't seem to work, haven't figured out why yet. The code is still there but maybe not called? This causes the failures in Date and time and User timezone tests
- The currently logged in and UserBlockTest rely a lot on the uid column and are therefore completely messed up obviously, not sure what to do with that. Don't really see a way to properly support this without storing the logged in users somewhere else, throw it out?

Let's see what's left :)

Nice work with the update tests!

The currently logged in and UserBlockTest rely a lot on the uid column and are therefore completely messed up obviously, not sure what to do with that. Don't really see a way to properly support this without storing the logged in users somewhere else, throw it out?

All those tests doing direct calls to the session table must be rewrote to fetch the current container Session object and do a get('uid') instead. They cannot arbitrary load or check for session anymore and those tests must be dropped if they need this amount of flexibility. Anyway, any test outside of the session tests that attempts a direct access or need this amount of flexibility over the session table are doing it wrong(TM) and must be encapsulated to the strict minimum the API allows them to access.

@pounard: Right but that doesn't work for the currently logged in user block for two reasons:

a) The tests insert multiple sessions for different users and uid's and verify that they have been saved (that part could be removed). I don't think that is possible through an API.
b) The actual block does a query on the sessions table like this: "$authenticated_count = db_query("SELECT COUNT(DISTINCT s.uid) FROM {sessions} s WHERE s.timestamp >= :timestamp AND s.uid > 0", array(':timestamp' => $interval))->fetchField();". That's impossible to support by using an abstracted way and is already obviously broken if you use memcache or another session backend.

So, I don't see a way to support that without tracking this information somewhere else (like an expirable keyvalue store), which could/should (?) be done in contrib. => Throw it out is the only option I see.

Oh I see, bad logged in block is bad. This piece of core must be either dropped or rewrite to fit with the new session handling (decoupled from it). You're right logged in users should be tracked using an additional table (SQL or whatever else) but decoupled from session handling in order to be able to reproduce this feature.

This something we must fix or drop in this patch IMO. If we choose to fix it, I'd suggest we disable (ugly commenting) the feature in the first patch, make it work and commit it, then restore it into a follow-up where we'd have plenty of time to think of a nice and efficient solution.

I'm not sure K/V would help if we need to be able to proceed to logged in user listing. Considering that we might end up with thousands of users, doing getAll() calls on a K/V that is not an option, we need paging and sorting abilities for this.

Status:Needs review» Needs work

The last submitted patch, session-335411-269.patch, failed testing.

We have done that sort of "disable for now, open a critical to fix it afterward" process before, but it requires approval from Dries/catch to do so.

Is tracking logged in users something that's generally needed, or just to support that one block? If it's just for that feature, and it's one that lots of people don't use (I don't think I've ever used it), then might we split that out to its own core module, or make it part of the statistics module, or something like that? That way users that don't need to track who's logged in at any given time don't have to deal with it, or the extra DB space.

It's only one block, the "who's online" block. The feature by itself serves no purpose except displaying this block.

@Crell: Forgot to cross-reference, catch, as maintainer of the memcache module, is very aware of this problem and proposed a change that works for this as well already a while ago: #1488630: Who's online block doesn't work with swappable session backends and lazy session creation.

So we can ignore that problem and focus on the other failing stuff here. I'll try to get a working patch up there :)

Status:Needs work» Needs review
StatusFileSize
new600 bytes
new67.68 KB
FAILED: [[SimpleTest]]: [MySQL] 47,724 pass(es), 48 fail(s), and 2 exception(s).
[ View ]

Ok, the page cache and date time failures are actually very easy to fix, there is a wrong return $user that prevents the relevant lines of code for those two things from being called.

No progress yet on the upgrade test logout problem.

Oh I probably did forgot that ^^ thanks.

Status:Needs review» Needs work

The last submitted patch, session-335411-277.patch, failed testing.

I just spent some time to look through the current patch. Direction-wise, it looks relatively good already.

However, it also looks like there's still a (really big) ton of todos to resolve, comments to write and fix, and other clean-up work to do. As we're nearing #300, and since d.o issue comments will nastily wrap past that number, I'd highly recommend to create a fresh issue soon-ish to get all the remaining tasks done. Of course, ensure to copy over the existing summary as well as any remaining architectural issues/questions/concerns from past comments that aren't addressed yet into the new summary.

Implementation-wise, the only part I'm concerned about is the amount of special-casing being introduced for the installer. I wonder whether my patch in #1798732-54: Convert install_task, install_time and install_current_batch to use the state system might help to reduce that or possibly even eliminate it altogether?

Also, but less important: The database connection should be injected into the session storage.

And, perhaps the cause for some more test failures:

+++ b/core/includes/common.inc
@@ -5011,8 +5011,8 @@ function drupal_cron_run() {
-  $original_session_saving = drupal_save_session();
-  drupal_save_session(FALSE);
+  $session = drupal_container()->get('session');
+  $session->disableSave();
@@ -5078,7 +5078,7 @@ function drupal_cron_run() {
-  drupal_save_session($original_session_saving);
+  $session->enableSave();

Watch out: The original code takes extra care to not (re-)enable session saving if it was disabled before.

@sun: Thanks for the review. I think most of this code was written before the container existed and certainly before the database service was defined in the bootstrap container. Will update that. Also plan to work on the comments once we are green.

Nice catch on the re-enable part. Would you say that it makes sense to wrap this within the API automatically? So that a enableSave() does not enable but goes back to the previous value, with a stack? Similar to the issue that adds a function to do a user switch.

StatusFileSize
new2.07 KB

Ok, I made some progress.

disableSave() doesn't work because PHP calls directly into the storage handler and circumvents the Session class completely. The attached interdiff takes care of that by calling $this->storage->getSaveHandler()->setActive(). That method is currently not in the interface, so we will have to extend that.

Somehow my system got all messed up and I'm getting very confusing results right now with and without that change. Will continue later.

Be careful, don't change the active state of storage lightly, this can lead to pretty ugly behavior. I fought a lot with this one: we already have the save() operation in control, it won't be called if the we disableSave() on the Session object, weither or not the storage is active. Storage being active also means that the session_start() method have been called, and no matter if we save it or not, we cannot let the storage think it it's not active anymore else it will attempt new session_start() calls in some cases when we reactivate it, which will lead to bad PHP notices.

@pounard: I know. But the point is that we do *not* have save under control. The automated session save, which is called by PHP at the end of the request circumvents both isSaveEnabled() checks because it calls directly into the session handler object, *not* the functional wrapper in session.inc or the Session class. So we need to know *there* if we can save or not.

From what I remember, the DrupalProxy is supposed to do that: it is a proxy over the real session handler (database implementation) and *not* the storage.

+  public function write($id, $data) {
+
+    if (!$this->active) {
+      return FALSE;
+    }

EDIT: Oups accidental submit. This code belongs to the CookieOverrideProxy class, this is what is supposed to avoid double save: once it has been save the SF2 Session API is supposed to set active to FALSE by itself onto the handler, if not, we have a bug (or the SF code evolved).

If the disableSave() is not correctly checked and sometimes being saved anyway, then it's here that we need to attach: we must inactivate the handler, and not the storage.

@pounard: That active flag is exactly what I'm setting in my patch? class CookieOverrideProxy extends SessionHandlerProxy which extends AbstractProxy, which provides setActive() and isActive().

+++ b/core/includes/common.incundefined
@@ -2379,8 +2379,8 @@ function drupal_exit($destination = NULL) {
     }
-    drupal_session_commit();
   }
+  drupal_session_commit();
   exit;

This looks related to my issues with logout not working after upgrade? Do you remember why you changed this?

I changed this because the install process and some other various menu weird callbacks weren't working without this. In all cases, it's dumb to make this exception, drupal_session_commit() should always been called whatever happens (except in 50X errors). You can try switching this back as it was before, but you have to be extra careful during the install process and check the drupal_session_commit() happens in there. I think the best patch to do is probably a static boolean variable in this function that prevent a double save.

In all cases, this function must disapear and be implement into a kernel listener onto the terminate event, but for this to happen, the drupal_exit() function must die too: only the kernel should be able to terminate a request. Sadly, this is not the case, for two reasons: the first because some callbacks still call drupal_exit(), and the second because the install process doesn't do anything right.

Status:Needs work» Needs review
StatusFileSize
new68.05 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Ok, here is a new patch.

I used your interdiff from #283 and changed:

  • Removed the protected saveEnabled boolean from Drupal Session object
  • Added a static that disallow two successive calls to drupal_session_commit()
  • Improved a bit drupal_session_commit() algorithm for pure performance reasons (no functional changes)

Hope the test bot will be slightly happier!

Status:Needs review» Needs work
Issue tags:-API clean-up, -WSCCI, -symfony

The last submitted patch, session-335411-290.patch, failed testing.

Status:Needs work» Needs review

#290: session-335411-290.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+API clean-up, +WSCCI, +symfony

The last submitted patch, session-335411-290.patch, failed testing.

Status:Needs work» Needs review

I hope this test failure was an odd random one, because installation actually works pretty well here (windows box with a very normal WAMP stack).

And it isn't...

StatusFileSize
new68.01 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Rerolling. Whitespace and offset only. Why is testbot unhappy?
  '@reason' => 'Installing: failed to complete installation by setting admin username/password/etc.', is what's rapported.

I'm just jumping in here. Tested on OSX homebrew setup and my brief test looked OK. What are we looking for?

Status:Needs review» Needs work

The last submitted patch, session-335411-296.patch, failed testing.

The failure reason is not a valuable message, I'm clueless here.

I can confirm that the installation works locally. I can also confirm that logout is now consistently broken with the interdiff from #283. Login works, logout doesn't.

Let's call this the "Hotel California" feature. ;) You can log in whenever you like, but you can never leave... ;)

@webchick ahah! :)

@Berdir that's due to the Session::migrate() calls: the custom proxy is not activated per default by symfony (actually I'm not really sure of how they use this "active state") so when you migrate() the session in order to destroy it, it actually does nothing because of this.

I have to take a few more moments to see what's under the hood with SF sessions about that highy complex proxy handling, I missed a few details there.

Status:Needs work» Needs review
StatusFileSize
new69.06 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch session-335411-302.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok did some better research. Symfony session storage will not set the active state of the session handler, never, except in some weird use case I'm not sure I understood: but something is sure, the proxy needs to set its own state by itself when PHP interface is called. Actually our code does this, and that's good.

We cannot change the active state ourselves, it reflects the internal PHP session state: it would be a dangerous and error prone thing to do. I made one exception thought: when session save is disabled, I disable manually the handler, but I also store the previous state and restore it when save is enabled back.

We cannot rely on the handler to tell us if save is disabled or not, because we do some lazy triggered operations and the real handler state may not reflect if we are going to save our session or not in end, so I restored the Session::saveEnabled boolean property.

Ok, let's try this.

Status:Needs review» Needs work

The last submitted patch, session-335411-302.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new69.07 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

Manually edited patch count++

Status:Needs review» Needs work

The last submitted patch, session-335411-304.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new69.07 KB
FAILED: [[SimpleTest]]: [MySQL] 47,955 pass(es), 48 fail(s), and 1 exception(s).
[ View ]

Catch seems to be experiencing a commit frenzy.

[Manually edited patch count]++

Status:Needs review» Needs work

The last submitted patch, session-335411-306.patch, failed testing.

Yes! Way better than it was :)

Pages