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

Original report by @pounard

CommentFileSizeAuthor
#193 drupal_1858196_193.patch69.78 KBXano
#187 1858196-187.patch69.2 KBdamiankloip
#175 interdiff.txt711 bytesjoelpittet
#175 1858196-175-session-service.patch68.57 KBjoelpittet
#171 interdiff.txt4.06 KBcosmicdreams
#171 1858196_178.patch68.55 KBcosmicdreams
#164 drupal8.user-system.1858196-164.patch72.57 KBneclimdul
#164 interdiff.txt920 bytesneclimdul
#162 1858196_162.patch70.53 KBcosmicdreams
#162 interdiff_162.txt8.22 KBcosmicdreams
#160 1858196_160.patch70.71 KBcosmicdreams
#160 interdiff.txt4.7 KBcosmicdreams
#150 interdiff.txt890 bytesjoelpittet
#150 1858196-150-session-service.patch72.57 KBjoelpittet
#144 session_1858196_143.patch72.55 KBcosmicdreams
#141 session_1858196_141.patch72.61 KBcosmicdreams
#141 interdiff.txt2.22 KBcosmicdreams
#136 session-1858196-136.patch72.27 KBRainbowArray
#132 session-1858196-132.patch72.26 KBdawehner
#127 session-1858196-127.patch72.41 KBtim.plunkett
#127 interdiff.txt1.89 KBtim.plunkett
#124 session-1858196-124.patch75.07 KBtim.plunkett
#124 interdiff.txt3.66 KBtim.plunkett
#121 1858196-121.patch72.54 KBpfrenssen
#120 1858196-120.patch1.37 KBpfrenssen
#116 1858196-116.patch39.6 KBcosmicdreams
#107 interdiff.txt772 bytesfubhy
#107 1858196-107.patch72.6 KBfubhy
#104 1858196-104.patch72.66 KBfubhy
#104 interdiff.txt6.31 KBfubhy
#100 drupal-sf_sessions-1858196-100.patch74.94 KBLetharion
#98 drupal-sf_sessions-1858196-98.patch74.94 KBLetharion
#89 drupal-sf_sessions-1858196-89.patch74.34 KBLetharion
#86 drupal-sf_sessions-1858196-86.patch74.39 KBParisLiakos
#84 drupal-sf_sessions-1858196-84.patch75.23 KBParisLiakos
#83 drupal-sf_sessions-1858196-83.patch76.23 KBParisLiakos
#80 1858196_80.patch75.18 KBcosmicdreams
#77 drupal-sf_sessions-1858196-77.patch75.11 KBParisLiakos
#75 drupal-sf_sessions-1858196-75.patch75.11 KBParisLiakos
#72 1858196_72.patch74.77 KBcosmicdreams
#69 drupal-sf_sessions-1858196-69.patch74.75 KBParisLiakos
#67 drupal-sf_sessions-1858196-66.patch70.02 KBParisLiakos
#64 drupal-sf_sessions-1858196-64.patch70.02 KBParisLiakos
#61 drupal-sf_sessions-1858196-60.patch69.7 KBParisLiakos
#58 drupal-sf_sessions-1858196-58.patch69.55 KBParisLiakos
#54 drupal-sf_sessions-1858196-54.patch30.34 KBParisLiakos
#46 drupal-1858196-46.patch24.33 KBdawehner
#46 interdiff.txt6.84 KBdawehner
#40 1858196_40.patch20.2 KBcosmicdreams
#36 1858198_35.patch17.9 KBcosmicdreams
#35 1858196_34.patch18.67 KBcosmicdreams
#34 drupal-1858196-25.patch18.93 KBcosmicdreams
#25 drupal-1858196-25.patch18.93 KBneclimdul
#18 drupal-1858196-18.patch18.88 KBneclimdul
#16 drupal-1858196-16.patch18.88 KBdawehner
#13 interdiff.txt908 bytesdawehner
#13 drupal-1858196-13.patch18.83 KBdawehner
#5 session-ph53-test-only.patch18.18 KBmarcingy
#5 session-ph53.patch18.7 KBmarcingy
#1 1858196_1.patch17.52 KBcosmicdreams
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cosmicdreams’s picture

Issue summary: View changes

