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 mechanism, 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:
- 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]
- 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] - Put back lazy cookie sending in action, implement a proxy pattern arround Symfony's
Session
object (for exposing the public API) and arroundNativeSessionStorage
(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] - 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. - 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)
Comment | File | Size | Author |
---|---|---|---|
#340 | drupal-session-335411-340.patch | 69.06 KB | mbrett5062 |
#338 | drupal-session-335411-338.patch | 52.8 KB | mbrett5062 |
#336 | session-335411-306-2.patch | 68.39 KB | fubhy |
#333 | session-335411-306_1.patch | 69.07 KB | cosmicdreams |
#332 | 335411_332_notest.patch | 0 bytes | cosmicdreams |
Comments
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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.
Comment #3
chx CreditAttribution: chx commentedwell 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.
Comment #4
Heine CreditAttribution: Heine commentedIt 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.
Comment #5
chx CreditAttribution: chx commentedFurther decision with Heine (and summing up the issue): we will
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.Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe 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.)
Comment #7
chx CreditAttribution: chx commentedyes 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).
Comment #8
sdboyer CreditAttribution: sdboyer commentedI'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.
Comment #9
sdboyer CreditAttribution: sdboyer commentedErrr...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.
Comment #10
sdboyer CreditAttribution: sdboyer commentedIn 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 likesession_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
indrupal_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 calleddrupal_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.
Comment #12
Crell CreditAttribution: Crell commentedHonestly 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.
Comment #13
chx CreditAttribution: chx commentedand 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
Comment #14
sdboyer CreditAttribution: sdboyer commentedCool, 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...
Comment #15
sdboyer CreditAttribution: sdboyer commentedOK, here's the next crack at this. Some quick notes:
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 forglobal $user
. So I stopped trying and it went away :)Yar!
Comment #16
sdboyer CreditAttribution: sdboyer commentedAnother 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.
Comment #17
Crell CreditAttribution: Crell commentedDo 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.
Comment #18
sdboyer CreditAttribution: sdboyer commentedGreat, thanks for the review. Item by item:
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.
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.
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...Works for me - I had getInstance() initially, I'll add it back.
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.
Yeah. See earlier comment :)
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?
Comment #19
Crell CreditAttribution: Crell commentedVariables 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:
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:
Make sense?
Comment #20
sdboyer CreditAttribution: sdboyer commentedSure, 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.Comment #22
Damien Tournoud CreditAttribution: Damien Tournoud commentedIt'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.
Comment #24
sdboyer CreditAttribution: sdboyer commentedNow 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 :)
Comment #25
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat should solve the installation issue (during install, drupal_init_language() is apparently called before session initialization).
Comment #27
chx CreditAttribution: chx commentedI would like to see another round with the testingbot to make sure the patch broke the install.
Comment #29
Crell CreditAttribution: Crell commentedI 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().
Comment #30
Damien Tournoud CreditAttribution: Damien Tournoud commentedI don't see why. Accessing $_SESSION as an array works just like before. And because that
We still have a explicit factory, that is called DrupalSession::init(). Easy to swap out implementations from there.
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.
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.
That's actually a great idea.
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.
Comment #31
sdboyer CreditAttribution: sdboyer commentedYeah, 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.
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.
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.
I agree. Implementing that level of defensiveness is over the top.
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.
+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!
Comment #32
Crell CreditAttribution: Crell commentedI 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.
Comment #33
sdboyer CreditAttribution: sdboyer commentedFair 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.
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 behindsession_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.
Comment #34
slantview CreditAttribution: slantview commentedwhy 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:
Comment #35
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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?
Comment #40
dixon_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
Comment #41
catchMissed 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.
Comment #42
dixon_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.Comment #43
catchThere'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.
Comment #44
Crell CreditAttribution: Crell commentedCorrect, 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.
Comment #46
sdboyer CreditAttribution: sdboyer commentedSince 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 theSessionStorage
, 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.Comment #47
sdboyer CreditAttribution: sdboyer commentedAnd 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.
Comment #48
catchWe 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.
Comment #49
sdboyer CreditAttribution: sdboyer commentedOh yeah, I could totally dig using their storage but our own Session class/interface.
Comment #50
sdboyer CreditAttribution: sdboyer commentedChatted 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
: +1NativeSessionStorage
: +1ArraySessionStorage
: mehFilesystemSessionStorage
: -1, this seems just like Native but done in userspace with a configurable path...unless I missed something?PdoSessionStorage
: -1, replace with our ownDbtngSessionStorage
(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 :)Comment #51
pounardJust 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.
Comment #52
pounard@#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.
Comment #53
Crell CreditAttribution: Crell commentedI 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.
Comment #54
catchI 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.
Comment #55
DamienMcKennaTagging this to make it easier to identify all of the Symfony-related issues.
Comment #57
Shellingfox CreditAttribution: Shellingfox commentedSubscribe
Comment #58
pounardI 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).
Comment #60
sdboyer CreditAttribution: sdboyer commented@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.
Comment #61
pounardI'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.
Comment #62
catch@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.
Comment #63
pounard@#62 File storage can be used in tmpfs or ramfs, some web applications are configured this way, it makes sense.
Comment #64
catchI've opened #1277682: Move responsibility for global $user out of session as a possible interim step for this.
Comment #65
lsmith77 CreditAttribution: lsmith77 commentedFYI the "FilesystemSessionStorage" is mostly used for functional testing.
Comment #66
sdboyer CreditAttribution: sdboyer commented@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?
Comment #67
pounardI 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!).
Comment #68
Anonymous (not verified) CreditAttribution: Anonymous commentedre #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.
Comment #69
pounard#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.
Comment #70
sdboyer CreditAttribution: sdboyer commentedLet'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.
Comment #72.0
deviantintegral CreditAttribution: deviantintegral commentedAdding an issue summary.
Comment #73
Grayside CreditAttribution: Grayside commentedComment #74
deviantintegral CreditAttribution: deviantintegral commentedIMO, 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
Comment #75
Crell CreditAttribution: Crell commentedThe patch is in, this is back in business.
Comment #76
pounard@#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.
Comment #77
catchThere 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.
Comment #78
Crell CreditAttribution: Crell commentedcatch: 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.
Comment #79
catch@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?
Comment #80
pounardI'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).
Comment #81
chx CreditAttribution: chx commentedsomeone 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.
Comment #81.0
chx CreditAttribution: chx commentedReuse of $_SESSION
Comment #81.1
chx CreditAttribution: chx commentedremove chx
Comment #82
cboden CreditAttribution: cboden commentedSymfony 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.
Comment #83
lsmith77 CreditAttribution: lsmith77 commentedjust 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...
Comment #84
pounardSymfony 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.
Comment #85
grendzy CreditAttribution: grendzy commentedHas anyone had a chance to evaluate Symfony's support for HTTPS sessions and mixed HTTP/S sessions?
Comment #86
catchI haven't but was going to ask the same thing. I didn't see any sign of it last time I looked though.
Comment #87
webmozart CreditAttribution: webmozart commentedgrendzy: Are you talking about something similar to this?
Comment #88
grendzy CreditAttribution: grendzy commentedbschussek: 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.
Comment #89
Damien Tournoud CreditAttribution: Damien Tournoud commentedThere 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.
Comment #90
Crell CreditAttribution: Crell commentedNote 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.
Comment #92
pounardI 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).
Comment #93
pounardAssigning to me as it is work in progress now. I will need eyes as soon as the first non invasive patch lands.
Comment #94
pounardHere 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.
Comment #95
pounardDamien's notes are also revelant to this issue.
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.
Comment #96
catchI 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.
Comment #97
pounardOh 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.
Comment #98
webchickDrupal.org actually runs a patch that puts users.access in its own table. Maybe core could do the same?
Comment #99
pounard@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!
Comment #100
pounardOne 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 checkdrupal_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.
Comment #101
Crell CreditAttribution: Crell commentedJust 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. :-)
Comment #102
pounardHere 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.
SessionTokenProvider
. This component is injected into theSession
object which will trigger the token send or destroy on various method calls such asSession::invalidate()
,Session::save()
etc...Symfony\Component\HttpFoundation\Session
class asDrupal\Core\Session
in order to achieve the upper statement. This should still remain easy to port to Symfony 2.1 I guess.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 usingSession::(get|set|remove)()
session.inc
file which explains everythingI 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.
Comment #103
pounard@Crell oups cross-post, nice, then I should go for it :)
Comment #104
webchickAlso, 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)
Comment #105
Crell CreditAttribution: Crell commentedOur 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.
Comment #106
neclimdulAlso, symfony sessions now supports our message system! The flash improvements where pulled during the Drupalcon code sprint. Go open source!
Comment #107
pounardNice, 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.
Comment #108
Crell CreditAttribution: Crell commentedThis 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.
Comment #109
pounardIt 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.
Comment #110
pounardHere 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.
Comment #112
pounardOupsie, 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:)
Comment #113
pounardTags change.
Comment #114
cosmicdreams CreditAttribution: cosmicdreams commentedThing I found it. You redeclared system_update_8006
Comment #115
cosmicdreams CreditAttribution: cosmicdreams commentedComment #117
cosmicdreams CreditAttribution: cosmicdreams commentedpounard: can you please provide an updated summary of what this patch does. I get a bit lost in the code.
Comment #118
cosmicdreams CreditAttribution: cosmicdreams commentedThe 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).
Comment #119
cosmicdreams CreditAttribution: cosmicdreams commentedAs per #118 this needs work.
Comment #120
tnightingale CreditAttribution: tnightingale commentedAs 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:
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.
Comment #121
tnightingale CreditAttribution: tnightingale commentedAnd 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... :-\
Comment #122
pounardWow, 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).
Comment #123
Crell CreditAttribution: Crell commentedSymfony 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.)
Comment #124
cosmicdreams CreditAttribution: cosmicdreams commentedHere's an updated patch that gets to the installing modules part. Errors on language stuff.
Comment #125
cosmicdreams CreditAttribution: cosmicdreams commentedIt 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.
Comment #126
cosmicdreams CreditAttribution: cosmicdreams commentedSomething 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?
Comment #127
cosmicdreams CreditAttribution: cosmicdreams commentedOk.... 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:
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:
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.
Comment #128
cboden CreditAttribution: cboden commentedcosmicdreams: 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.
Comment #129
Crell CreditAttribution: Crell commentedI 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.
Comment #130
cosmicdreams CreditAttribution: cosmicdreams commentedOK, 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.
Comment #131
Crell CreditAttribution: Crell commentedTagging
Comment #132
BerdirNice stuff in here, review below :)
Looks like the check is the wrong way round here.
Looks like you don't use this anywhere in this function. the next chunk is already another one.
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..
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.
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).
Not sure but I think a description line for @return is still required?
That FIXME can probably be removed, doesn't look custom to me?
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.
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.
I've never seen us use the term "business layer", not sure if something else should be used.
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 :)
DBtng => the database or something like that. We don't use DBtng in official documentation, that's the secret code name :)
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).
Interface implementations should have a "Implements Interface::method()" docblock.
I think all chained method calls should be put on separate lines.
db_delete()
->condition()
->execute();
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).
Should also have a description I think, not just the @var.
See http://drupal.org/node/1354#classes for coding standards on class docblocks.
Should be "Can not".
Maybe "Returns TRUE if the session is empty".
Also, I think we use @todo and not FIXME.
Is this short for Lazy Larry? :) -> should be Lazy.
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.
Comment #133
cosmicdreams CreditAttribution: cosmicdreams commentedSorry Berdir, please disregard the patch in #127 and review the patch in #124 instead
Comment #134
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
***
FYI, "cannot" is actually the preferred usage.
Comment #135
Berdir@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?
Comment #136
Paul Simard CreditAttribution: Paul Simard commentedI'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?
Comment #137
webchickThose actually aren't directory separators. Those are namespace separators. The syntax comes from PHP. http://us.php.net/namespaces
Comment #138
pounard@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!
Comment #139
webchickI 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
Comment #140
pounardExactly, but after saying it 10 times, that sounded clearer that way :)
Comment #141
Paul Simard CreditAttribution: Paul Simard commented@webchick: Do'ah! My Homer Simpson moment for April.
Thanks for the link, too. :)
Comment #142
cosmicdreams CreditAttribution: cosmicdreams commentedThanks 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.
Comment #143
Paul Simard CreditAttribution: Paul Simard commented@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
Comment #144
neclimdulI 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)
Comment #145
cosmicdreams CreditAttribution: cosmicdreams commented#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
Comment #146
pounardGood move
Comment #147
David_Rothstein CreditAttribution: David_Rothstein commentedWish 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 :)
Comment #148
RobLoachWe'd obviously be using PSR-0 :-) .
Comment #149
pounardIndeed. This not really a kernel-followup thought, but tagging with it will trigger more reviews! Please, people, need DESIGN reviews!
Comment #150
pounardFirst directed questions:
Comment #151
BerdirCan 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).
Comment #152
sunNo, 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.
Comment #153
pounardHum, 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:
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.
Comment #154
pounardThat 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.
Comment #155
neclimdul+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.
Comment #156
pounardOk, 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.
Comment #157
sunUnless I'm mistaken, then
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).
Comment #158
Crell CreditAttribution: Crell commentedThe 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.
Comment #159
pounardI 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?
Comment #160
pounard@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.
Comment #161
sunYes, 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.
Comment #162
pounardOk, thanks, I will try to take some time and ask those people for details.
Comment #163
pounardOk, here is a proof of concept patch.
Some additional notes, that adds up to the notes from #153:
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:
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!
Comment #164
pounardHum 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.
Comment #165
pounardGot 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.
Comment #166
pounardNew patch attached, almost the same as the one above plus:
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.
Comment #168
Crell CreditAttribution: Crell commentedIt 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.)
Comment #169
pounardOh nice, that would solve a huge part of the problem for us and make it really easy to integrate.
Comment #169.0
pounardMoved to Crell
Comment #170
podarok#166: 335411-166-sf_session.patch queued for re-testing.
Comment #172
pounardThe 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.
Comment #172.0
pounardAdd deep link to first useful comment.
Comment #173
Gaelan CreditAttribution: Gaelan commentedReroll. Still doesn't work because of something seemingly unrelated(can't find entity_load_multiple in user.module).
Comment #175
pounardPosted #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).
Comment #176
pounardOk 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.
Comment #177
cosmicdreams CreditAttribution: cosmicdreams commented#173: sf_sesion-335411-173.patch queued for re-testing.
Comment #179
pounardI need to get back to this patch, especially during DC so people can take some time to discuss @Munich.
Comment #180
cosmicdreams CreditAttribution: cosmicdreams commentedpounard: 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?
Comment #181
pounard@#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.
Comment #182
cosmicdreams CreditAttribution: cosmicdreams commentedRegarding 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?
Comment #183
pounardNo 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.
Comment #184
Crell CreditAttribution: Crell commentedRequiring 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. ;-)
Comment #185
chx CreditAttribution: chx commentedI 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.
Comment #186
Crell CreditAttribution: Crell commentedchx: 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.
Comment #187
chx CreditAttribution: chx commentedThis is a new concern I have brought up.
Comment #188
Crell CreditAttribution: Crell commentedIt'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.
Comment #189
chx CreditAttribution: chx commentedOK then, postponed on #1451056: [policy] How to handle unforeseen diversion of Symfony code in stable/API-locked Drupal core? and especially http://groups.drupal.org/node/250578 .
Comment #190
q0rban CreditAttribution: q0rban commentedI 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.
Comment #191
pounardI'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.
Comment #191.0
pounardNew summary
Comment #192
chx CreditAttribution: chx commentedI 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.
Comment #193
Crell CreditAttribution: Crell commentedNo. 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.
Comment #194
q0rban CreditAttribution: q0rban commentedCrell,
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.
Comment #195
cosmicdreams CreditAttribution: cosmicdreams commentedTried to reroll this today. Probably broken.
Comment #197
Crell CreditAttribution: Crell commentedq0rban: 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.
Comment #198
pounard@cosmicdreams Thanks, I will try a review. Did you encounter any problems?
Comment #199
pounard@cosmicdreams New core files (core/Session/*) are missing in your patch.
Comment #200
q0rban CreditAttribution: q0rban commentedThank you for your explanation, Crell.
Comment #201
cosmicdreams CreditAttribution: cosmicdreams commented@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.
Comment #202
RobLoachThis 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?
Comment #203
podarokstatus
Comment #205
pounard@#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.
Comment #206
chx CreditAttribution: chx commentedReviewed this. Let's do it. I hope dearly I won't need to eat my words later, but let's do it.
Comment #207
cosmicdreams CreditAttribution: cosmicdreams commentedAs 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...
Comment #208
cosmicdreams CreditAttribution: cosmicdreams commentedok, 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.
Comment #209
pounard@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.
Comment #210
cosmicdreams CreditAttribution: cosmicdreams commentedSure, 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.
Comment #210.0
cosmicdreams CreditAttribution: cosmicdreams commentedAdded security note
Comment #211
cosmicdreams CreditAttribution: cosmicdreams commentedYo folks, looking into this again this evening. Looks like session is never comitted because the isSaveEnabled is returning function is returning FALSE.
Comment #212
cosmicdreams CreditAttribution: cosmicdreams commentedOther questions / comments. I'm asking these because I'm trying to find out more about this system. Sorry if I'm nit-picking.
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...()
Should be revised to "Cookie handling is decoupled from core session handling and storage" and reference function that does cookie handling
Should we remove checks test whether a session is started then?
Outdated, we're using Symfony 2.1-dev now.
direct access to what? be clear in this comment.
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.
2.1 is our base, this sounds like a necessary @todo in order to get this patch working.
Is this really a job for the plugin system, or would getting this class from the DIC be better?
as a result of registering session services in the bootstrap, do we still need to do this?
Is this redoing the work bootstrap is already doing (when it registers session stuff)?
why is the $is_https global removed here but not in other functions?
After install, this test always passes. Then the function immediately returns.
That leads us to question. Is the session ever saved enabled?
Comment #213
cosmicdreams CreditAttribution: cosmicdreams commentedalso _drupal_session_load_user() states that it should
Replace this with a container lazy initialization function instead.
How is that done?
Comment #214
cosmicdreams CreditAttribution: cosmicdreams commentedAlso, 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.
Comment #215
cosmicdreams CreditAttribution: cosmicdreams commentedin 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.
Comment #216
cosmicdreams CreditAttribution: cosmicdreams commentedLooked 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.
Comment #217
pounardYou're working in HEAD of which sandbox? Can I access it? Can I see the code in the actual state?
Some answers to #202:
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.
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.
Nope, the existing tests in code are important because the session is still lazy started, but at component init.
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.
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.
Indeed, there is nothing that technically prevent a developer to use it.
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.
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.
This is exactly the kind of code that must be replaced by DIC registration/compilation in core Bundle.
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.
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.
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.
Comment #218
pounardIt'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:
Then try to login and login, everything should be working fine.
Comment #220
pounardOups, 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.
Comment #221
Crell CreditAttribution: Crell commentedBad bot!
Comment #222
catchDon't see how this can possibly be described as a feature.
Comment #223
pounardHere is a version where the installer actually works.
Comment #224
fubhy CreditAttribution: fubhy commentedThen let's test it :)
Comment #226
pounardIt was on purpose I didn't tested it, tests themselves needs fixing! EDIT: But it's interesting to see the result, thanks.
Comment #227
pounardOk, random notes about current status, patch to be posted here soon:
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.
Comment #228
pounardThis should start to be encouraging test wise.
Comment #230
pounardTest log actually shows a lot of things actually passes, but some fatals make it go crazy.
Comment #231
pounardComment #232
cosmicdreams CreditAttribution: cosmicdreams commentedI 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.
Comment #233
cosmicdreams CreditAttribution: cosmicdreams commentedI 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.
Comment #234
pounardThanks, good to ear! From now on I will post interdiff to all my patches, it starts to stabilize (I hope).
Comment #235
aspilicious CreditAttribution: aspilicious commentedAccording 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
We have a DIC?
Is there an issue for this?
Don't write stuff like "\SessionHandlerInterface". "use" it on top of the file
I think it is @implements without the capital and I think we need the full path \Drupal\Core\Session\Handler\SessionHandlerInterface
Constructs a CookieOverrideProxy object.
GetS
80 chars max for a summary
$container isn't defined in this class. This can't work... or can it?
If you're posting a node id use the node id pointing to the correct issue that is fixing this for D7.
Comment #236
aspilicious CreditAttribution: aspilicious commentedReviewing the logs to make it easier to remove the fatals:
and
fatals, some page caching and poll vote tests are failing but that will be visible once the fatals are gone
Comment #237
pounard@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:
Comment #238
cosmicdreams CreditAttribution: cosmicdreams commentedI 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.
Comment #241
Fabianx CreditAttribution: Fabianx commented@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 ...
Comment #242
xjmOkay 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.
Comment #243
cosmicdreams CreditAttribution: cosmicdreams commentedOriginal post by @cosmicdreams:
Reposted by xjm
Comment #244
cosmicdreams CreditAttribution: cosmicdreams commentedrevisiting this for about an hour. Going to try to address #236 and others.
Comment #245
cosmicdreams CreditAttribution: cosmicdreams commentedIn tests, I get proper failures now. What I see in those failures is confusing. I get an this error:
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.
Comment #246
cosmicdreams CreditAttribution: cosmicdreams commentedHere'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.
Comment #247
BerdirI'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.
Comment #249
BerdirOk, a handful tests threw up but most of them seem to have passed.
That doesn't look too bad :)
Comment #250
cosmicdreams CreditAttribution: cosmicdreams commentedThe test results from 246 are likely dirty because the patch needs to be rerolled.
Comment #251
das-peter CreditAttribution: das-peter commentedRe-rolled against 8.x and, together with Berdir, tried to fix the lost session.
Comment #252
pounard#251 I'd be interresting to have an interdiff please.
Comment #253
das-peter CreditAttribution: das-peter commentedDamn. I forgot to remove a debugging line. Here's the cleaned patch.
Comment #255
cosmicdreams CreditAttribution: cosmicdreams commentedI tried applying this patch and it applied fine. So I'll reupload the diff
Comment #257
cosmicdreams CreditAttribution: cosmicdreams commentedAgain, but with an updated system.install function
Comment #258
BerdirWorking 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 :)
Comment #260
pounard48 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.
Comment #261
cosmicdreams CreditAttribution: cosmicdreams commentedWe 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.
Comment #262
pounardOk, sounds like a plan, we need to get this green.
Comment #263
BerdirYes!
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.
Comment #264
pounard@Berdir, oh that was just that then, very nice!
Comment #266
cosmicdreams CreditAttribution: cosmicdreams commentedAh 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.
Comment #267
BerdirThe DateTime component was commited *today*. I assume there there actually is a call to that function left.
Comment #268
BerdirAlso, 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...)
Comment #269
BerdirHere'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 :)
Comment #270
pounardNice work with the update tests!
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.
Comment #271
Berdir@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.
Comment #272
pounardOh 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.
Comment #274
Crell CreditAttribution: Crell commentedWe 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.
Comment #275
pounardIt's only one block, the "who's online" block. The feature by itself serves no purpose except displaying this block.
Comment #276
Berdir@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 :)
Comment #277
BerdirOk, 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.
Comment #278
pounardOh I probably did forgot that ^^ thanks.
Comment #280
cosmicdreams CreditAttribution: cosmicdreams commentedalso #961508: Dual http/https session cookies interfere with drupal_valid_token() seems related.
Comment #281
sunI 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:
Watch out: The original code takes extra care to not (re-)enable session saving if it was disabled before.
Comment #282
Berdir@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.
Comment #283
BerdirOk, 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.
Comment #284
pounardBe 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.
Comment #285
Berdir@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.
Comment #286
pounardFrom 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.
Comment #287
pounardEDIT: 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.
Comment #288
Berdir@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().
This looks related to my issues with logout not working after upgrade? Do you remember why you changed this?
Comment #289
pounardI 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.
Comment #290
pounardOk, here is a new patch.
I used your interdiff from #283 and changed:
Hope the test bot will be slightly happier!
Comment #292
pounard#290: session-335411-290.patch queued for re-testing.
Comment #294
pounardI hope this test failure was an odd random one, because installation actually works pretty well here (windows box with a very normal WAMP stack).
Comment #295
pounardAnd it isn't...
Comment #296
steinmb CreditAttribution: steinmb commentedRerolling. 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?
Comment #298
pounardThe failure reason is not a valuable message, I'm clueless here.
Comment #299
BerdirI 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.
Comment #300
webchickLet's call this the "Hotel California" feature. ;) You can log in whenever you like, but you can never leave... ;)
Comment #301
pounard@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.
Comment #302
pounardOk 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.
Comment #304
pounardManually edited patch count++
Comment #306
pounardCatch seems to be experiencing a commit frenzy.
[Manually edited patch count]++
Comment #308
pounardYes! Way better than it was :)
Comment #309
steinmb CreditAttribution: steinmb commentedWas unable to apply #306
Comment #310
steinmb CreditAttribution: steinmb commentedStrike #309, fail on my side. Confirm that #306 remove the Hotel California functionality :)
Comment #311
pounardLol OK, I had it confirmed on my own box, but thanks for testing! Now we have only the session, secure session, and update tests failing:
PS: Now is time for a pass of whitespace review if anyone wants to, I'll fix everything I can ASAP even regarding this. It's also now the time to do a last architectural review, and documentation review, and possible documentation improvements.
Comment #312
cosmicdreams CreditAttribution: cosmicdreams commentedGetting the easy/obvious ones out of the way.
I came here to report how a previous patch broke core and see that pounard has pushed things forward since the last patch I tested.
Perhaps out of scope, but can this be a EFQ instead?
extra whitespace
need newline here
extra space here
Comment #313
pounardThat's an excellent question.
Ideally, it would, but this would not be good for performances. The current code already adds an extra SQL query, loading the full user account (which would definitely be better) is: 1) not what Drupal already does, 2) currently happening very early and would necessitate a very early init of the Entity and Cache APIs.
Ideally the global $user would dissapear and a logged in user context object would be live into the DIC instead, and this particular object would load the current user when asked on demand. This could also help decorellate the user access parts from the user structure: we would not load all user roles with the entity, but with the security context instead.
Thanks for the review.
Comment #314
pounardThis is a excellent topic for a follow up: refactoring the security and global user.
Comment #315
catchNow we're over 300 comments and Drupal.org is still on D6, would anyone object to opening a session part II issue?
Comment #316
pounard@catch We're almost there for the initial patch, we have a pretty good history here.
EDIT: Ouch, I'm on page 2!
Comment #317
Fabianx CreditAttribution: Fabianx commented#314:
There are issues for that already:
#1549526: Change global $user into $session
... and also related #1545680: Convert all uses of $_SESSION to symfony session syntax.
Comment #318
Crell CreditAttribution: Crell commentedI vote for a new issue. This issue has drifted so many times already that the first 2/3 of the comments are off topic anyway. :-) Maybe as soon as we get something green (which seems close) we can skip over to a new issue with a green patch to start?
Comment #319
sunI strongly recommended it earlier already. We're past 300 now, and not nearly done. So here we go:
#1841198: Clean-up the session patch
Please make sure to copy over the relevant + required parts.
I additionally feel we need a unbiased, independent party that goes through existing comments to double-check whether all concerns that have been raised before were properly addressed already or not. Ideally, someone who's more familiar with the current session handling. I'd offer myself, but I doubt I'll have time to do that in the next week or two. :-/ @chx? @Damien Tournoud?
Comment #320
pounardPlease @sun at least respect what the others think. Let's get it green first then open the new issue for follow-ups.
Comment #321
BerdirSpent some more time debugging the failures in the upgrade tests. Here's what I noticed:
- The session id is correctly regenerated and the session id emptied.
- However, because the session has been emptied, the call to drupal_session_commit() results in a session clear, which in turn calls setActive(FALSE).
- This results in the actual session write to be aborted, so the session id change is not propagated to the client and the next request is done with the old session id.
The attached interdiff disables that behavior and the upgrade tests worked for me. But I'm quite sure that this will cause other issues, like breaking session lazy-loading.
Comment #322
pounardHum, we could catch the session migrate calls and send cookies in that particular case, without breaking the lazy cookie sending anywhere else. A logged in user will always have the session loaded/sent anyway because it has a uid set in.
Comment #323
pounardSwitching to NR I'm curious to see what's the bot gonna say
Comment #325
cosmicdreams CreditAttribution: cosmicdreams commentedI've rerolled the patch in 306 and combined it with the patch in 321 since it seems like an interdiff (it's the size of one). I manually tried to do a fresh installation with the result and received the following error
Comment #326
cosmicdreams CreditAttribution: cosmicdreams commentedthis one actually increments the update function in system.install like I said.
Comment #327
pounardDid you intentionnally ignore the testbot?
Comment #328
andyceo CreditAttribution: andyceo commentedComment #330
mbrett5062 CreditAttribution: mbrett5062 commentedThis fails here:
Trying to add some PHP before opening PHP tag and also before namespace declaration. All looks a bit confused.
Checked in #302, the code should be as follows.
And obviously those FIXME's should be fixed.
Also note, line numbers in the file are now wrong, this will need to be re-rolled on latest 8.x.
Comment #331
cosmicdreams CreditAttribution: cosmicdreams commentedYea, it was my reroll that was messed up. I'll give it another go tonight.
Comment #332
cosmicdreams CreditAttribution: cosmicdreams commentedOK, this is a straight-up reroll of 306. It was a dirty reroll, meaning that I had to cut out some code that didn't merge right. I wanted to post it here so that others can start working with something that applied right. Clearly some work needs to be done in order to work with the DIC (or maybe that's just a comment cleanup) and the movement of the container code.
Comment #333
cosmicdreams CreditAttribution: cosmicdreams commentedsorry, I meant this one
Comment #334
eigentor CreditAttribution: eigentor commentedLet the testbot loose.
Comment #336
fubhy CreditAttribution: fubhy commentedRe-roll of 306 as described by #332
Comment #338
mbrett5062 CreditAttribution: mbrett5062 commentedJust re-rolled #336 against latest 8.x.
Comment #340
mbrett5062 CreditAttribution: mbrett5062 commentedForgot the new directory with new files in my last patch. Here it is, hopefully will go a little better.
Comment #342
mbrett5062 CreditAttribution: mbrett5062 commentedForgot to mention, I have not assigned to myself, because I do not believe I have what it takes to get this completed, so left for @pounard, unless someone else wants to take it over. Just trying to get it up to latest D8 and save some time.
Comment #343
mbrett5062 CreditAttribution: mbrett5062 commentedTrying manual testing now, to see if I can help a little. First thing: Installed OK but at end of process get the following error.
Comment #344
mbrett5062 CreditAttribution: mbrett5062 commentedActivated 'testing' and 'syslog'.
Log shows lots of these:
Comment #345
mbrett5062 CreditAttribution: mbrett5062 commentedAll test failures at the moment are due to header information already sent when attempting to send cookies. This needs to be resolved first, then see what other failures if any.
Comment #346
pounardAgree, some of the latest 8.x commits must have disturbed the session handling, there is a lot of spaguetthi code in bootstrap and install.
Comment #347
cosmicdreams CreditAttribution: cosmicdreams commentedHI pounard! I created #1858196: [meta] Leverage Symfony Session components to help make this a simpler task. That issue is just trying to get the Session Object onto the DIC. The patch marcingy provided passes, but it's crashing for me in php 5.3.
Comment #348
YesCT CreditAttribution: YesCT commentedLooks like this is still waiting for a green patch before moving to a new issue?
Anyone up for a reroll? (in case of a brave new contributor, take a stab. reroll doc: http://drupal.org/patch/reroll)
Comment #349
neclimdulClosing this and moving discussion to new broken up issue. I think discussion here has mostly died and is unmanageable and the work has been carried over. Will update the summary to reflect this since the link to the new issue is on the second page and easily missed.
Comment #349.0
neclimdulUpdated issue summary.
Comment #349.1
neclimdulNote issue move.
Comment #350
neetu morwani CreditAttribution: neetu morwani as a volunteer and at Acquia commented