Switch to Symfony2-based session handling

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.

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.

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.)

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.

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.

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.

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...

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.

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?

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:

or maybe

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.

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

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.

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 :)

The last submitted patch failed testing.

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.

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 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.

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:

subscribe

+1

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

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.

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.

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.

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 :)

@#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 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.

Subscribe

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.

@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.

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

@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?

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.

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.

sub.

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

@#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.

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.

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).

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.

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.

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

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.