Updated issue summary.

cosmicdreams’s picture

Status: Active » Needs review
FileSize
17.52 KB

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.

Status: Needs review » Needs work

The last submitted patch, 1858196_1.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

Crell’s picture

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

:-(

marcingy’s picture

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.

marcingy’s picture

Status: Needs work » Needs review
FileSize
18.7 KB
18.18 KB

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.

cosmicdreams’s picture

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

cosmicdreams’s picture

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');
}
marcingy’s picture

$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',
Crell’s picture

#5: session-ph53.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, session-ph53.patch, failed testing.

neclimdul’s picture

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.

neclimdul’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.83 KB
908 bytes

What about just using that?

marcingy’s picture

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.

Status: Needs review » Needs work

The last submitted patch, drupal-1858196-13.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
18.88 KB

Ah I see.

Status: Needs review » Needs work

The last submitted patch, drupal-1858196-16.patch, failed testing.

neclimdul’s picture

FileSize
18.88 KB

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.

neclimdul’s picture

Status: Needs work » Needs review

run tests

marcingy’s picture

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.

Status: Needs review » Needs work

The last submitted patch, drupal-1858196-18.patch, failed testing.

aspilicious’s picture

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?

neclimdul’s picture

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.

    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/stubs

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.

marcingy’s picture

