This patch intend to register our Session logic as a service so that session information can be obtained from the request. When working on this patch take special care. The session are initialized very early in the lifetime of Drupal's response to HTTP requests and is a dependency for many other parts of the system. This can lead to problems if you use the dependency injection container for information as the session is initialized before the DIC is.
Note: I'm writing this to ensure that my understanding of Drupal's session handling is correct. Please critique / review.
Problem/Motivation
History of Drupal Session Handling
In web applications, sessions are used to persist information across multiple requests. In Drupal, this information is typically about the user. Drupal has historically handled sessions differently than other applications and different from how PHP natively handles sessions.
Session support in PHP allows one to preserve data across subsequent accesses. A visitor accessing your website is assigned a unique ID, the so-called session ID. The session ID is stored in a cookie on the user side and sent to your website on every page request.
Drupal stores the session ID alongside user IDs in the database. On every page access Drupal receives the session ID from the visitor's browser. It then checks the session table to find the associated user ID. The user ID determines which permissions the visitor has on the site.
-- from https://drupal.org/node/101499
Lazy session handling
In #744384: Do not write unchanged sessions to the database we introduced the ability to lazily store session information. Before this work, the session handler always attempted to save session information, whether there was session information available to store or not. When no session information was available to save, we stored an empty session. This lead to performance problem as we were filling the session table with empty records.
Lazy session handling fixed the problem by only storing sessions when there was information to save. This optimizes performance for anonymous users. We also don't send a cookie when the session has no data so that we can cache pages with advanced caching systems such as Varnish and Memcache.
This a significant performance win that we don't want to regress.
Swappable Session Handling logic
Since Drupal 5, Drupal has provided a way to plugin alternative logic for session handling. We need to preserve that capability.
HTTP vs HTTPS Session Handling
Drupal manages separate sessions for HTTP and HTTPS connections.
Session Handling 101
A PHP session has six basic operations it must define: open, read, write, close, destroy, and gc (garbage collection). In PHP 5.4 the method in which we register these functions has slightly changed to: session_set_save_handler ( SessionHandlerInterface $sessionhandler [, bool $register_shutdown = true ] )
Proposed resolution
We should extend the Symfony2 Session object to add our Drupal specific logic and registers it onto the DI Container. Accomplishing that means that we can access our session implementation from the Request object and spread that information through the application.
Session
We define our own session object that extends Symfony2's session object. We need to implement the logic that ensures we aren't saving the session during every request.
Cookie
We need to ensure we're not sending cookies when the session is empty.
CookieProxy
In order to use our custom logic for handling cookies we need to provide our own CookieProxy otherwise Symfony will autoload it's own SaveHandler.
reference: http://symfony.com/doc/master/components/http_foundation/session_configu...
Remaining tasks
#2228393: Decouple session from cookie based user authentication
#2238561: Use the default PHP session ID instead of generating a custom one
User interface changes
None
API changes
The main change is that we will use the Symfony Stack's methodology for manipulating / replacing the session implementation that Drupal is using.
Other related issues
- #2286591: [meta] Security audit the Authentication component is postponed on this. Re-open it when this is fixed.
Original report by @pounard
Comment | File | Size | Author |
---|---|---|---|
#193 | drupal_1858196_193.patch | 69.78 KB | Xano |
Comments
Comment #0.0
cosmicdreams CreditAttribution: cosmicdreams commentedUpdated issue summary.
Comment #1
cosmicdreams CreditAttribution: cosmicdreams commentedHere's the previous work, this should go green as this doesn't impact current session handling.
Note that this includes a StaticSessionStorage for historical purposes. The StaticSessionStorage doesn't appear to be used so I'm considering removing it.
Comment #2.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #3
Crell CreditAttribution: Crell commentedSessionHandlerInterface was only added in PHP 5.4:
http://us3.php.net/sessionhandlerinterface
:-(
Comment #4
marcingy CreditAttribution: marcingy commentedSeems like symfony has a work around for this https://github.com/fabpot/Silex/issues/411 - as Symfony\Component\HttpFoundation\Resources\stubs implements a backwards comptable layer not sure 100% how we handle invoking it though.
Comment #5
marcingy CreditAttribution: marcingy commentedThis should hopefully work and was based off https://github.com/symfony/symfony/pull/3384#L3R37. A test is included that shows the failure on php 5.3. I tried to add more to the test but ran into clashes with the existing session code.
Comment #6
cosmicdreams CreditAttribution: cosmicdreams commentedI think we need to make this logic more robust. If you do something stupid, like applying this patch over a working D8 site, it will break because it thinks you're trying to look for the SessionHandlerInstance of Assetic.
Perhaps we should a check on to check if we're looking at the Session object on the DI before we look to see if it has a SessionHandlerInterface?
Comment #7
cosmicdreams CreditAttribution: cosmicdreams commentedOH Ok, I get it. Here's the code form their pull request
which since we have the path to the HttpFoundation library at that point in the bootstrap.inc we can change to:
Comment #8
marcingy CreditAttribution: marcingy commentedIs the same as $namespaces['HttpFoundation'] . "/Resources/stubs' if you look in the compsor file, ie we actually have the namespace qualified already as
Comment #9
Crell CreditAttribution: Crell commented#5: session-ph53.patch queued for re-testing.
Comment #11
neclimdulI'm not a master of what's happening at that point in the classloader but what you're saying about I being in the namespaces seems to be false. I went to some trouble to get a 5.3 install going and I can't get to install.php. It's wonk-tastic to debug because apparently its we've already set the error handler for the request which requires config() which we haven't finished setting up...
Anyways, the useful stuff, this is the contents of the namespaces array provided by a var_dump inside the new if:
Your assumption would have been correct but I don't think we're actually including the classmap. We seem to load only the autload_namespaces.php file. Hope that helps.
Comment #12
neclimdulso this may have been a bit confusing. Ignore the bit about error handlers, catch pointed me to the issue.
The problem seems to be we don't actually include the classmap so we're not getting the mapping this patch was relying on. I could find no reference to including the classmap anywhere in our code. To confirm this, I did
var_dump($namespaces);
in the if interface exists check added by this patch which you see in the comment above. You will notice the needed 'SessionHandlerInterface' does not exist in the outputted array.Comment #13
dawehnerWhat about just using that?
Comment #14
marcingy CreditAttribution: marcingy commentedAccording to symfony docs see http://drupal.org/node/1858196#comment-6822246 it should use registerPrefixFallback rather than registerPrefix the issue with the patch in that comment is that for some reason $namespaces['SessionHandlerInterface'] is longer populated (but was in the past), so basically http://drupal.org/node/1858196#comment-6831148 seems to be the valid solution.
Comment #16
dawehnerAh I see.
Comment #18
neclimdulAs stated in my comment, that is not one the values stored in the $namespaces array. This gets as far as finishing the installer in my 5.3 install.
Comment #19
neclimdulrun tests
Comment #20
marcingy CreditAttribution: marcingy commentedOk now this makes sense #1834594: Update dependencies (Symfony and Twig) follow up fixes for Composer removed the namespace. Assuming this comes back green it looks RTBC to me.
Comment #22
aspilicious CreditAttribution: aspilicious commentedFatal error: Interface 'SessionHandlerInterface' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Session/Handler/DatabaseSessionHandler.php on line 14
FATAL Drupal\system\Tests\Session\SessionTest: test runner returned a non-zero error code (255).
Does the fallback works automagicly or do we need to "use" he symfony directories?
Comment #23
neclimdulPlayed with this a little this morning. First thing I noticed, I think the registerPrefixFallback comment mis-interpreted something in the PR on github or something has changed or I'm missing something because they're using registerPrefix() https://github.com/symfony/symfony/pull/3384/files
Second, the path to stubs is actually
$namespaces['Symfony\Component\HttpFoundation'] . 'Symfony/Component/HttpFoundation/Resources/stubs'
. I didn't even completely read the var_dump apparently, it only contains the path to the base of the namespace so we need to put the rest of the namespace in the folder structure.I don't have a patch because it's still failing somehow. I have this in my bootstrap to test it.
This outputs:
Note: /var/www/d8/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Resources/stubs exists on the file system and contains the file SessionHandlerInterface.php.
Comment #24
marcingy CreditAttribution: marcingy commentedThey have a docs issue then as the readme states :(
Is the crux of the issue that we are setting the autload to FALSE when doing the interface test?
Comment #25
neclimduloh... "settings autoload to FALSE"... copy paste fail...
lets see how the testbot likes this.
Comment #26
neclimdulSo there we go. passing patch! Don't know what the plan was from here but we've got an implementation.
Comment #27
cosmicdreams CreditAttribution: cosmicdreams commentedAwesome! Thanks neclimdul!
From here the plan was to use the session from the DI as per pounard's previous patch. And then figure out how to properly handle HTTPS requests.
I don't have the full plan in my head right now I'll have to read up on it again. But I think the plan might have to change. I don't recall any code referencing the session that the request object owns. That I feel is a major deficiency.
Comment #28
marcingy CreditAttribution: marcingy commentedComment #28.0
marcingy CreditAttribution: marcingy commentedUpdated issue summary.
Comment #29
pounardPlan is to try to restore the rest of the patch I guess :) But due to all recent changes this won't be easy.
Comment #30
cosmicdreams CreditAttribution: cosmicdreams commentedpounard!!!! Hi how have you been?
Please stop by #1858198: Implement Symfony's Session handling in core and take a look at the SessionListener I found in Symfony. Seems like a good idea, but it's not working well for me. Let me know what you think.
Comment #31
neclimdulwe'll figure it out. it will be easier with some code set and some traction in core. I'm excited guys! Hope we can get a yay/nay soon :)
Comment #32
sunHm... I reviewed this patch, and I made sure to understand the plan of attack. Works for me — even though I think the original master patch wasn't that far away from being ready...
So in looking through this fairly minimal thingie (that actually doesn't do anything, so my review is purely theoretical, just like the code)... — some of the contained prose surely made me laugh :)
So here's the thing: Even when splitting the major effort up into bite-sized, chunked patches, then we still need to follow coding standards and coding style for each of those smaller patches... And, yeah, less prose please ;-)
Heh, can we tone this down a bit and keep out the hard emotions? ;)
These shouldn't have @ in front of them.
mmm, the language in some of the contained comments really worries me... talking to your code, eh? ;)
Can we scale these comments down to pure, technical facts? :) Actually, after calming down, most of them look like they'll consume a single line or two only. ;)
oops
Isn't this a little bit too late? Session auto-start needs to be disabled at php.ini or .htaccess level already. (We're disabling it in .htaccess.)
Hmmm... this gets set long before in settings.php already, before anything Symfon-ish and Drupal-ish is booted.
In tests, we should always use $this->container.
Also, how about adding at least one assertion there to confirm that the returned variable actually is what you expect?
And another method call to actually perform an action on the session object would be fantastic. :) E.g., ->migrate().
Comment #33
cosmicdreams CreditAttribution: cosmicdreams commentedThanks for the review sun. I'll work to improve the comments and other uncommitables.
Comment #34
cosmicdreams CreditAttribution: cosmicdreams commentedThis fixes many typos, cleans up the comments some, and adds a test to the testDICSession() test case.
Comment #35
cosmicdreams CreditAttribution: cosmicdreams commentedForget 34, that was the wrong patch. This one is much better.
Comment #36
cosmicdreams CreditAttribution: cosmicdreams commentedgoing to try this again. if the patch's file name isn't 1858196_35.patch then it's the wrong one.
Comment #37
chx CreditAttribution: chx commentedWe have
DatabaseExceptionWrapper
for this purpose.in advance. The n is missing. And also, a global? In a constructor? Ouch.
Doesn't this whole enchilada needs a Request scope and the request inject and read cookies from there?
A raw setcookie? See above for the Request object... but I am not sure whether that has a setcookie method but this seems wrong in the subrequest, mock for unit test etc world.
uniqid(mt_rand())
what's the purpose of this code?private? Either final the class or make it protected.
Comment #38
pounardMost of the globals are copy/pasted code from the original session handling code from Drupal 7 / actual HEAD. It surely can be improved, but let's make tests pass before doing that.
Using the Request would be probably better instead of reading $_COOKIE, but I'd rather keep this if the global setcookie() call remains. Either everything goes contextual, either keep it all global. We probably can make this a whole lot cleaner but let make the tests pass first using the original code knowing that this is supposed to work, after that we can refactor this class.
Regarding the drupal_hash_base64(uniqid(mt_rand(), TRUE) this is actually original code from HEAD session handling.
If I remember correctly (a few monthes passed since) I did catch the \PDOException myself back in the original patch because some of these were raised without the wrapper, maybe the Database layer has been fixed since.
Comment #39
Crell CreditAttribution: Crell commentedIt's probably better for now to use Database::getConnection()->query() (et al) for now. Eventually this will be an injected DB object, but for now that at least will lazy load nicely.
Comment #40
cosmicdreams CreditAttribution: cosmicdreams commentedWith a week left I think we need to go back to doing all of this in one patch. So here's an attempt to move in that direction.
This patch consolidates the efforts of this patch and #1858198: Implement Symfony's Session handling in core by including Symfony2 FrameworkBundle's SessionListener and registering it into our CoreBundle. Local testing shows that this leads to some failed tests, I'll have to figure out how to resolve them.
I'll try by applying the remainder of changes advocated by the previous session patch and see what needs to be altered from there.
Comment #42
pounardWe should right now inject it, since we are working with the DIC.
EDIT: Oh I see what you mean with lazy loading. But as soon as we manipulate session, the database will be initialized anyway?
Comment #43
dawehnerCan we get this file in via composer instead of copying it into that directory?
Comment #44
cosmicdreams CreditAttribution: cosmicdreams commenteddawehner: I don't think so, this comes from Symfony2's FrameworkBundle. Which is rather large to pull in.
Comment #45
cosmicdreams CreditAttribution: cosmicdreams commentedGood news watchers of this issue. dawehner and I are getting together tomorrow to get some solid work done here.
As per EclipseGC's suggestion, we'll focus on getting the first patch that registers the service properly to the DI container working. We'll also add some tests that demonstrate the way it will be used in core.
Then we'll work on getting the SessionListener in and revise the code provided by the session objects to work with the current state of D8.
Comment #46
dawehnerSo we did quite some coding of tests and realized that you have issues with NativeStorage once you are in the context of a simpletest, because just initialize NativeStorage loads the values from $_SESSION.
Comment #47
cosmicdreams CreditAttribution: cosmicdreams commentedto recap, we're taking the session listener out of this patch so we can re-introduce it later. We're focusing on demonstrating the way the session will be used with tests so we can be sure it's solid.
Comment #48
cosmicdreams CreditAttribution: cosmicdreams commentedputting to needs review so we can see how many tests fail.
Comment #50
ParisLiakos CreditAttribution: ParisLiakos commentedI already cleaned this up quite a bit, checking simpletest now
also closed #1545680: Convert all uses of $_SESSION to symfony session syntax as duplicate
Comment #51
Crell CreditAttribution: Crell commentedTagging
Comment #52
ParisLiakos CreditAttribution: ParisLiakos commentedwould be nice to have those issues done, for anyone that wants to help with something smaller
#1960344: Replace $is_https global with Request::isSecure()
#1798832: Convert https to $settings
#1929288: Move cryptographic functions to Crypt component
#1969846: Convert session_write_interval to settings
also this will help with the next step
#1825332: Introduce an AccountInterface to represent the current user
Comment #53
ParisLiakos CreditAttribution: ParisLiakos commentednot working anymore on this, stuck on issues above..this should be postponed, but i am not gonna change status in case someone wants to go around them
Comment #54
ParisLiakos CreditAttribution: ParisLiakos commentedWhile waiting for #1929288: Move cryptographic functions to Crypt component
how big hack is DatabaseSessionHandler::updateIdentifiers() ?
Comment #55
ParisLiakos CreditAttribution: ParisLiakos commentedComment #56
Crell CreditAttribution: Crell commentedComment #58
ParisLiakos CreditAttribution: ParisLiakos commentedlets make this green
Comment #59
ParisLiakos CreditAttribution: ParisLiakos commentedand a title with the scope of the patch i previous posted
Comment #61
ParisLiakos CreditAttribution: ParisLiakos commentedwebtests batch in simpletest is failing, and still havent fixed upgrade tests
Comment #63
dawehnerJust a drive-by-review.
Just in general: Wouldn't it be also possible to pass in the session and the session proxy as separate objects? Using the container feels always wrong.
... It seems to be a GeteResponseEvent.
Missing "\"
Is there a reason for the reference? Afaik php 5 objects aren't copied anymore.
What about just provide a default value for this variable?
should be $save_handler
Sounds like a possible feature request for symfony.
What about "... when trying to delete session data"?
Needs documentation.
Comment #64
ParisLiakos CreditAttribution: ParisLiakos commentedfixed simpletest batch..at least sanity is back there..lets see where we are now
also fixed most points above..the reference on $user global is necessary cause in the start it is normally
null
Comment #65
ParisLiakos CreditAttribution: ParisLiakos commentedComment #67
ParisLiakos CreditAttribution: ParisLiakos commentedComment #69
ParisLiakos CreditAttribution: ParisLiakos commentedi made upgrade tests not fatal but fixing the rest of simpletest is way above me
Comment #70
Crell CreditAttribution: Crell commentedLet's see what's left.
Comment #72
cosmicdreams CreditAttribution: cosmicdreams commentedrerolled to see where we stand.
Comment #73
ParisLiakos CreditAttribution: ParisLiakos commentedComment #75
ParisLiakos CreditAttribution: ParisLiakos commentedthis will fix bot
also now that #1953800: Make the database connection serializable is in i updated the handler
Comment #77
ParisLiakos CreditAttribution: ParisLiakos commentedthis empty check supposed to be isset
Comment #79
cosmicdreams CreditAttribution: cosmicdreams commentedI'm going to be working on this patch today. Right now, a good path towards discovering the problems that lead to the test failures has been to find every use of depreciated methods or functions and convert them to non-depreciated methods.
Comment #80
cosmicdreams CreditAttribution: cosmicdreams commentedHere's a reroll
Comment #82
cosmicdreams CreditAttribution: cosmicdreams commentedPerhaps this is the course of least resistance for now: http://symfony.com/doc/master/components/http_foundation/session_php_bri...
Comment #83
ParisLiakos CreditAttribution: ParisLiakos commentedlets see how this go under php5.3
locally simpletest is not failing anymore:)
Comment #84
ParisLiakos CreditAttribution: ParisLiakos commentedsorry, this is the correct patch, but please, dont cancel the previous one, i still want to see how it acts under 5.3
Comment #86
ParisLiakos CreditAttribution: ParisLiakos commentedmeh, conflicts
Comment #88
corvus_ch CreditAttribution: corvus_ch commentedNow that we have a pluggable authentication system (#1890878: Add modular authentication system, including Http Basic; deprecate global $user), this task should be easier to resolve I guess. Also have a look at the follow up #2032115: Remove session.inc and move code to the cookie authentication provider.
Comment #89
Letharion CreditAttribution: Letharion commentedThis patch installs locally. Let's see what the bot says.
The interdiff needs a sanity check from someone less tired, which could be me, after some rest. :)
Can someone explain what made the patch double in size between #54 and #58?
Comment #90
Letharion CreditAttribution: Letharion commentedNo actual interdiff uploaded cause interdiff didn't like parsing the patches. :(
Comment #92
ParisLiakos CreditAttribution: ParisLiakos commentedfor anyone that wants to work on that:
https://drupal.org/node/1969260/git-instructions/symfony-session
make sure to checkout the latest code.
@Letharion i granted you access.
and installation only fails for php 5.3, so you ll need 5.3 enviroment to debug this
Comment #92.0
ParisLiakos CreditAttribution: ParisLiakos commentedA more verbose issue summary
Comment #92.1
ParisLiakos CreditAttribution: ParisLiakos commentedadd sandbox link
Comment #92.2
ParisLiakos CreditAttribution: ParisLiakos commentedUpdated issue summary
Comment #93
Letharion CreditAttribution: Letharion commentedThanks :) I'll see about the 5.3 issue. :)
Comment #94
Letharion CreditAttribution: Letharion commentedCannot reproduce with 5.3 on my machine. :(
Comment #95
Letharion CreditAttribution: Letharion commented#89: drupal-sf_sessions-1858196-89.patch queued for re-testing.
Comment #97
Letharion CreditAttribution: Letharion commentedThe error message that comes from the test bot originates in one of the test-bot specific modules.
Randy said on IRC that if we're making changes to the install process (which we are), simpletest/testbot may also need updates. I've set up my own testbot, and will try to figure out the details of what's happening.
Comment #98
Letharion CreditAttribution: Letharion commentedReroll.
Comment #100
Letharion CreditAttribution: Letharion commentedProblem:
PHP Fatal error: Cannot access protected property Drupal\\Core\\Session\\UserSession::$uid in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/session.inc on line 47
Not sure why this doesn't manifest outside of the testbot?
Solution/Interdiff:
- if ($user->uid && REQUEST_TIME - $user->access > settings()->get('session_write_interval', 180)) {
+ if ($user->id() && REQUEST_TIME - $user->access > settings()->get('session_write_interval', 180)) {
Unfortunately the test doesn't get much longer, but it's a small step forward.
Comment #102
Berdirglobal $user->uid is protected now, you need to update all instances that directly access it:
isAuthenticated() is the same as id() in this case but is self-documenting.
id()
$user = $request->attributes->get('account') should work now.
$user->isAuthenticated() instead of the !empty()
isAnonymous() in this case.
not sure if the second condition here is actually used, if it returns something, then uid is always set (protected now), and methods are also always there.
id()
You should mock the id() method instead. And assigning a rand() seems weird? 0 might have completely different behavior than 1?
This call and others in similar files are already in core now, in different places. Just a few lines above this one, in this case ;)
Comment #103
fubhy CreditAttribution: fubhy commentedComment #104
fubhy CreditAttribution: fubhy commentedI decided to properly set the property instead.
Apparently that's invoked too early in the request. I tried to use the account from the request but it was not set in those places yet.
Comment #106
BerdirUh, why would you do that? :)
It's already a mock class, no need to mess with reflection to set protected properties that are not actually used as id() only does what we tell it to do?
Comment #107
fubhy CreditAttribution: fubhy commentedUmh, no reason, yeah. That was stupid.
Comment #109
Letharion CreditAttribution: Letharion commentedThe reason the install fails is that the testbot tries to submit the last config step, site-name, admin account name etc, but Drupal is serving up the initial install step, the language selection.
Since the forms don't match, the test browser reject the installation as a failure.
Could it be that our session changes makes state propagation between the various forms fail, so that Drupal accidentally reverts to the language selection because previous choices get lost?
That would explain why I can install locally, because I do it with Drush only. I couldn't make the UI installer work at all due to #1987262: [Installation Error] Symfony: "The service definition 'request' does not exist.". Significant work seems to have gone into that one since I tried though, so I'll give it another go.
Comment #110
Letharion CreditAttribution: Letharion commentedNah, still can't reproduce the form error locally. :(
Comment #111
webchickIt's my understanding that this patch is required to remove these lines from drupal_handle_request():
Since this would remove 4 bootstrap levels from every page request, this seems like it should be major. We can't call it critical, because Drupal 7 did a DRUPAL_BOOTSTRAP_FULL on every request, but if we could get this done it would be pretty cool.
That said, we are now past API freeze, so we need to be more careful, e.g. leaving in BC layers and marking things @deprecated or the like wherever possible.
Comment #112
Crell CreditAttribution: Crell commentedI don't know how feasible a BC would be, since the existing API for sessions is $_SESSION, isn't it? (I'd still love to get this in.)
The blocker here, as far as I understand it, is another chicken-and-egg issue with testbot where certain changes have to be coordinated so that the testbot can understand new bootstrap code. (Side note: Code smell in testbot AND bootstrap.) If we want to resolve that, we will need some testbot-fu, I think.
Comment #113
markhalliwellThis issue would help #1798732: Convert install_task, install_time and install_current_batch to use the state system tremendously. We need a way to save the state (probably using session) and then transferring that to the DB once we can fully bootstrap in install.
Comment #114
pounardSymfony session handler when using the native session storage implementation allows to use the $_SESSION[] array directly, which means no BC break.
Comment #115
cosmicdreams CreditAttribution: cosmicdreams commentedI'm very excited to see this issue gain the support of other Drupal developers. I would very much love to see it completed for D8. I've tried multiple times to get it to pass all tests and failed but willing to keep trying as long as I'm helping.
I've tried to apply the patch in #107. I'll try to reroll and start from there.
Comment #116
cosmicdreams CreditAttribution: cosmicdreams commentedOK, I think I've rerolled this, but it has problems. I've refactored one function and added a todo where _drupal_session_write should be removed from a test case.
Here are the problems I found:
* WebTestBase.php has a todo that refers to the _drupal_session_write
* CookieProxy.php has an error because it is trying to set user access roles and that functionality no longer exists.
There may be other issues with the session.inc but I think they might be obvious to others who have seen this patch a few times. In short, we need more eyeballs on this patch. We might be close.
Comment #117
Crell CreditAttribution: Crell commentedOh good lord it's passing! Chris, which god did you sacrifice a virgin to in order to get testbot to behave? :-)
While I'm tempted to just hit the RTBC button while we can, I do have some comments below. None is fatal. We should get someone more familiar with this part of the old system to comment, though.
I'm tempted to just rip this out entirely, since drupal_page_is_cacheable() needs to go away anyway, but I don't know if that will break tests. If not, remove. If it does, leave it. :-)
Let's give this an explicit weight, for clarity.
Really? Is trigger_error() ever the correct thing to do in modern PHP? I suspect not... If there's a reason we're doing that, it should be documented. Also, please include an explicit level.
The DB should be injectable at this point.
Comment #118
ParisLiakos CreditAttribution: ParisLiakos commentedthis is passing because the code is dead. it is not actually being used. #107 is 72KB
Comment #119
tim.plunkett#107 needs a reroll. Not #116.
Comment #120
pfrenssenRerolled #107.
Comment #121
pfrenssenThis is the right patch.
Comment #123
pfrenssenComment #124
tim.plunkettCookieProxy tried to use protected properties from UserSession. I moved the setters from UserInterface up to AccountInterface.
Comment #126
BerdirThe login after installation seems to be broken, it doesn't allow me to log in. I'm pretty sure that's what's causing this to fail, because the testbot does a logout and login after the install.
Comment #127
tim.plunkettBerdir pointed out that in #2045275: Convert user properties to methods, session.inc was changed, and that hasn't been reflected in this patch.
The interdiff is against #121, reverting the changes from #124.
Comment #129
ParisLiakos CreditAttribution: ParisLiakos commentedhmm maybe this is the root of all evil for "session already started" simpletest errors
Comment #130
Crell CreditAttribution: Crell commented#107: 1858196-107.patch queued for re-testing.
Comment #131
jthorson CreditAttribution: jthorson commentedFor #127:
On testbot, once hitting the database configuration page, I got taken to the language page again ... and after selecting the language, to the installation profile page. The testbot sets both of these as attributes in the original install URI (http://drupaltestbot-dev-mysql.osuosl.test/checkout/core/install.php?lan...), and does not expect to see them again.
Once I select the language and installation profile, then fill in the admin account information, and proceed with the installation, I get a white screen and the following error in the apache log:
Comment #132
dawehnerThe problem mentioned in 131 seemed to be fix in core already. This is just a straight rerole.
Comment #134
cosmicdreams CreditAttribution: cosmicdreams commented#132: session-1858196-132.patch queued for re-testing.
When this was uploaded, something screwy was happening with the installation process, (as I could verify it was leading to ajax errors when I tried on my own machine).
Comment #136
RainbowArrayHere's a reroll of #132 based on suggestions by larowlan.
Comment #138
RainbowArrayI was able to get a local installation to work. I did receive the following red warning upon installation:
"Notice: Undefined index: update in module_set_weight() (line 342 of core/includes/module.inc)."
Comment #139
cosmicdreams CreditAttribution: cosmicdreams commentedNot proper syntax anymore should use \Drupal::
Also, use of \Drupal::getContainer is deprecated.
We shouldn't get the request this way.
Should use \Drupal::request() instead
I think this should be \Drupal::service('session') now.
see Drupal::service('session') is used here. Why isn't this .inc file consistent?
Comment #140
cosmicdreams CreditAttribution: cosmicdreams commentedBut we aren't using $user as a global anymore right?
Comment #141
cosmicdreams CreditAttribution: cosmicdreams commentedNot sure how to handle the global user stuff but here's a new patch that fixes the out of date service calls.
Comment #142
cosmicdreams CreditAttribution: cosmicdreams commentedComment #144
cosmicdreams CreditAttribution: cosmicdreams commentedMade my patch wrong
Comment #146
joelpittet#144: session_1858196_143.patch queued for re-testing.
Comment #148
jthorson CreditAttribution: jthorson commentedEDIT: Manually submitting the final 'Site information' installation page takes me to a WSOD.
This is what I get when visiting the site after the failed install:
Comment #149
cosmicdreams CreditAttribution: cosmicdreams commentedSooooooo yea, we'll need to reroll this. Lot's changed since last time.
Comment #150
joelpittetSo this is probably not kosher but I added back in the !drupal_is_cli()...
Comment #152
joelpittet#150: 1858196-150-session-service.patch queued for re-testing.
Comment #154
joelpittetWell that did work locally with drush :(
Comment #155
cosmicdreams CreditAttribution: cosmicdreams commentedManually tested #150 on my laptop that is running PHP 5.4.9 and everything installed fine.
Re: #148
@jthorson that seems unrelated to this patch. We might be in the process of removing the legacy field api stuff.
I think the environment that is failing is getting this code to run for PHP 5.3 as there it is easier to register legacy session support into a SessionListener with PHP 5.4.
I wish we could get better error messages. I'll see if I can demote the version of php I'm using to manually test php 5.3
Comment #156
cosmicdreams CreditAttribution: cosmicdreams commented#150: 1858196-150-session-service.patch queued for re-testing.
Comment #158
jthorson CreditAttribution: jthorson commentedRan manually against a 5.4.20 testbot. Too much noise for a decent result (environment issue), but the following issues occurred frequently, and look like they're probably relevant:
Comment #159
cosmicdreams CreditAttribution: cosmicdreams commentedSo we have two classes named SessionTest. This smells to me. It's likely that we haven't modified this patch since AccountInterface was introduced.
All of the other classes in Drupal\Core don't have tests, so why do we provide them here?
Comment #160
cosmicdreams CreditAttribution: cosmicdreams commentedUpdated SessionListener with most recent from https://raw.github.com/symfony/symfony/master/src/Symfony/Bundle/Framewo...
Comment #162
cosmicdreams CreditAttribution: cosmicdreams commentedNamespace was wrong with the new SessionListener which made it fail to register properly.
Comment #164
neclimdulTrying this, diffed off of #150. Installed cleanly local and fixed a warning I was getting on the command line.
Comment #165
neclimdulrun tests to see.
Comment #167
neclimdulSo this is what I found looking into the install problem on the test bot.
In
install_configure_form_validate()
we calluser_validate_name()
.user_validate_name()
callsAt this point typedData goes nuts and long story short we end up in
FieldTypePluginManager::__construct()
where we try to instantiateuse Drupal\field\Plugin\Type\FieldType\LegacyFieldTypeDiscoveryDecorator
The current state of the class loader is not able to find that and we get a Fatal error. I'm not sure why this is suddenly different but whats important here is Drupal\field is a module namespace not a core namespace so it requires more of a bootstrap to be available and something has changed so that's not available.
Tag, someone else is it. I'm going to get some sleep.
Comment #167.0
neclimdulMake the sandbox url a link
Comment #168
joelpittet164: drupal8.user-system.1858196-164.patch queued for re-testing.
Comment #170
XanoComment #171
cosmicdreams CreditAttribution: cosmicdreams commentedIn testing this patch again tonight I found that I wasn't able to complete the installation normally. The installation process hung at the last stage of module installation (it appeared to be stuck on Tour UI). Diving deeper.
Also, rerolled patch, interdiff looks strange though. Diagnosing...
Comment #172
neclimdulEverything went smooth testing this locally. Need to run it through test bot. I'll dig through the other form in a sec and try to trigger that.
$user->access causes a notice. i think its suppose to be $user->getLastAccessedTime()
Comment #173
neclimdulComment #175
joelpittet@neclimdul I think you're right, here's the patch.
Comment #177
cosmicdreams CreditAttribution: cosmicdreams commentedUpdated issue summary with the issue summary template, tried to explain the purpose of this patch better. More changes to come when I have time.
Comment #178
cosmicdreams CreditAttribution: cosmicdreams commentedComment #179
cosmicdreams CreditAttribution: cosmicdreams commentedComment #180
cosmicdreams CreditAttribution: cosmicdreams commentedComment #181
cosmicdreams CreditAttribution: cosmicdreams commentedComment #182
cosmicdreams CreditAttribution: cosmicdreams commentedComment #183
cosmicdreams CreditAttribution: cosmicdreams commentedComment #184
cosmicdreams CreditAttribution: cosmicdreams commentedComment #185
joelpittetThis needs a re-roll, the changes look a bit rich for my blood.
Comment #186
joelpittetThis shouldn't be necessary now as 5.4 is a requirement
#2152073: Bump Drupal core's PHP requirement to 5.4.2 which is postponed.
Comment #187
damiankloip CreditAttribution: damiankloip commentedReroll of #175 plus missing use statement in SessionTestSubscriber.
Have there been changes in session.inc lately that I didn't capture here? I think the changes are just to do with the replacement of super globals, and error handling.
Comment #188
damiankloip CreditAttribution: damiankloip commentedComment #192
jthorson CreditAttribution: jthorson commentedTried to revisit what was causing the installation issues with this one, but patch no longer applies. Will require a reroll, and then ping me to dig into the logs.
Comment #193
XanoComment #195
jibranDrupal installation worked locally I think this patch is ready for reviews.
Comment #196
XanoI did a quick test on Simplytest.me and had trouble installing a site, though. I'm not sure what's going on, but maybe this piece of info is helpful.
Comment #197
ParisLiakos CreditAttribution: ParisLiakos commentedthe installer fails because we the session needs to be updated to account for the current_user service.
i did some work to fix that in prague, i will post it tomorrow
Comment #198
joelpittetDon't know if this helps any but the error message I got on simplytest.me is
Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "field.info" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 873 of core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).
Could be more of a symptom than the cause, though likely related to #197
Comment #199
joelpittetAlso for the user service there are a few more to do, I tried to do as many as possible in one issue from a request from xjm here #2163203: Remove calls to deprecated global $user wherever possible (outside bootstrap and authentication)
Comment #200
ParisLiakos CreditAttribution: ParisLiakos commentedwell, we need to do #2187431: Remove current_user service, retrieve it from the authentication manager
otherwise this is impossible unless we do everything ContainerAware
Comment #201
XanoThis is blocked on #2180109: Change the current_user service to a proxy instead.
Comment #203
cosmicdreams CreditAttribution: cosmicdreams commentedJoel: the problem you are experience as long been the bane of this issue. I've been having that issue for a very long time. I haven't been able to track down the cause.
Comment #204
cosmicdreams CreditAttribution: cosmicdreams commentedOh now I remember. neclimdul has some good insight as to why this is happening in #167.
We're using the TypedDataAPI too early. It's actually a rather large issue related to that API. Don't know what the fix for that should be.
Comment #205
andypostThe overall approach is interesting, just wondering that it started as a stub to pass a tests!
Once core in polish phase this patch does not looks like feature-complete.
The used
settings()->get('session_inc', ..
is not documented insettings.php
- any reason to have 2 kind of overrides for session handling? and the same time deprecated a half of functions... this actually needs work.This brings more confusion to core.
CookieProxy
accepts settings singleton argument and stores in protected variable - is it ok?I think the magic with autoload could be removed, Symfony already provides forward compatibility via NativeSessionHandler so maybe better to re-use it.
Also summary need update about current architecture and possible usage.
currentUser() service already could be used.
Anyway there's too much references to global user added but init() is called in different contexts
no doc block
Comment #206
cosmicdreams CreditAttribution: cosmicdreams commentedAs a bit of history on this issue: the approach that we see in this issue is largely unchanged from where the code was a year ago. This despite the dramatic amount of change we've seen in core.
So when you find things like a the improper use of settings() and use of global user keep in mind that those are things that were introduced after the majority of this code was architected. I agree that we certainly should evolve this code to adopt the new code that the rest of core enjoys. From time to time I've attempted to do that (specifically use currentUser) but have usually found that those systems weren't ready, that I couldn't use those systems so early within the lifecycle of a request, or that I was just plain not using them from lack of understanding.
Please feel free to evolve this patch as you see fit. I am eager to pitch in, but am more scarce on time than I've ever experienced.
Comment #207
sunBy now we should know that the taken approach of Blatantly Replace All The Things™ and Adapt To Fancy New Bells & Whistles™ does not work out and will not work out.
The path to success has a name: Baby steps.
Then, and only then, consider to swap out and further abstract the implementation by adopting the Symfony HttpFoundation SessionHandler pattern with all the proxies, bells, and whistles. → Inherently much simpler to do, because all the legacy crap has been objectified already.
This issue is not only stuck on getting past the automated tests gate (in partially questionable ways after glancing over the latest patch). The first and foremost reason for doing baby steps is that there are literally only a handful of people who would actually be able to review the massive changes in this patch and understand the consequences of every single change to its fullest extent.
Postponing this issue until steps 1) to 3) are done.
Comment #208
Crell CreditAttribution: Crell commentedJust a note that PHP 5.4 testbots are now a thing (woohoo!), so we can ignore the 5.3-support now. Hopefully that makes life easier.
Comment #209
sunBoth baby steps have landed:
#2205295: Objectify session handler
#2228341: Objectify session management functions + remove session.inc
→
/core/includes/session.inc
is gone + we have the following classes now:Some sub-issues have been created as follow-ups to the
SessionManager
conversion:#2228393: Decouple session from cookie based user authentication
#2229145: Register symfony session components in the DIC and inject the session service into the request object
That said, I'm not sure whether it makes sense to continue to split the effort into child issues, because — the main goal is achieved:
Changes to the session code can be properly reviewed now. Patches are no longer swapping out ~2,000 LoC with completely different code + concepts. :-)
So it could make sense to attack the full conversion in one go now, but I'm leaving that decision to @znerol, @skipyT, and @ParisLiakos.
All of that being said, in case of going with a single issue, it might make sense to restart in a new one (with a clear issue summary explaining the proposed changes), since this one here is >200 comments already...
Comment #210
cpj CreditAttribution: cpj commented+1 for a single issue that completes the task - the two new sub-issues seem inter-related to me & I don't think they cover all that needs to be done.
Comment #211
znerol CreditAttribution: znerol commentedI think it would be great to follow-up on #2228341: Objectify session management functions + remove session.inc and rebase
SessionManager
onto[...]\HttpFoundation\Session\Storage\NativeSessionStorage
. After that we can introduce the Symfony session object and add it to therequest
and gradually remove the Drupal custom session management stuff.Comment #212
znerol CreditAttribution: znerol commentedSubissue: #2238087: Rebase SessionManager onto Symfony NativeSessionStorage
Comment #213
sunBriefly discussed with @znerol — let's move forward with 1-2+ child issues to complete this effort, inherently turning this issue into a meta issue.
Comment #214
cosmicdreams CreditAttribution: cosmicdreams commentedComment #215
znerol CreditAttribution: znerol commentedAlso filed #2238561: Use the default PHP session ID instead of generating a custom one.
Comment #216
cosmicdreams CreditAttribution: cosmicdreams commentedadded
Comment #217
znerol CreditAttribution: znerol commented#2238087: Rebase SessionManager onto Symfony NativeSessionStorage landed. Would be great if we could fix #2122761: SessionHttpsTest only runs half the tests before the refactoring continues.
Comment #218
YesCT CreditAttribution: YesCT commented#2286591: [meta] Security audit the Authentication component is postponed on this. updated issue summary with that info.
Comment #219
znerol CreditAttribution: znerol commentedAdding #2256257: Move token seed in SessionManager in isSessionObsolete() and regenerate() to the MetadataBag
Comment #220
znerol CreditAttribution: znerol commentedNot directly related to the Symfony conversion process but #2265099: Cleanup SessionHttpsTest and fix redirect to non-existing URL after POST requests and #2330279: When operating in mixed mode SSL: data from anonymous non-HTTP session is still available after login, both need review.
Comment #221
znerol CreditAttribution: znerol commentedAdding another piece to the puzzle #2338571: Remove SessionManager::startLazy().
Comment #222
znerol CreditAttribution: znerol commentedAnother one #2338727: Replace static SessionManager::$enabled property with WriteSafeSessionHandler class and resolve hidden circular dependency between SessionManager and SessionHandler. This has its own issue because it introduces a new class plus unit-tests.
Comment #223
znerol CreditAttribution: znerol commentedAlso #2342593: Remove mixed SSL support from core could streamline further development.
Comment #224
XanoSeeing as SessionManager now uses the Symfony session handling components, is this issue still necessary?
Comment #225
znerol CreditAttribution: znerol commented@Xano: The conversion not yet complete. #2229145: Register symfony session components in the DIC and inject the session service into the request object will put the remaining services in place, but even after that we need to clean-up session manager and the session handler quite a bit in order to make it easy to replace it.
Comment #226
znerol CreditAttribution: znerol commentedNeed reviews on #2347877: Move DrupalKernel::initializeCookieGlobals() into a SessionConfiguration service, this will help with #2342593: Remove mixed SSL support from core and also resolves #1934730: Alternative session handler implementation is not able to override session_name() and #2331909: Move DrupalKernel::initializeCookieGlobals() into page cache kernel decorator.
Comment #227
znerol CreditAttribution: znerol commentedPlease help with the issues linked in #2371629: [meta] Finalize Session and User Authentication API. I think we should now concentrate on removing
global $user
and that is only possible when taking in account the session as well as the authentication subsystem.Comment #228
znerol CreditAttribution: znerol commentedFiled #2372389: Expose session handler in container
Comment #229
jibranWould anyone be interested in creating a workflow diagram for session api? I can create a separate issue for that.
Comment #230
YesCT CreditAttribution: YesCT commented@znerol (or other) is this the issue that #2286591: [meta] Security audit the Authentication component is postponed on?
Comment #231
znerol CreditAttribution: znerol commented@YesCT: See #2371629: [meta] Finalize Session and User Authentication API
Comment #232
almaudoh CreditAttribution: almaudoh commentedComment #233
skipyT CreditAttribution: skipyT commentedComment #234
almaudoh CreditAttribution: almaudoh commentedComment #235
znerol CreditAttribution: znerol commentedClosing this as a duplicate of #2371629: [meta] Finalize Session and User Authentication API.