Issue Summary
In #335411: Switch to Symfony2-based session handling we tried to create a patch that re-implement Drupal session handling all in one go. Our efforts in that patch were frequently hindered by a fast moving head. Instead we are trying to create a series of patches that will help land a Symfony 2 based implementation of Sessions while not having to reroll the patch as often.
This patch does just one thing, Register an implementation of the Symfony 2 Session object onto the DI Container. The actual implementation is not complete and will be fleshed out in #1858198: Implement Symfony's Session handling in core and #1858200: Implement secure session with Symfony's Session object. When we finish these three issues, we'll have a working implementation of Symfony 2's session handling in Drupal.
Previous summary:
In order to continue the efforts of #335411: Switch to Symfony2-based session handling, we aim to break the complex issue into more digestible parts.
Step 1: This issue
We contribute an implementation of Symfony's Session object to core but don't include any code that uses it.
Step 2: We implement the basics of Session management using Symfony's Session Object.
#1858198: Implement Symfony's Session handling in core
Step 3: We implement complex topics
Lazy Loading (should be automatically provided by Symfony's Session but extra work may be required)
#1858200: Implement secure session with Symfony's Session object
Comments
#1
Here'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.
#2
The last submitted patch, 1858196_1.patch, failed testing.
#3
+++ b/core/lib/Drupal/Core/Session/Handler/DatabaseSessionHandler.php@@ -0,0 +1,87 @@
+class DatabaseSessionHandler implements \SessionHandlerInterface {
SessionHandlerInterface was only added in PHP 5.4:
http://us3.php.net/sessionhandlerinterface
:-(
#4
Seems 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.
#5
This 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.
#6
+++ b/core/includes/bootstrap.incundefined@@ -3113,6 +3113,10 @@ function drupal_classloader() {
+ if (!interface_exists('SessionHandlerInterface')) {
+ $loader->registerPrefixFallback($namespaces['SessionHandlerInterface']);
+ }
I 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?
#7
OH Ok, I get it. Here's the code form their pull request
if (!interface_exists('SessionHandlerInterface')) {$loader->registerPrefixFallback(__DIR__.'/../vendor/symfony/src/Symfony/Component/HttpFoundation/Resources/stubs');
}
which since we have the path to the HttpFoundation library at that point in the bootstrap.inc we can change to:
if (!interface_exists('SessionHandlerInterface')) {$loader->registerPrefixFallback($namespaces['HttpFoundation'] . "/Resources/stubs');
}
#8
$loader->registerPrefixFallback($namespaces['SessionHandlerInterface']);Is the same as $namespaces['HttpFoundation'] . "/Resources/stubs' if you look in the compsor file, ie we actually have the namespace qualified already as
'SessionHandlerInterface' => $vendorDir . '/symfony/http-foundation/Symfony/Component/HttpFoundation/Resources/stubs',#9
#5: session-ph53.patch queued for re-testing.
#10
The last submitted patch, session-ph53.patch, failed testing.
#11
I'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:
array
'Symfony\Component\Yaml' => string '/var/www/.../core/vendor/symfony/yaml/' (length=40)
'Symfony\Component\Serializer' => string '/var/www/.../core/vendor/symfony/serializer/' (length=46)
'Symfony\Component\Routing' => string '/var/www/.../core/vendor/symfony/routing/' (length=43)
'Symfony\Component\Process' => string '/var/www/.../core/vendor/symfony/process/' (length=43)
'Symfony\Component\HttpKernel' => string '/var/www/.../core/vendor/symfony/http-kernel/' (length=47)
'Symfony\Component\HttpFoundation' => string '/var/www/.../core/vendor/symfony/http-foundation/' (length=51)
'Symfony\Component\EventDispatcher' => string '/var/www/.../core/vendor/symfony/event-dispatcher/' (length=52)
'Symfony\Component\DependencyInjection' => string '/var/www/.../core/vendor/symfony/dependency-injection/' (length=56)
'Symfony\Component\ClassLoader' => string '/var/www/.../core/vendor/symfony/class-loader/' (length=48)
'Symfony\Cmf\Component\Routing' => string '/var/www/.../core/vendor/symfony-cmf/routing/' (length=47)
'Guzzle\Stream' => string '/var/www/.../core/vendor/guzzle/stream/' (length=41)
'Guzzle\Parser' => string '/var/www/.../core/vendor/guzzle/parser/' (length=41)
'Guzzle\Http' => string '/var/www/.../core/vendor/guzzle/http/' (length=39)
'Guzzle\Common' => string '/var/www/.../core/vendor/guzzle/common/' (length=41)
'Doctrine\Common' => string '/var/www/.../core/vendor/doctrine/common/lib/' (length=47)
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.
#12
so 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.#13
What about just using that?
#14
According 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.
#15
The last submitted patch, drupal-1858196-13.patch, failed testing.
#16
Ah I see.
#17
The last submitted patch, drupal-1858196-16.patch, failed testing.
#18
As 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.
#19
run tests
#20
Ok 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.
#21
The last submitted patch, drupal-1858196-18.patch, failed testing.
#22
Fatal 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?
#23
Played 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.
<?php
if (!interface_exists('SessionHandlerInterface', FALSE)) {
$loader->registerPrefix('SessionHandlerInterface', $namespaces['Symfony\Component\HttpFoundation'] . 'Symfony/Component/HttpFoundation/Resources/stubs');
}
// Register the loader with PHP.
$loader->register();
if (!interface_exists('SessionHandlerInterface', FALSE)) {
echo 'We failed. The session handler interface is still not there!';
echo $namespaces['Symfony\Component\HttpFoundation'] . 'Symfony/Component/HttpFoundation/Resources/stubs';
}
?>
This outputs:
We failed... The session handler interface is still not there!/var/www/d8/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Resources/stubsNote: /var/www/d8/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Resources/stubs exists on the file system and contains the file SessionHandlerInterface.php.
#24
They have a docs issue then as the readme states :(
// SessionHandlerInterfaceif (!interface_exists('SessionHandlerInterface')) {
$loader->registerPrefixFallback(__DIR__.'/../vendor/symfony/src/Symfony/Component/HttpFoundation/Resources/stubs');
}
Is the crux of the issue that we are setting the autload to FALSE when doing the interface test?
#25
oh... "settings autoload to FALSE"... copy paste fail...
lets see how the testbot likes this.
#26
So there we go. passing patch! Don't know what the plan was from here but we've got an implementation.
#27
Awesome! 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.
#28
#29
Plan is to try to restore the rest of the patch I guess :) But due to all recent changes this won't be easy.
#30
pounard!!!! 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.
#31
we'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 :)
#32
Hm... 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 ;-)
+++ b/core/lib/Drupal/Core/Session/Storage/DrupalSessionStorage.php@@ -0,0 +1,74 @@
+/**
+ * Default session storage.
+ *
+ * This is a proxy class between the $_SESSION super global and the Session
+ * object bags. There is no way on earth we would want to write our own
+ * implementation, this one only exists in order to override some harcoded PHP
+ * ini by the Symfony implementation that may disturb some Drupal cookie
+ * handling implementation details.
Heh, can we tone this down a bit and keep out the hard emotions? ;)
+++ b/core/lib/Drupal/Core/Session/Handler/DatabaseSessionHandler.php@@ -0,0 +1,87 @@
+ * @Implements SessionHandlerInterface::open().
...
+ * @Implements SessionHandlerInterface::close().
...
+ * @Implements SessionHandlerInterface::destroy().
...
+ * @Implements SessionHandlerInterface::gc().
...
+ * @Implements SessionHandlerInterface::read().
...
+ * @Implements SessionHandlerInterface::write().
These shouldn't have @ in front of them.
+++ b/core/lib/Drupal/Core/Session/Proxy/CookieOverrideProxy.php
@@ -0,0 +1,164 @@
+ // Cookie sending must be when we are sure we need to keep the session, this
+ // ensure the lazy session init. Lazy session init is abusive talking we are
+ // not lazy initializing the session, but lazy sending the session cookie
+ // instead. Each anonymous user will intrinsecly have a session tied, which
+ // allows to generate tokens for forms and such, but if the session ends up
+ // empty, the cookies will not be sent and the session will not be saved on
+ // disk.
+++ b/core/lib/Drupal/Core/Session/Session.php
@@ -0,0 +1,131 @@
+ * @todo This is the most absurd implementation that could ever been written
+ * but there is no clean solution because bags can not be directly accessed
+ * via protected attributes, and they don't have either a count() or isEmpty()
+ * method.
...
+ // @todo: This code is incredebly ugly and this foreach needs to be removed
+ // once all Drupal code is ported to use the session bag instead of direct
+ // $_SESSION superglobal usage.
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. ;)
+++ b/core/lib/Drupal/Core/Session/Session.php@@ -0,0 +1,131 @@
+ /**sy
oops
+++ b/core/lib/Drupal/Core/Session/Storage/DrupalSessionStorage.php@@ -0,0 +1,74 @@
+class DrupalSessionStorage extends NativeSessionStorage {
+
+ public function __construct(array $options = array(), $handler = null, MetadataBag $metaBag = null) {
+ // We are going to start the session ourselves.
+ ini_set('session.auto_start', 0);
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.)
+++ b/core/lib/Drupal/Core/Session/Storage/DrupalSessionStorage.php@@ -0,0 +1,74 @@
+ if (null !== $lifetime) {
+ ini_set('session.cookie_lifetime', $lifetime);
+ }
Hmmm... this gets set long before in settings.php already, before anything Symfon-ish and Drupal-ish is booted.
+++ b/core/modules/system/lib/Drupal/system/Tests/Session/SessionTest.php@@ -27,6 +27,13 @@ class SessionTest extends WebTestBase {
/**
+ * Test that a DIC based session can be created.
+ */
+ function testDICSession() {
+ $session = drupal_container()->get('session');
+ }
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().
#33
Thanks for the review sun. I'll work to improve the comments and other uncommitables.
#34
This fixes many typos, cleans up the comments some, and adds a test to the testDICSession() test case.
#35
Forget 34, that was the wrong patch. This one is much better.
#36
going to try this again. if the patch's file name isn't 1858196_35.patch then it's the wrong one.
#37
+++ b/core/lib/Drupal/Core/Session/Handler/DatabaseSessionHandler.phpundefined@@ -0,0 +1,87 @@
+ catch (\PDOException $e) {
We have
DatabaseExceptionWrapperfor this purpose.+++ b/core/lib/Drupal/Core/Session/Proxy/CookieOverrideProxy.phpundefined@@ -0,0 +1,159 @@
+ // (like drupal_get_token()) needs to know the future session ID i
in advance. The n is missing. And also, a global? In a constructor? Ouch.
+++ b/core/lib/Drupal/Core/Session/Proxy/CookieOverrideProxy.phpundefined@@ -0,0 +1,159 @@
+ if (!empty($_COOKIE[$name])) {
Doesn't this whole enchilada needs a Request scope and the request inject and read cookies from there?
+++ b/core/lib/Drupal/Core/Session/Proxy/CookieOverrideProxy.phpundefined@@ -0,0 +1,159 @@
+ setcookie($this->getName(), $id, $expire, $params['path'], $params['domain'], $params['secure'], $params['httponly']);
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.
+++ b/core/lib/Drupal/Core/Session/Proxy/CookieOverrideProxy.phpundefined@@ -0,0 +1,159 @@
+ $id = drupal_hash_base64(uniqid(mt_rand(), TRUE) . drupal_random_bytes(55));
uniqid(mt_rand())what's the purpose of this code?+++ b/core/lib/Drupal/Core/Session/StaticSessionFactory.phpundefined@@ -0,0 +1,44 @@
+ static private $session;
private? Either final the class or make it protected.
#38
Most 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.
#39
+++ b/core/lib/Drupal/Core/Session/Handler/DatabaseSessionHandler.php@@ -0,0 +1,87 @@
+ $data = db_query("SELECT s.* FROM {sessions} s WHERE s.sid = :sid", array(':sid' => $sessionId))->fetchObject();
It'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.
#40
With 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.
#41
The last submitted patch, 1858196_40.patch, failed testing.
#42
We 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?
#43
+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined--- /dev/null
+++ b/core/lib/Drupal/Core/EventSubscriber/SessionListener.phpundefined
Can we get this file in via composer instead of copying it into that directory?
#44
dawehner: I don't think so, this comes from Symfony2's FrameworkBundle. Which is rather large to pull in.
#45
Good 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.
#46
So 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.
#47
to 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.
#48
putting to needs review so we can see how many tests fail.
#49
The last submitted patch, drupal-1858196-46.patch, failed testing.
#50
I already cleaned this up quite a bit, checking simpletest now
also closed #1545680: Convert all uses of $_SESSION to symfony session syntax as duplicate
#51
Tagging
#52
would 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 / UserInterface interface for use in \Drupal\Core namespaces
#53
not 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
#54
While waiting for #1929288: Move cryptographic functions to Crypt component
how big hack is DatabaseSessionHandler::updateIdentifiers() ?
#55
#56
#57
The last submitted patch, drupal-sf_sessions-1858196-54.patch, failed testing.
#58
lets make this green
#59
and a title with the scope of the patch i previous posted
#60
The last submitted patch, drupal-sf_sessions-1858196-58.patch, failed testing.
#61
webtests batch in simpletest is failing, and still havent fixed upgrade tests
#62
The last submitted patch, drupal-sf_sessions-1858196-60.patch, failed testing.
#63
Just a drive-by-review.
+++ b/core/lib/Drupal/Core/EventSubscriber/SessionListener.phpundefined@@ -0,0 +1,114 @@
+ /**
+ * The currently active container object.
+ *
+ * @var \Symfony\Component\DependencyInjection\ContainerInterface
+ */
+ protected $container;
+
+ /**
+ * Constructs a SessionListener object.
+ *
+ * @param \Drupal\Core\Session\Storage\DrupalSessionStorage $container
+ * The currently active container object.
+ */
+ public function __construct(ContainerInterface $container) {
+ $this->container = $container;
+ }
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.
+++ b/core/lib/Drupal/Core/EventSubscriber/SessionListener.phpundefined@@ -0,0 +1,114 @@
+ * @param Symfony\Component\HttpKernel\Event\FilterResponseEvent $event
... It seems to be a GeteResponseEvent.
+++ b/core/lib/Drupal/Core/EventSubscriber/SessionListener.phpundefined@@ -0,0 +1,114 @@
+ * @param Symfony\Component\HttpKernel\Event\FilterResponseEvent $event
Missing "\"
+++ b/core/lib/Drupal/Core/Session/Session.phpundefined@@ -0,0 +1,234 @@
+ public function init(User &$user_session = NULL) {
+ $this->userSession = &$user_session;
Is there a reason for the reference? Afaik php 5 objects aren't copied anymore.
+++ b/core/lib/Drupal/Core/Session/Session.phpundefined@@ -0,0 +1,234 @@
+ if ($this->lastSaveHandlerState !== NULL && $this->lastSaveHandlerState) {
+ $this->storage->getSaveHandler()->setActive($this->lastSaveHandlerState);
+ }
What about just provide a default value for this variable?
+++ b/core/lib/Drupal/Core/Session/Session.phpundefined@@ -0,0 +1,234 @@
+ $saveHandler = $this->storage->getSaveHandler();
should be $save_handler
+++ b/core/lib/Drupal/Core/Session/Session.phpundefined@@ -0,0 +1,234 @@
+ * Note:
+ * Bags can not be directly accessed via protected attributes, and they
+ * don't have either a count() or isEmpty() method.
Sounds like a possible feature request for symfony.
+++ b/core/lib/Drupal/Core/Session/Storage/Handler/DatabaseSessionHandler.phpundefined@@ -0,0 +1,172 @@
+ throw new \RuntimeException(sprintf('DatabaseException was thrown when trying to manipulate session data: %s', $e->getMessage()), 0, $e);
...
+ throw new \RuntimeException(sprintf('DatabaseException was thrown when trying to manipulate session data: %s', $e->getMessage()), 0, $e);
What about "... when trying to delete session data"?
+++ b/core/lib/Drupal/Core/Session/Storage/Handler/MixedModeHandlerInterface.phpundefined@@ -0,0 +1,80 @@
+<?php
+
...
+interface MixedModeHandlerInterface extends \SessionHandlerInterface {
Needs documentation.
#64
fixed 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#65
#66
The last submitted patch, drupal-sf_sessions-1858196-64.patch, failed testing.
#67
#68
The last submitted patch, drupal-sf_sessions-1858196-66.patch, failed testing.
#69
i made upgrade tests not fatal but fixing the rest of simpletest is way above me
#70
Let's see what's left.
#71
The last submitted patch, drupal-sf_sessions-1858196-69.patch, failed testing.
#72
rerolled to see where we stand.
#73
#74
The last submitted patch, 1858196_72.patch, failed testing.
#75
this will fix bot
also now that #1953800: Make the database connection serializable is in i updated the handler
#76
The last submitted patch, drupal-sf_sessions-1858196-75.patch, failed testing.
#77
this empty check supposed to be isset
#78
The last submitted patch, drupal-sf_sessions-1858196-77.patch, failed testing.
#79
I'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.