They have a docs issue then as the readme states :(

If you are using PHP 5.3.x you must add the following to your autoloader:

// SessionHandlerInterface
if (!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?

neclimdul’s picture

Status: Needs work » Needs review
FileSize
18.93 KB

oh... "settings autoload to FALSE"... copy paste fail...
lets see how the testbot likes this.

neclimdul’s picture

So there we go. passing patch! Don't know what the plan was from here but we've got an implementation.

cosmicdreams’s picture

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.

marcingy’s picture

Status: Needs review » Reviewed & tested by the community
marcingy’s picture

Issue summary: View changes

Updated issue summary.

pounard’s picture

Plan is to try to restore the rest of the patch I guess :) But due to all recent changes this won't be easy.

cosmicdreams’s picture

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.

neclimdul’s picture

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

sun’s picture

Component: other » user system
Category: feature » task
Status: Reviewed & tested by the community » Needs review
Issue tags: +session, +symfony

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

cosmicdreams’s picture

Thanks for the review sun. I'll work to improve the comments and other uncommitables.

cosmicdreams’s picture

FileSize
18.93 KB

This fixes many typos, cleans up the comments some, and adds a test to the testDICSession() test case.

cosmicdreams’s picture

FileSize
18.67 KB

Forget 34, that was the wrong patch. This one is much better.

cosmicdreams’s picture

FileSize
17.9 KB

going to try this again. if the patch's file name isn't 1858196_35.patch then it's the wrong one.

chx’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Session/Handler/DatabaseSessionHandler.phpundefined
@@ -0,0 +1,87 @@
+    catch (\PDOException $e) {

We have DatabaseExceptionWrapper for 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.

pounard’s picture

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.

Crell’s picture

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

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
20.2 KB

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.

Status: Needs review » Needs work

The last submitted patch, 1858196_40.patch, failed testing.

pounard’s picture

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.

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?

dawehner’s picture

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

cosmicdreams’s picture

dawehner: I don't think so, this comes from Symfony2's FrameworkBundle. Which is rather large to pull in.

cosmicdreams’s picture

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.

dawehner’s picture

FileSize
6.84 KB
24.33 KB

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.

cosmicdreams’s picture

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.

cosmicdreams’s picture

Status: Needs work » Needs review

putting to needs review so we can see how many tests fail.

Status: Needs review » Needs work

The last submitted patch, drupal-1858196-46.patch, failed testing.

ParisLiakos’s picture

Assigned: Unassigned » ParisLiakos

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

Crell’s picture

Issue tags: +WSCCI

Tagging

ParisLiakos’s picture

ParisLiakos’s picture

Assigned: ParisLiakos » Unassigned

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

ParisLiakos’s picture

While waiting for #1929288: Move cryptographic functions to Crypt component

how big hack is DatabaseSessionHandler::updateIdentifiers() ?

ParisLiakos’s picture

Crell’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-sf_sessions-1858196-54.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
69.55 KB

lets make this green

ParisLiakos’s picture

Title: Add Implementation of Symfony Session to DI Container » Turn session.inc into a service leveraging symfony session components

and a title with the scope of the patch i previous posted

Status: Needs review » Needs work

The last submitted patch, drupal-sf_sessions-1858196-58.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
69.7 KB

webtests batch in simpletest is failing, and still havent fixed upgrade tests

Status: Needs review » Needs work

The last submitted patch, drupal-sf_sessions-1858196-60.patch, failed testing.

dawehner’s picture

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.

ParisLiakos’s picture

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

ParisLiakos’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-sf_sessions-1858196-64.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
70.02 KB

Status: Needs review » Needs work

The last submitted patch, drupal-sf_sessions-1858196-66.patch, failed testing.

ParisLiakos’s picture

Assigned: ParisLiakos » Unassigned
FileSize
74.75 KB

i made upgrade tests not fatal but fixing the rest of simpletest is way above me

Crell’s picture

Status: Needs work » Needs review

Let's see what's left.

Status: Needs review » Needs work

The last submitted patch, drupal-sf_sessions-1858196-69.patch, failed testing.

cosmicdreams’s picture

FileSize
74.77 KB

rerolled to see where we stand.

ParisLiakos’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1858196_72.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
75.11 KB

this will fix bot
also now that #1953800: Make the database connection serializable is in i updated the handler

Status: Needs review » Needs work

The last submitted patch, drupal-sf_sessions-1858196-75.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
75.11 KB

this empty check supposed to be isset

Status: Needs review » Needs work

The last submitted patch, drupal-sf_sessions-1858196-77.patch, failed testing.

cosmicdreams’s picture

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.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
75.18 KB

Here's a reroll

Status: Needs review » Needs work

The last submitted patch, 1858196_80.patch, failed testing.

cosmicdreams’s picture

Perhaps this is the course of least resistance for now: http://symfony.com/doc/master/components/http_foundation/session_php_bri...

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
76.23 KB

lets see how this go under php5.3
locally simpletest is not failing anymore:)

ParisLiakos’s picture

sorry, this is the correct patch, but please, dont cancel the previous one, i still want to see how it acts under 5.3

Status: Needs review » Needs work

The last submitted patch, drupal-sf_sessions-1858196-84.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
74.39 KB

meh, conflicts

Status: Needs review » Needs work

The last submitted patch, drupal-sf_sessions-1858196-86.patch, failed testing.

corvus_ch’s picture

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

Letharion’s picture

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

Letharion’s picture

Status: Needs work » Needs review

No actual interdiff uploaded cause interdiff didn't like parsing the patches. :(

Status: Needs review » Needs work

The last submitted patch, drupal-sf_sessions-1858196-89.patch, failed testing.

ParisLiakos’s picture

for 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

ParisLiakos’s picture

Issue summary: View changes

A more verbose issue summary

ParisLiakos’s picture

Issue summary: View changes

add sandbox link

ParisLiakos’s picture

Issue summary: View changes

Updated issue summary

Letharion’s picture

Thanks :) I'll see about the 5.3 issue. :)

Letharion’s picture

Cannot reproduce with 5.3 on my machine. :(

Letharion’s picture

Status: Needs work » Needs review
Issue tags: -session, -WSCCI, -symfony

Status: Needs review » Needs work
Issue tags: +session, +WSCCI, +symfony

The last submitted patch, drupal-sf_sessions-1858196-89.patch, failed testing.

Letharion’s picture

Status: Needs work » Needs review

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

Letharion’s picture

Reroll.

Status: Needs review » Needs work

The last submitted patch, drupal-sf_sessions-1858196-98.patch, failed testing.

Letharion’s picture

Status: Needs work » Needs review
FileSize
74.94 KB

Problem:

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.

Status: Needs review » Needs work

The last submitted patch, drupal-sf_sessions-1858196-100.patch, failed testing.

Berdir’s picture

global $user->uid is protected now, you need to update all instances that directly access it:

+++ b/core/includes/session.incundefined
@@ -305,175 +39,39 @@ function drupal_session_start() {
+  if ($user->id() && REQUEST_TIME - $user->access > settings()->get('session_write_interval', 180)) {

isAuthenticated() is the same as id() in this case but is self-documenting.

+++ b/core/includes/session.incundefined
@@ -305,175 +39,39 @@ function drupal_session_start() {
+      ->condition('uid', $user->uid)

id()

+++ b/core/lib/Drupal/Core/EventSubscriber/SessionListener.phpundefined
@@ -0,0 +1,116 @@
+    // @todo Kill this global.
+    global $user;

$user = $request->attributes->get('account') should work now.

+++ b/core/lib/Drupal/Core/EventSubscriber/SessionListener.phpundefined
@@ -0,0 +1,116 @@
+    if (!empty($user->uid) || !empty($_SESSION)) {

$user->isAuthenticated() instead of the !empty()

+++ b/core/lib/Drupal/Core/Session/Session.phpundefined
@@ -0,0 +1,227 @@
+    if (empty($this->userSession->uid) && $this->isEmpty()) {

isAnonymous() in this case.

+++ b/core/lib/Drupal/Core/Session/Storage/Proxy/CookieProxy.phpundefined
@@ -0,0 +1,437 @@
+    if (!$user_session || !isset($user_session->uid)) {

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.

+++ b/core/lib/Drupal/Core/Session/Storage/Proxy/CookieProxy.phpundefined
@@ -0,0 +1,437 @@
+        'uid' => $user_session->uid,

id()

+++ b/core/tests/Drupal/Tests/Core/Session/Storage/Proxy/CookieProxyTest.phpundefined
@@ -0,0 +1,85 @@
+    $account = $this->getMock('Drupal\Core\Session\UserSession');
+    // The user id needs to be set.
+    $account->uid = rand(0, 10);
+    $this->session->setUserSession($account);

You should mock the id() method instead. And assigning a rand() seems weird? 0 might have completely different behavior than 1?

+++ b/core/update.phpundefined
@@ -438,6 +438,10 @@ function update_check_requirements($skip_warnings = FALSE) {
+// Determine if the current user has access to run update.php.
+require_once __DIR__ . '/includes/session.inc';
+drupal_session_initialize();

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

fubhy’s picture

Assigned: Unassigned » fubhy
fubhy’s picture

Status: Needs work » Needs review
FileSize
6.31 KB
72.66 KB

You should mock the id() method instead. And assigning a rand() seems weird? 0 might have completely different behavior than 1?

I decided to properly set the property instead.

$user = $request->attributes->get('account') should work now.

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.

Status: Needs review » Needs work

The last submitted patch, 1858196-104.patch, failed testing.

Berdir’s picture

+++ b/core/tests/Drupal/Tests/Core/Session/Storage/Proxy/CookieProxyTest.phpundefined
@@ -34,16 +55,17 @@ protected function setUp() {
     $account = $this->getMock('Drupal\Core\Session\UserSession');
-    // The user id needs to be set.
-    $account->uid = rand(0, 10);
+
+    $property = new \ReflectionProperty('Drupal\Core\Session\UserSession', 'uid');
+    $property->setAccessible(TRUE);
+    $property->setValue($account, 10);

Uh, 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?

fubhy’s picture

Assigned: fubhy » Unassigned
Status: Needs work » Needs review
FileSize
72.6 KB
772 bytes

Uh, why would you do that? :)

Umh, no reason, yeah. That was stupid.

Status: Needs review » Needs work

The last submitted patch, 1858196-107.patch, failed testing.

Letharion’s picture

Status: Needs work » Needs review

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

Letharion’s picture

Status: Needs review » Needs work

Nah, still can't reproduce the form error locally. :(

webchick’s picture

Priority: Normal » Major

It's my understanding that this patch is required to remove these lines from drupal_handle_request():

  // @todo Remove this once everything in the bootstrap has been
  //   converted to services in the DIC.
  $kernel->boot();
  drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);

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.

Crell’s picture

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

markhalliwell’s picture

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

pounard’s picture

Symfony session handler when using the native session storage implementation allows to use the $_SESSION[] array directly, which means no BC break.

cosmicdreams’s picture

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

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
39.6 KB

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

Crell’s picture

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

+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/SessionListener.php
@@ -0,0 +1,118 @@
+    if ($user->isAuthenticated()) {
+      // @todo this does not really belong here.
+      drupal_page_is_cacheable(FALSE);
+    }

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

+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/SessionListener.php
@@ -0,0 +1,118 @@
+    $events[KernelEvents::RESPONSE][] = array('setSessionCookies');

Let's give this an explicit weight, for clarity.

+++ b/core/tests/Drupal/Tests/Core/Session/Session.php
@@ -0,0 +1,226 @@
+        trigger_error($e->getMessage());

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.

+++ b/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Session/Storage/Proxy/CookieProxy.php
@@ -0,0 +1,438 @@
+      $rids = db_query("SELECT ur.rid FROM {users_roles} ur WHERE ur.uid = :uid", array(':uid' => $values['uid']))->fetchCol();

The DB should be injectable at this point.

ParisLiakos’s picture

this is passing because the code is dead. it is not actually being used. #107 is 72KB

tim.plunkett’s picture

Issue tags: +Needs reroll

#107 needs a reroll. Not #116.

pfrenssen’s picture

FileSize
1.37 KB

Rerolled #107.

pfrenssen’s picture

FileSize
72.54 KB

This is the right patch.

Status: Needs review » Needs work

The last submitted patch, 1858196-121.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
tim.plunkett’s picture

FileSize
3.66 KB
75.07 KB

CookieProxy tried to use protected properties from UserSession. I moved the setters from UserInterface up to AccountInterface.

Status: Needs review » Needs work

The last submitted patch, session-1858196-124.patch, failed testing.

Berdir’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.89 KB
72.41 KB

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

Status: Needs review » Needs work

The last submitted patch, session-1858196-127.patch, failed testing.

ParisLiakos’s picture

+++ b/core/includes/session.inc
@@ -300,175 +39,39 @@ function drupal_session_start() {
-function drupal_session_started($set = NULL) {
-  static $session_started = FALSE;
-  if (isset($set)) {
-    $session_started = $set;
-  }
-  return $session_started && session_id();
+function drupal_session_started() {
+  return (bool) Drupal::service('session')->getId();

hmm maybe this is the root of all evil for "session already started" simpletest errors

Crell’s picture

Status: Needs work » Needs review

#107: 1858196-107.patch queued for re-testing.

jthorson’s picture

For #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:

[Mon Sep 09 01:37:02 2013] [error] [client 10.20.0.1] PHP Fatal error:  Call to undefined function Drupal\\Core\\Entity\\field_attach_load() in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Entity/DatabaseStorageController.php on line 344, referer: http://drupaltestbot-dev-mysql.osuosl.test/checkout/core/install.php?langcode=en&profile=standard
dawehner’s picture

FileSize
72.26 KB

The problem mentioned in 131 seemed to be fix in core already. This is just a straight rerole.

Status: Needs review » Needs work
Issue tags: -session, -WSCCI, -symfony

The last submitted patch, session-1858196-132.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +session, +WSCCI, +symfony

The last submitted patch, session-1858196-132.patch, failed testing.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
72.27 KB

Here's a reroll of #132 based on suggestions by larowlan.

Status: Needs review » Needs work

The last submitted patch, session-1858196-136.patch, failed testing.

RainbowArray’s picture

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

cosmicdreams’s picture

+++ b/core/includes/session.incundefined
@@ -3,295 +3,34 @@
+  $container = Drupal::getContainer();

Not proper syntax anymore should use \Drupal::

Also, use of \Drupal::getContainer is deprecated.

+++ b/core/includes/session.incundefined
@@ -3,295 +3,34 @@
+  $request = $container->get('request');

We shouldn't get the request this way.

Should use \Drupal::request() instead

+++ b/core/includes/session.incundefined
@@ -3,295 +3,34 @@
+  $session = $container->get('session');

I think this should be \Drupal::service('session') now.

+++ b/core/includes/session.incundefined
@@ -300,175 +39,40 @@ function drupal_session_start() {
+  Drupal::service('session')->save();

see Drupal::service('session') is used here. Why isn't this .inc file consistent?

cosmicdreams’s picture

+++ b/core/includes/session.incundefined
@@ -3,295 +3,34 @@
+    // Initialize session, so that $user global is functioning correctly.

@@ -300,175 +39,40 @@ function drupal_session_start() {
   global $user;

+++ b/core/lib/Drupal/Core/EventSubscriber/SessionListener.phpundefined
@@ -0,0 +1,118 @@
+    global $user;

+++ b/core/lib/Drupal/Core/Session/Session.phpundefined
@@ -0,0 +1,226 @@
+    global $user;

But we aren't using $user as a global anymore right?

cosmicdreams’s picture

FileSize
2.22 KB
72.61 KB

Not sure how to handle the global user stuff but here's a new patch that fixes the out of date service calls.

cosmicdreams’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
72.55 KB

Made my patch wrong

Status: Needs review » Needs work
Issue tags: -session, -WSCCI, -symfony

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

joelpittet’s picture

Status: Needs work » Needs review

#144: session_1858196_143.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +session, +WSCCI, +symfony

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

jthorson’s picture

EDIT: Manually submitting the final 'Site information' installation page takes me to a WSOD.

[Fri Oct 11 22:08:04 2013] [error] [client 10.20.0.1] PHP Fatal error:  Class 'Drupal\\field\\Plugin\\Type\\FieldType\\LegacyFieldTypeDiscoveryDecorator' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Entity/Field/FieldTypePluginManager.php on line 51, referer: http://drupaltestbot1843/checkout/core/install.php?langcode=en&profile=standard

This is what I get when visiting the site after the failed install:

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "field.info" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 875 of /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php). 
cosmicdreams’s picture

Sooooooo yea, we'll need to reroll this. Lot's changed since last time.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
72.57 KB
890 bytes

So this is probably not kosher but I added back in the !drupal_is_cli()...

Status: Needs review » Needs work
Issue tags: -session, -WSCCI, -symfony

The last submitted patch, 1858196-150-session-service.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

#150: 1858196-150-session-service.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +session, +WSCCI, +symfony

The last submitted patch, 1858196-150-session-service.patch, failed testing.

joelpittet’s picture

Well that did work locally with drush :(

cosmicdreams’s picture

Manually 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

cosmicdreams’s picture

Status: Needs work » Needs review
Issue tags: -session, -WSCCI, -symfony

#150: 1858196-150-session-service.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +session, +WSCCI, +symfony

The last submitted patch, 1858196-150-session-service.patch, failed testing.

jthorson’s picture

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

exception \'LogicException\' with message \'Cannot change the ID of an active session\' in /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Session/Storage/Proxy/AbstractProxy.php:123
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Session/Storage/Proxy/CookieProxy.php(85): Symfony\\Component\\HttpFoundation\\Session\\Storage\\Proxy\\AbstractProxy->setId(\'E2cyKVnYYDmeWfC...\')
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Session/Session.php(50): Drupal\\Core\\Session\\Storage\\Proxy\\CookieProxy->init()
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/session.inc(20): Drupal\\Core\\Session\\Session->init()
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc(1801): drupal_session_initialize()
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc(710): install_bootstrap_full(Array)
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc(563): install_run_task(Array, Array)
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc(94): install_run_tasks(Array)
#7 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php(785): install_drupal(Array)
#8 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorTestBase.php(27): Drupal\\simpletest\\WebTestBase->setUp()
#9 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(766): Drupal\\aggregator\\Tests\\AggregatorTestBase->setUp()
#10 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(489): Drupal\\simpletest\\TestBase->run()
#11 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(26): simpletest_script_run_one_test(\'5\', \'Drupal\\aggregat...\')
#12 {main}
Fatal error: Uncaught exception \'PDOException\' with message \'SQLSTATE[HY000]: General error: user-supplied statement does not accept constructor arguments\' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php:336
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php(336): PDO->prepare(\'SELECT 1 AS exp...\')
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php(536): Drupal\\Core\\Database\\Connection->prepareQuery(\'SELECT 1 AS exp...\')
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Query/Select.php(421): Drupal\\Core\\Database\\Connection->query(\'SELECT 1 AS exp...\', Array, Array)
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Query/Merge.php(418): Drupal\\Core\\Database\\Query\\Select->execute()
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Session/Storage/Handler/DatabaseSessio in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Session/Storage/Handler/DatabaseSessionHandler.php on line 139
cosmicdreams’s picture

+++ b/core/lib/Drupal/Core/Session/Storage/Proxy/CookieProxyInterface.phpundefined
--- a/core/modules/system/lib/Drupal/system/Tests/Session/SessionTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Session/SessionTest.phpundefined

+++ b/core/tests/Drupal/Tests/Core/Session/SessionTest.phpundefined
@@ -0,0 +1,97 @@
+ * Contains \Drupal\Tests\Core\Session\SessionTest.

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

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
4.7 KB
70.71 KB

Status: Needs review » Needs work

The last submitted patch, 1858196_160.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
8.22 KB
70.53 KB

Namespace was wrong with the new SessionListener which made it fail to register properly.

Status: Needs review » Needs work

The last submitted patch, 1858196_162.patch, failed testing.

neclimdul’s picture

Trying this, diffed off of #150. Installed cleanly local and fixed a warning I was getting on the command line.

neclimdul’s picture

Status: Needs work » Needs review

run tests to see.

Status: Needs review » Needs work

The last submitted patch, drupal8.user-system.1858196-164.patch, failed testing.

neclimdul’s picture

So this is what I found looking into the install problem on the test bot.

In install_configure_form_validate() we call user_validate_name(). user_validate_name() calls

  $data = \Drupal::typedData()->create(array(
    'type' => 'string',
    'constraints' => array('UserName' => array()),
  ));

At this point typedData goes nuts and long story short we end up in FieldTypePluginManager::__construct() where we try to instantiate use 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.

neclimdul’s picture

Issue summary: View changes

Make the sandbox url a link

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 164: drupal8.user-system.1858196-164.patch, failed testing.

Xano’s picture

cosmicdreams’s picture

Issue summary: View changes
FileSize
68.55 KB
4.06 KB

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

neclimdul’s picture

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

+++ b/core/includes/session.inc
@@ -300,175 +39,40 @@ function drupal_session_start() {
+  if ($user->isAuthenticated() && REQUEST_TIME - $user->access > settings()->get('session_write_interval', 180)) {

$user->access causes a notice. i think its suppose to be $user->getLastAccessedTime()

neclimdul’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 171: 1858196_178.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
68.57 KB
711 bytes

@neclimdul I think you're right, here's the patch.

Status: Needs review » Needs work

The last submitted patch, 175: 1858196-175-session-service.patch, failed testing.

cosmicdreams’s picture

Issue summary: View changes

Updated issue summary with the issue summary template, tried to explain the purpose of this patch better. More changes to come when I have time.

cosmicdreams’s picture

Issue summary: View changes
cosmicdreams’s picture

Issue summary: View changes
cosmicdreams’s picture

cosmicdreams’s picture

Issue summary: View changes
cosmicdreams’s picture

Issue summary: View changes
cosmicdreams’s picture

Issue summary: View changes
cosmicdreams’s picture

Issue summary: View changes
joelpittet’s picture

Issue tags: +Needs reroll

This needs a re-roll, the changes look a bit rich for my blood.

joelpittet’s picture

+++ b/core/includes/bootstrap.inc
@@ -2767,6 +2767,12 @@ function drupal_classloader($class_loader = NULL) {
+
+    // Fallback to symfony SessionHandlerInterface for PHP < 5.4.0.
+    if (!interface_exists('SessionHandlerInterface', FALSE)) {
+      $loader->add('SessionHandlerInterface', DRUPAL_ROOT . '/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Resources/stubs');
+    }
+

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

damiankloip’s picture

Issue tags: -Needs reroll
FileSize
69.2 KB

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

damiankloip’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 187: 1858196-187.patch, failed testing.

The last submitted patch, 187: 1858196-187.patch, failed testing.

The last submitted patch, 187: 1858196-187.patch, failed testing.

jthorson’s picture

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

Xano’s picture

Status: Needs work » Needs review
FileSize
69.78 KB

Status: Needs review » Needs work

The last submitted patch, 193: drupal_1858196_193.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

Drupal installation worked locally I think this patch is ready for reviews.

Xano’s picture

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

ParisLiakos’s picture

the 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

joelpittet’s picture

Don'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

joelpittet’s picture

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

ParisLiakos’s picture

Status: Needs review » Postponed

well, we need to do #2187431: Remove current_user service, retrieve it from the authentication manager
otherwise this is impossible unless we do everything ContainerAware

Xano’s picture

Status: Postponed » Needs work

The last submitted patch, 193: drupal_1858196_193.patch, failed testing.

cosmicdreams’s picture

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

cosmicdreams’s picture

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

andypost’s picture

Issue tags: +Needs architectural review, +Needs issue summary update

The 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 in settings.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.

  1. +++ b/core/includes/session.inc
    @@ -3,276 +3,21 @@
     function drupal_session_initialize() {
    ...
    +  if (!$request->hasSession()) {
    +    $request->setSession($session);
    +    \Drupal::service('session.storage.proxy')->setRequest($request);
    +    // Initialize session, so that $user global is functioning correctly.
    +    $session->init();
    
    +++ b/core/lib/Drupal/Core/EventSubscriber/SessionListener.php
    @@ -0,0 +1,116 @@
    +  public function initSession(GetResponseEvent $event) {
    ...
    +    // @todo Kill this global.
    +    global $user;
    ...
    +    // Set the session service to the current request.
    +    $request->setSession($this->session);
    +    // And initialize it.
    +    $this->session->init();
    ...
    +    if ($user->isAuthenticated()) {
    +      // @todo this does not really belong here.
    +      drupal_page_is_cacheable(FALSE);
    
    +++ b/core/lib/Drupal/Core/Session/Session.php
    @@ -0,0 +1,226 @@
    +  public function init() {
    +    $this->setUserSessionFromGlobal();
    ...
    +  public function setUserSessionFromGlobal() {
    +    global $user;
    +    $this->userSession = &$user;
    +  }
    

    currentUser() service already could be used.

    Anyway there's too much references to global user added but init() is called in different contexts

  2. +++ b/core/lib/Drupal/Core/Session/Storage/Proxy/CookieProxy.php
    @@ -0,0 +1,434 @@
    +  public function setActive($flag) {
    

    no doc block

cosmicdreams’s picture

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

sun’s picture

Status: Needs work » Postponed
Issue tags: -Needs architectural review, -Needs issue summary update

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

  1. #2205295: Objectify session handler
  2. Turn 1) into a service.
  3. Refactor bootstrap, tests, and edge-case stuff like the installer, update.php, and authorize.php to utilize it.

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.

Crell’s picture

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

sun’s picture

Title: Turn session.inc into a service leveraging symfony session components » Leverage Symfony Session components
Status: Postponed » Active

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

Drupal\Core\Session\SessionManager implements Drupal\Core\Session\SessionManagerInterface
Drupal\Core\Session\SessionHandler implements \SessionHandlerInterface

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

cpj’s picture

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

znerol’s picture

I 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 the request and gradually remove the Drupal custom session management stuff.

znerol’s picture

sun’s picture

Title: Leverage Symfony Session components » [meta] Leverage Symfony Session components

Briefly discussed with @znerol — let's move forward with 1-2+ child issues to complete this effort, inherently turning this issue into a meta issue.

cosmicdreams’s picture

Issue summary: View changes
znerol’s picture

cosmicdreams’s picture

Issue summary: View changes

added

znerol’s picture

Issue summary: View changes
YesCT’s picture

Issue summary: View changes

#2286591: [meta] Security audit the Authentication component is postponed on this. updated issue summary with that info.

znerol’s picture

Issue summary: View changes

Adding another piece to the puzzle #2338571: Remove SessionManager::startLazy().

znerol’s picture

znerol’s picture

Also #2342593: Remove mixed SSL support from core could streamline further development.

Xano’s picture

Seeing as SessionManager now uses the Symfony session handling components, is this issue still necessary?

znerol’s picture

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

znerol’s picture

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

znerol’s picture

jibran’s picture

Would anyone be interested in creating a workflow diagram for session api? I can create a separate issue for that.

YesCT’s picture

@znerol (or other) is this the issue that #2286591: [meta] Security audit the Authentication component is postponed on?

znerol’s picture

almaudoh’s picture

skipyT’s picture

Issue summary: View changes
almaudoh’s picture

Issue summary: View changes
znerol’s picture

Status: Active » Closed (duplicate)