Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[2.1][HttpFoundation] Refactor session handling and flash messages #2853

Merged
merged 31 commits into from

16 participants

Drak Marijn Huizendveld Lukas Kahwe Smith Andrew Berry Mark Mateo Tibaquirá Palacios Daniel Anderson Tiecher Tobias Schultze Florin Patan Fabien Potencier Christophe Coevoet Joseph Bielawski Bernhard Schussek Deni Vladimir Kartaviy Jordi Boggiano
Drak

Bug fix: yes
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #2607, #2591, #2717, #2773
References the following tickets: #2592, #2543, #2541, #2510, #2714, #2684
Todo: -

Introduction

This extensive PR is a refactor with minimal BC breaks of the [HttpFoundation] component's session management which fixes several issues in the current implementation. This PR includes all necessary changes to other bundles and components is documented in the CHANGELOG-2.1 and UPGRADING-2.1.

Summary of Changes

Session:

  • Session object now implements SessionInterface

Attributes:

  • Attributes now handled by AttributeBagInterface
  • Added two AttributeBag implementations: AttributeBag replicates the current Symfony2 attributes behaviour, and the second, NamespacedAttributeBag introduces structured namespaced representation using '/' in the key. Both are BC. FrameworkBundle defaults to the old behaviour.

Flash messages:

  • Flash messages now handled by FlashBagInterface
  • Introduced FlashBag which changes the way flash messages expire, they now expire on use rather than automatically, useful for ESI.
  • Introduced AutoExpireFlashBag (default) which replicates the old automatic expiry behaviour of flash messages.

Session Storage:

  • Introduced a base object, AbstractSessionStorage for session storage drivers
  • Introduced a SessionSaveHandlerInterface when using custom session save handlers
  • Introduced a NullSessionStorage driver which allows for unsaved sessions
  • Introduced new session storage drivers for Memcache and Memcached
  • Introduced new session storage drivers for PHP's native SQLite, Memcache and Memcached support

General:

  • Fixed bugs where session attributes are not saved and all cases where flash messages would get lost
  • Wrote new tests, refactored related existing tests and increased test coverage extensively.

Rationale/Details

I'll explain more detail in the following sections.

Unit Tests

All unit and functional tests pass.

Note on Functional Testing

I've introduced MockFileSessionStorage which replaces FilesystemSessionStorage to emulate a PHP session for functional testing. Essentially the same methodology of functional testing has been maintained but without interrupting the other session storage drivers interaction with real PHP sessions. The service is now called session.storage.mock_file.

Session Workflow

PHP sessions follow a specific workflow which is not being followed by the current session management implementation and is responsible for some unpredictable bugs and behaviours.

Basically, PHP session workflow is as follows: open, read, write, close. In between these can occur, destroy and garbage collection. These actions are handled by session save handlers and one is always registered in all cases. By default, the files save handler (internally to PHP) is registered by PHP at the start of code execution.

PHP offers the possibility to change the save handler to another internal type, for example one provided by a PHP extension (think SQLite, Memcache etc), or you can register a user type and set your own handlers. However in all cases PHP requires the handlers.

The handlers are called when the following things occur:

  • open and read when session_start() or the session autostarts when PHP outputs some display
  • destroy when session_regenerate_id(true) is called
  • write and close when PHP shuts down or when session_write_close() is called
  • garbage collection is called randomly according to configurable probability

The next very important aspect of this PR is that $_SESSION plays an important part in this workflow because the contents of the $SESSION is populated __after_ the read handler returns the previously saved serialised session data. The write handler is sent the serialised $_SESSION contents for save. Please note the serialisation is a different format to serialize().

For this reason, any session implementation cannot get rid of using $_SESSION.

I wrote more details on this issue here

In order to make writing session storage drivers simple, I created a light base class AbstractSessionStorage and the SessionSaveHandlerInterface which allows you to quickly write native and custom save handler drivers.

Flash Messages [BC BREAK]

Flash messages currently allow representation of a single message per $name. Fabien designed the original system so that $name was equivalent to flash message type. The current PR changes the fact that Flash messages expire explicitly when retrieved for display to the user as opposed to immediately on the next page load.

The last issue fixes potential cases when flash messages are lost due to an unexpected intervening page-load (an error for example). The API get() has a flag which allows you to override the clear() action.

Flash message translation

This PR does not cover translation of flash messages because messages should be translated before calling the flash message API. This is because flash messages are used to present messages to the user after a specific action, and in any case, immediately on the next page load. Since we know the locale of the request in every case we can translate the message before storing. Secondly, translation is simply a string manipulation. Translation API calls should always have the raw untranslated string present because it allows for extraction of translation catalogs. For a complete answer see my answer here

Session attribute and structured namespacing

This has been implemented without changing the current default behaviour but details are below for the alternative:

Attributes are currently stored in a flat array which limits the potential of session attributes:

Here are some examples to see why this 'structured namespace' methodology is extremely convenient over using a flat system. Let's look at an example with csrf tokens. Let's say we have multiple csrftokens stored by form ID (allowing multiple forms on the page and tabbed browsing).

If we're using a flat system, you might have

'tokens' => array('a' => 'a6c1e0b6',
                  'b' => 'f4a7b1f3')

With a flat system when you get the key tokens, you will get back an array, so now you have to analyse the array. So if you simply want to add another token, you have to follow three steps: get the session attribute tokens, have to add to the array, and lastly set the entire array back to the session.

$tokens = $session->get('tokens');
$tokens['c'] = $value;
$session->set('tokens', $tokens);

Doable, but you can see it's pretty long winded.

With structured namespacing you can simply do:

$session->set('c', $value, '/tokens');

There are several ways to implement this, either with an additional $namespace argument, or by treating a character in the $key as a namespacer. NamespacedAttributeBag treats / as a namespacer so you can represent user.tokens/a for example. The namespace character is configurable in NamespacedAttributeBag.

UPGRADE-2.1.md
((23 lines not shown))
+.. note::
+
+ You can of course declare `<?php use Symfony\Component\HttpFoundation\FlashBag; ?>` at the beginning
+ of the PHP template so you can use the shortcut `FlashBag::NOTICE`.
+
+ Before (Twig):
+
+ {% if app.session.hasFlash('notice') %}
+ <div class="flash-notice">
+ {{ app.session.flash('notice') }}
+ </div>
+ {% endif %}
+
+ After (Twig):
+
+ {% for flashMessage in app.session.getFlashes(constant(Symfony\Component\HttpFoundation\FlashBag::NOTICE)) %}
Christophe Coevoet Collaborator
stof added a note

missing quotes for the argument of constant

Drak
drak added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Symfony/Component/HttpFoundation/SessionInterface.php
((12 lines not shown))
+namespace Symfony\Component\HttpFoundation;
+
+use Symfony\Component\HttpFoundation\SessionStorage\AttributeInterface;
+use Symfony\Component\HttpFoundation\FlashBagInterface;
+
+/**
+ * Interface for the session.
+ *
+ * @author Drak <drak@zikula.org>
+ */
+interface SessionInterface extends AttributeInterface, \Serializable
+{
+ /**
+ * Starts the session storage.
+ *
+ * @throws \RutimeException If session fails to start.
Tobias Schultze Collaborator
Tobion added a note

typo

Drak
drak added a note

Sorry, I don't immediately see the typo

Tobias Schultze Collaborator
Tobion added a note

\RuNtimeException

Drak
drak added a note

Thanks, fixed. Will come in next push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...dation/SessionStorage/SessionSaveHandlerInterface.php
((70 lines not shown))
+ *
+ * @return boolean
+ */
+ function sessionClose();
+
+ /**
+ * Read session.
+ *
+ * This method is for internal use by PHP and must not be called manually.
+ *
+ * This method is called by PHP itself when the session is started.
+ * This method should retrieve the session data from storage by the
+ * ID provided by PHP. Return the string directly as is from storage.
+ * If the record was not found you must return an empty string.
+ *
+ * The returned data will be automatically unserialized by PHP using a
Tobias Schultze Collaborator
Tobion added a note

typo: unserialized automatically

Lukas Kahwe Smith Collaborator

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...dation/SessionStorage/SessionSaveHandlerInterface.php
((24 lines not shown))
+ * by session management.
+ *
+ * PHP requires session save handlers. There are some defaults set automatically
+ * when PHP starts, but these can be overriden using this command if you need anything
+ * other than PHP's default handling.
+ *
+ * When the session starts, PHP will call the sessionRead() handler which should return a string
+ * extactly as stored (which will have been encoded by PHP using a special session serializer
+ * session_decode() which is different to the serialize() function. PHP will then populate these
+ * into $_SESSION.
+ *
+ * When PHP shuts down, the sessionWrite() handler is called and will pass the $_SESSION contents
+ * to be stored. Again PHP will automatically serialize these itself using session_encode()
+ *
+ * When a session is specifically destroyed, PHP will call the sessionDestroy() handler with the
+ * session ID. This happens when the session is regenerated for example and th handler
Tobias Schultze Collaborator
Tobion added a note

typo: the
Why did you use two spaces between sentences several times? Here and in lines 26, 32, 36...

Drak
drak added a note

That's a force of habit (typewriter days+UK conventions), I'm old school. Anyhow I realise it's not the convention here, I'll change it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
CHANGELOG-2.1.md
@@ -148,6 +153,32 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
* removed the ContentTypeMimeTypeGuesser class as it is deprecated and never used on PHP 5.3
* added ResponseHeaderBag::makeDisposition() (implements RFC 6266)
* made mimetype to extension conversion configurable
+ * Flashes are now stored as a bucket of messages per `$type` so there can be multiple messages per type.
+ There are four interface constants for type, `FlashBagInterface::INFO`, `FlashBagInterface::NOTICE`,
+ `FlashBagInterface::WARNING` and `FlashBagInterface::ERROR`.
+ * Added `FlashBag` (default). Flashes expire when retrieved by `popFlashes()`.
+ This makes th implementation ESI compatible.
Lukas Kahwe Smith Collaborator

s/th/the

Drak
drak added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
UPGRADE-2.1.md
@@ -40,3 +44,67 @@ UPGRADE FROM 2.0 to 2.1
Before: $session->getLocale()
After: $request->getLocale()
+
+* Flash Messages now returns and array based on type
+
+ Before (PHP):
+
+ <?php if ($view['session']->hasFlash('notice')): ?>
+ <div class="flash-notice">
+ <?php echo $view['session']->getFlash('notice') ?>
+ </div>
+ <?php endif; ?>
+
+ After (PHP):
+
+ <?php foreach ($view['session']->popFlashes(Symfony\Component\HttpFoundation\FlashBag::NOTICE) as $notice): ?>
Lukas Kahwe Smith Collaborator

imho inside templates its makes more sense to use the types as strings, rather than the constants

Drak
drak added a note

Changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ny/Component/HttpFoundation/AttributeBagInterface.php
((7 lines not shown))
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpFoundation;
+
+use Symfony\Component\HttpFoundation\AttributeBagInterface;
+use Symfony\Component\HttpFoundation\SessionStorage\AttributeInterface;
+
+/**
+ * Attributes store.
+ *
+ * @author Drak <drak@zikula.org>
+ *
+ * @api
Lukas Kahwe Smith Collaborator

@api annotation is premature here

Drak
drak added a note

Removed, I missed this one, I previously removed them all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...mfony/Component/HttpFoundation/AutoExpireFlashBag.php
((38 lines not shown))
+ * @param type $storageKey The key used to store flashes in the session.
+ */
+ public function __construct($storageKey = '_sf2_flashes')
+ {
+ $this->storageKey = $storageKey;
+ $this->flashes = array('display' => array(), 'new' => array());
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function initialize(array &$flashes)
+ {
+ $this->flashes = &$flashes;
+
+ // The logic: messgaes from the last request will be stored in new, so we move them to previous
Lukas Kahwe Smith Collaborator

s/messgaes/messages

Drak
drak added a note

changed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...y/Component/HttpFoundation/NamespacedAttributeBag.php
((12 lines not shown))
+namespace Symfony\Component\HttpFoundation;
+
+/**
+ * This class provides structured storage of session attributes using
+ * a name spacing character in the key.
+ *
+ * @author Drak <drak@zikula.org>
+ */
+class NamespacedAttributeBag extends AttributeBag
+{
+ /**
+ * Namespace character.
+ *
+ * @var string
+ */
+ private $namespaceCharacter;
Lukas Kahwe Smith Collaborator

i don't know if we have standards for this .. maybe namespaceChar is enough? @stof?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...pFoundation/SessionStorage/FileMockSessionStorage.php
((57 lines not shown))
+
+ $this->savePath = $savePath;
+
+ parent::__construct($attributes, $flashes, $options);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function start()
+ {
+ if ($this->started) {
+ return true;
+ }
+
+ if (!ini_get('session.use_cookies') && isset($this->options['id']) && $this->options['id'] && $this->options['id'] != session_id()) {
Lukas Kahwe Smith Collaborator

probably would be good to rewrite this as:

if (!ini_get('session.use_cookies')
    && isset($this->options['id'])
    && $this->options['id']
    && $this->options['id'] != session_id()
) {

@stof is that the correct formatting?

Christophe Coevoet Collaborator
stof added a note

there is no length limit in the CS so this is not mandatory.

Regarding the splitting on multiple line, I don't remember if the operator should be at the beginning or the end of the line. @fabpot ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...pFoundation/SessionStorage/MemcacheSessionStorage.php
((47 lines not shown))
+ *
+ * @param \Memcache $memcache A \Memcache instance
+ * @param array $memcacheOptions An associative array of Memcachge options
+ * @param array $options Session configuration options.
+ * @param AttributeBagInterface $attributes An AttributeBagInterface instance, (defaults null for default AttributeBag)
+ * @param FlashBagInterface $flashes A FlashBagInterface instance (defaults null for default FlashBag)
+ *
+ * @see AbstractSessionStorage::__construct()
+ */
+ public function __construct(\Memcache $memcache, array $memcacheOptions = array(), array $options = array(), AttributeBagInterface $attributes = null, FlashBagInterface $flashes = null)
+ {
+ $this->memcache = $memcache;
+
+ // defaults
+ if (!isset($memcacheOptions['serverpool'])) {
+ $memcacheOptions['serverpool'] = array('host' => '127.0.0.1', 'port' => 11211, 'timeout' => 1, 'persistent' => false, 'weight' => 1);
Lukas Kahwe Smith Collaborator

here again it would help readability to split this across multiple lines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...Foundation/SessionStorage/MemcachedSessionStorage.php
((40 lines not shown))
+ *
+ * @param \Memcached $memcached A \Memcached instance
+ * @param array $memcachedOptions An associative array of Memcached options
+ * @param array $options Session configuration options.
+ * @param AttributeBagInterface $attributes An AttributeBagInterface instance, (defaults null for default AttributeBag)
+ * @param FlashBagInterface $flashes A FlashBagInterface instance (defaults null for default FlashBag)
+ *
+ * @see AbstractSessionStorage::__construct()
+ */
+ public function __construct(\Memcached $memcache, array $memcachedOptions = array(), array $options = array(), AttributeBagInterface $attributes = null, FlashBagInterface $flashes = null)
+ {
+ $this->memcached = $memcached;
+
+ // defaults
+ if (!isset($memcachedOptions['serverpool'])) {
+ $memcachedOptions['serverpool'] = array('host' => '127.0.0.1', 'port' => 11211, 'timeout' => 1, 'persistent' => false, 'weight' => 1);
Lukas Kahwe Smith Collaborator

again split across multiple lines please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
.../HttpFoundation/SessionStorage/NullSessionStorage.php
((22 lines not shown))
+ * @author Drak <drak@zikula.org>
+ *
+ * @api
+ */
+class NullSessionStorage extends AbstractSessionStorage implements SessionSaveHandlerInterface
+{
+ /**
+ * Constructor.
+ *
+ * @param array $options Session configuration options.
+ * @param AttributeBagInterface $attributes An AttributeBagInterface instance, (defaults null for default AttributeBag)
+ * @param FlashBagInterface $flashes A FlashBagInterface instance (defaults null for default FlashBag)
+ *
+ * @see AbstractSessionStorage::__construct()
+ */
+ public function __construct(array $options = array(), AttributeBagInterface $attributes = null, FlashBagInterface $flashes = null)
Lukas Kahwe Smith Collaborator

whats the purpose of this constructor?

Drak
drak added a note

Consistency with the other driver constructors.

Lukas Kahwe Smith Collaborator

but from my reading its redundant .. since it doesn't change any parent behavior.

Christophe Coevoet Collaborator
stof added a note

it should be removed as it does not change the parent constructor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...s/Component/HttpFoundation/AutoExpireFlashBagTest.php
((88 lines not shown))
+ {
+ $this->assertEquals(array(FlashBag::NOTICE), $this->bag->getTypes());
+ }
+
+ public function testAll()
+ {
+ $array = array('new' => array(
+ FlashBag::NOTICE => array('Foo'),
+ FlashBag::ERROR => array('Bar')));
+
+ $this->bag->initialize($array);
+ $this->assertEquals(array(
+ FlashBag::NOTICE => array('Foo'),
+ FlashBag::ERROR => array('Bar')),
+ $this->bag->all()
+ );
Lukas Kahwe Smith Collaborator

intending seems off here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...s/Component/HttpFoundation/AutoExpireFlashBagTest.php
((104 lines not shown))
+ }
+
+ public function testPop()
+ {
+ $this->assertEquals(array('A previous flash message'), $this->bag->pop(FlashBag::NOTICE));
+ $this->assertEquals(array(), $this->bag->pop(FlashBag::NOTICE));
+ $this->assertEquals(array(), $this->bag->pop('non_existing_type'));
+ }
+
+ public function testPopAll()
+ {
+ $this->bag->set(FlashBag::NOTICE, array('Foo'));
+ $this->bag->set(FlashBag::ERROR, array('Bar'));
+ $this->assertEquals(array(
+ FlashBag::NOTICE => array('A previous flash message'),
+ ), $this->bag->popAll());
Lukas Kahwe Smith Collaborator

the ); should be on the next line without any intention

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...s/Component/HttpFoundation/AutoExpireFlashBagTest.php
((124 lines not shown))
+ public function testClear()
+ {
+ $this->assertTrue($this->bag->has(FlashBag::NOTICE));
+ $this->assertEquals(array('A previous flash message'), $this->bag->clear(FlashBag::NOTICE));
+ $this->assertEquals(array(), $this->bag->clear(FlashBag::NOTICE));
+ $this->assertFalse($this->bag->has(FlashBag::NOTICE));
+ }
+
+ public function testClearAll()
+ {
+ $this->assertTrue($this->bag->has(FlashBag::NOTICE));
+ $this->bag->add('Smile, it might work next time', FlashBag::ERROR);
+ $this->assertFalse($this->bag->has(FlashBag::ERROR));
+ $this->assertEquals(array(
+ FlashBag::NOTICE => array('A previous flash message'),
+ ), $this->bag->clearAll());
Lukas Kahwe Smith Collaborator

here again ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...mfony/Tests/Component/HttpFoundation/FlashBagTest.php
((72 lines not shown))
+ public function testPop()
+ {
+ $this->assertEquals(array('A previous flash message'), $this->bag->pop(FlashBag::NOTICE));
+ $this->assertEquals(array(), $this->bag->pop(FlashBag::NOTICE));
+ $this->assertEquals(array(), $this->bag->pop('non_existing_type'));
+ }
+
+ public function testPopAll()
+ {
+ $this->bag->set(FlashBag::NOTICE, array('Foo'));
+ $this->bag->set(FlashBag::ERROR, array('Bar'));
+ $this->assertEquals(array(
+ FlashBag::NOTICE => array('Foo'),
+ FlashBag::ERROR => array('Bar')),
+ $this->bag->popAll()
+ );
Lukas Kahwe Smith Collaborator

intention seems off and in a few more places in this file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ndation/SessionStorage/AbstractSessionStorageTest.php
((8 lines not shown))
+use Symfony\Component\HttpFoundation\SessionStorage\SessionSaveHandlerInterface;
+
+/**
+ * Turn AbstractSessionStorage into something concrete because
+ * certain mocking features are broken in PHPUnit 3.6.4
+ */
+class ConcreteSessionStorage extends AbstractSessionStorage
+{
+}
+
+class CustomHandlerSessionStorage extends AbstractSessionStorage implements SessionSaveHandlerInterface
+{
+ public function sessionOpen($path, $id)
+ {
+ }
+ public function sessionClose()
Lukas Kahwe Smith Collaborator

please add an empty line between methods

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...ymfony/Tests/Component/HttpFoundation/SessionTest.php
((151 lines not shown))
{
- $this->storage->write('foo', 'bar');
+ $this->assertEquals(array(), $this->session->getAllFlashes());
+
+ $this->session->addFlash('a', FlashBag::NOTICE);
+ $this->assertEquals(array(
+ FlashBag::NOTICE => array('a')
+ ), $this->session->getAllFlashes());
Lukas Kahwe Smith Collaborator

again the closing ); should imho be on the next line .. but maybe @stof needs to confirm this before you change this.

Christophe Coevoet Collaborator
stof added a note

it would indeed be better. And I would format it a bit differently:

<?php
$this->assertEquals(
    array(FlashBag::NOTICE => array('a')),
    $this->session->getAllFlashes()
);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Marijn Huizendveld

I haven't read the code yet but the description from this PR and your line of thought seem very well structured.
Seems like a big +1 for me.

...Foundation/SessionStorage/MemcachedSessionStorage.php
((94 lines not shown))
+ /**
+ * {@inheritdoc}
+ */
+ public function sessionRead($sessionId)
+ {
+ $result = $this->memcached->get($this->prefix.$sessionId);
+
+ return $result ? $result : '';
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function sessionWrite($sessionId, $data)
+ {
+ return $this->memcached->set($this->prefix.$sessionId, $data, false, $this->memcachedOptions['expiretime']);

What about concurrency writes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Lukas Kahwe Smith
Collaborator

@deviantintegral could you look over this to see if it really addresses everything you wanted with PR #2510 ?

Andrew Berry

I've read through the documentation and upgrade notes, and I can't see anything that's obviously missing from #2510. Being able to support multiple flashes per type is the most important, and the API looks reasonable to me. Drupal does support supressing repeat messages, but that can easily be implemented in our code unless there's a compelling case for it to be a part of Symfony.

I wonder if PHP memcache support is required in Symfony given the availability of memcached. I'm not familiar with how other parts of Symfony handle it, but there is often quite a bit of confusion between the two PHP extensions. It could be simpler to remove one, or add a bit of info describing or linking to why there are two nearly identical classes.

Is it possible to make one class inherit from the other (memcached is a child of memcache)?

Mark

Interesting, maybe add: session events as I did with the current impl: https://github.com/Fristi/SessionBundle

Drak

@deviantintegral - I agree about the confusion between memcache and memcached but actually, it is necessary to support both because memcached is not available everywhere. For example on Debian Lenny and RHEL/CentOS 5, only memcache is available by default. This would preclude a massive amount of shared hosting environments. Also, it is not possible to inherit one from the other, they are completely different drivers.

@Fristi - I also thought about the events, but they do not belong as part of the standalone component as this would create a coupling to the event dispatcher. The way you have done it, ie, in a bundle is the right way to achieve it.

Mateo Tibaquirá Palacios

Impressive work, looks like a big improvement and deserves a big +1

Daniel Anderson Tiecher

Took some time to grok all the changes in this PR but all in all it is a keeper. Specially the new flash message API, it's really nicer to work with it then the previous one.

Nicely done @drak!

...y/Component/HttpFoundation/NamespacedAttributeBag.php
((45 lines not shown))
+ {
+ $attributes = $this->resolveAttributePath($name);
+ $name = $this->resolveKey($name);
+
+ return array_key_exists($name, $attributes);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function get($name, $default = null)
+ {
+ $attributes = $this->resolveAttributePath($name);
+ $name = $this->resolveKey($name);
+
+ return array_key_exists($name, $attributes) ? $attributes[$name] : $default;
Joseph Bielawski
stloyd added a note

return $this->has($name) ? $attributes[$name] : $default ? Be more DRY ;-)

Drak
drak added a note

Doesn't work here because it would mean repeating the attribute resolution in the has() method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...pFoundation/SessionStorage/AbstractSessionStorage.php
((81 lines not shown))
+ * upload_progress.enabled, "1"
+ * upload_progress.cleanup, "1"
+ * upload_progress.prefix, "upload_progress_"
+ * upload_progress.name, "PHP_SESSION_UPLOAD_PROGRESS"
+ * upload_progress.freq, "1%"
+ * upload_progress.min-freq, "1"
+ * url_rewriter.tags, "a=href,area=href,frame=src,form=,fieldset="
+ *
+ * @param AttributeBagInterface $attributes An AttributeBagInterface instance, (defaults null for default AttributeBag)
+ * @param FlashBagInterface $flashes A FlashBagInterface instance (defaults null for default FlashBag)
+ * @param array $options Session configuration options.
+ */
+ public function __construct(AttributeBagInterface $attributes = null, FlashBagInterface $flashes = null, array $options = array())
+ {
+ $this->attributeBag = $attributes ? $attributes : new AttributeBag();
+ $this->flashBag = $flashes ? $flashes : new FlashBag();
Joseph Bielawski
stloyd added a note
$this->attributeBag = $attributes ?: new AttributeBag();
$this->flashBag = $flashes ?: new FlashBag();
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...pFoundation/SessionStorage/AbstractSessionStorage.php
((212 lines not shown))
+ * @see http://www.php.net/manual/en/session.configuration.php
+ */
+ protected function setOptions(array $options)
+ {
+ $cookieDefaults = session_get_cookie_params();
+ $this->options = array_merge(array(
+ 'cookie_lifetime' => $cookieDefaults['lifetime'],
+ 'cookie_path' => $cookieDefaults['path'],
+ 'cookie_domain' => $cookieDefaults['domain'],
+ 'cookie_secure' => $cookieDefaults['secure'],
+ 'cookie_httponly' => isset($cookieDefaults['httponly']) ? $cookieDefaults['httponly'] : false,
+ ), $options);
+
+ // Unless session.cache_limiter has been set explicitly, disable it
+ // because this is managed by HeaderBag directly (if used).
+ if (!array_key_exists('cache_limiter', $this->options)) {
Joseph Bielawski
stloyd added a note

!isset($this->options['cache_limiter']) as null should be also admitted as 0.

Drak
drak added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...pFoundation/SessionStorage/MockFileSessionStorage.php
((35 lines not shown))
+ */
+ private $savePath;
+
+ /**
+ * Constructor.
+ *
+ * @param string $savePath Path of directory to save session files.
+ * @param array $options Session options.
+ * @param AttributeBagInterface $attributes An AttributeBagInterface instance, (defaults null for default AttributeBag)
+ * @param FlashBagInterface $flashes A FlashBagInterface instance (defaults null for default FlashBag)
+ *
+ * @see AbstractSessionStorage::__construct()
+ */
+ public function __construct($savePath = null, array $options = array(), AttributeBagInterface $attributes = null, FlashBagInterface $flashes = null)
+ {
+ if (is_null($savePath)) {
Joseph Bielawski
stloyd added a note

CS fix: null === $savePath

Drak
drak added a note

OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...pFoundation/SessionStorage/MockFileSessionStorage.php
((121 lines not shown))
+
+ private function destroy()
+ {
+ if (is_file($this->getFilePath())) {
+ unlink($this->getFilePath());
+ }
+ }
+
+ /**
+ * Calculate path to file.
+ *
+ * @return string File path
+ */
+ public function getFilePath()
+ {
+ return $this->savePath . '/' . $this->sessionId . '.sess';
Joseph Bielawski
stloyd added a note

CS issue.

Drak
drak added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...oundation/SessionStorage/NativeFileSessionStorage.php
((28 lines not shown))
+ */
+ private $savePath;
+
+ /**
+ * Constructor.
+ *
+ * @param string $savePath Path of directory to save session files.
+ * @param array $options Session configuration options.
+ * @param AttributeBagInterface $attributes An AttributeBagInterface instance, (defaults null for default AttributeBag)
+ * @param FlashBagInterface $flashes A FlashBagInterface instance (defaults null for default FlashBag)
+ *
+ * @see AbstractSessionStorage::__construct()
+ */
+ public function __construct($savePath = null, array $options = array(), AttributeBagInterface $attributes = null, FlashBagInterface $flashes = null)
+ {
+ if (is_null($savePath)) {
Joseph Bielawski
stloyd added a note

CS fix: null === $savePath

Drak
drak added a note

OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...pFoundation/SessionStorage/MemcacheSessionStorage.php
((101 lines not shown))
+ /**
+ * {@inheritdoc}
+ */
+ public function sessionClose()
+ {
+ return $this->memcache->close();
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function sessionRead($sessionId)
+ {
+ $result = $this->memcache->get($this->prefix.$sessionId);
+
+ return ($result) ? $result : '';
Joseph Bielawski
stloyd added a note

Simply: return $this->memcache->get($this->prefix.$sessionId) ?: '';

Drak
drak added a note

OK.

Drak
drak added a note

OK.

Lukas Kahwe Smith Collaborator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...pFoundation/SessionStorage/MemcacheSessionStorage.php
((52 lines not shown))
+ * @param FlashBagInterface $flashes A FlashBagInterface instance (defaults null for default FlashBag)
+ *
+ * @see AbstractSessionStorage::__construct()
+ */
+ public function __construct(\Memcache $memcache, array $memcacheOptions = array(), array $options = array(), AttributeBagInterface $attributes = null, FlashBagInterface $flashes = null)
+ {
+ $this->memcache = $memcache;
+
+ // defaults
+ if (!isset($memcacheOptions['serverpool'])) {
+ $memcacheOptions['serverpool'] = array(
+ 'host' => '127.0.0.1',
+ 'port' => 11211,
+ 'timeout' => 1,
+ 'persistent' => false,
+ 'weight' => 1);
Joseph Bielawski
stloyd added a note

Why not move this array into variable definition (above) and later just run array_merge() ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...HttpFoundation/SessionStorage/ArraySessionStorage.php
((134 lines not shown))
{
- $this->data[$key] = $data;
+ return sha1(uniqid(mt_rand(), true));
Joseph Bielawski
stloyd added a note

You should use md5 as it's used by default in PHP.

Drak
drak added a note

It really makes no difference here, this is a mock storage class that's only used for functional testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...pFoundation/SessionStorage/AbstractSessionStorage.php
((208 lines not shown))
+ * session_get_cookie_params() overrides values.
+ *
+ * @param array $options
+ *
+ * @see http://www.php.net/manual/en/session.configuration.php
+ */
+ protected function setOptions(array $options)
+ {
+ $cookieDefaults = session_get_cookie_params();
+ $this->options = array_merge(array(
+ 'cookie_lifetime' => $cookieDefaults['lifetime'],
+ 'cookie_path' => $cookieDefaults['path'],
+ 'cookie_domain' => $cookieDefaults['domain'],
+ 'cookie_secure' => $cookieDefaults['secure'],
+ 'cookie_httponly' => isset($cookieDefaults['httponly']) ? $cookieDefaults['httponly'] : false,
+ ), $options);
Joseph Bielawski
stloyd added a note

IMO better would be moving this array into variable definition an later just: $this->options = array_merge($this->options, $options);

Drak
drak added a note

I don't understand, session_get_cookie_params() returns different keys so they have to be translated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Lukas Kahwe Smith
Collaborator

@fabpot did you have time to review this yet? with all the work @drak has done its important that he gets some feedback soon. its clear this PR breaks BC in ways we never wanted to allow. but i think this PR also clearly explains why its necessary none the less.

Drak

@fabpot - I have removed the WIP status from this PR now and rebased against the current master branch.

UPGRADE-2.1.md
((53 lines not shown))
+ </div>
+ {% endforeach %}
+
+ Again you can process all flash messages in one go with
+
+ {% for type, flashMessages in app.session.popAllFlashes() %}
+ {% for flashMessage in flashMessages) %}
+ <div class="flash-{{ type }}">
+ {{ flashMessage }}
+ </div>
+ {% endforeach %}
+ {% endforeach %}
+
+.. note::
+
+ You can access optionally use constants in Twig templates using `constant()` e.g.
Tobias Schultze Collaborator
Tobion added a note

typo access

Drak
drak added a note

Fixed (will appear in next push).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
UPGRADE-2.1.md
((69 lines not shown))
+ `constant('Symfony\Component\HttpFoundation\FlashBag::NOTICE')`.
+
+* Session object
+
+ The methods, `setFlash()`, `hasFlash()`, and `removeFlash()` have been removed from the `Session`
+ object. You may use `addFlash()` to add flashes. `getFlashes()`, now returns an array. Use
+ `popFlashes()` to get flashes for display, or `popAllFlashes()` to process all flashes in on go.
+
+* Session storage drivers
+
+ Session storage drivers should inherit from
+ `Symfony\Component\HttpFoundation\SessionStorage\AbstractSessionStorage`
+ and no longer should implement `read()`, `write()`, `remove()` which were removed from the
+ `SessionStorageInterface`.
+
+ Any session storage drive that wants to use custom save handlers should
Tobias Schultze Collaborator
Tobion added a note

typo: driver

Drak
drak added a note

Fixed (will appear in next push).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...dation/SessionStorage/SessionSaveHandlerInterface.php
((40 lines not shown))
+ * When a session is specifically destroyed, PHP will call the
+ * sessionDestroy() handler with the session ID. This happens when the
+ * session is regenerated for example and th handler MUST delete the
+ * session by ID from the persistent storage immediately.
+ *
+ * PHP will call sessionGc() from time to time to expire any session
+ * records according to the set max lifetime of a session. This routine
+ * should delete all records from persistent storage which were last
+ * accessed longer than the $lifetime.
+ *
+ * PHP sessionOpen() and sessionClose() are pretty much redundant and
+ * can return true.
+ *
+ * @author Drak <drak@zikula.org>
+ */
+interface SessionSaveHandlerInterface
Drak
drak added a note

SessionHandler class is a PHP 5.4 feature, the class is not found in PHP 5.3. You can see this in the individual methods docs e.g. http://www.php.net/manual/en/sessionhandler.read.php

Lukas Kahwe Smith Collaborator
lsmith77 added a note

well if its available in 5.4, we can consider providing a user land implementation that is loaded for versions below 5.4

Drak
drak added a note

Actually, there is no benefit here and in-fact, there are more benefits of using an interface because we can interact nicely with the storage driver internals. Sure it can be done via composition but it just get overly complex for no extra reason. SessionHandler simply is a class stub for the five session save handlers - it's quite useless actually - I dont even know why they bothered to create a class, it should have been an interface.

Lukas Kahwe Smith Collaborator
lsmith77 added a note

<arpad> lsmith: i've added an interface and made that the check in session_set_save_handler, but ran into #60634 while testing it so not quite there yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...pFoundation/SessionStorage/AbstractSessionStorage.php
((264 lines not shown))
+ * When a session is specifically destroyed, PHP will call the sessionDestroy() handler with the
+ * session ID. This happens when the session is regenerated for example and th handler
+ * MUST delete the session by ID from the persistent storage immediately.
+ *
+ * PHP will call sessionGc() from time to time to expire any session records according to the
+ * set max lifetime of a session. This routine should delete all records from persistent
+ * storage which were last accessed longer than the $lifetime.
+ *
+ * PHP sessionOpen() and sessionClose() are pretty much redundant and can just return true.
+ *
+ * NOTE:
+ *
+ * To use PHP native save handlers, override this method using ini_set with
+ * session.save_handlers and session.save_path e.g.
+ *
+ * ini_set('session.save_handlers', 'files');
Fabien Potencier Owner
fabpot added a note

Looks like this option is actually called session.save_handler, not session.save_handlers (http://www.php.net/manual/en/session.configuration.php#ini.session.save-handler).

Drak
drak added a note

Nicely spotted. I've fixed this. The reason the tests were working was because the handlers were unwittingly being set by the session_module_name() tests in the constructors of the other Native* handlers when I should have been using extension_loaded() to test the presence. files is on by default so in this particular case it didn't matter. I fixed this in d3fe874

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
.../HttpFoundation/SessionStorage/NullSessionStorage.php
((60 lines not shown))
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function sessionDestroy($sessionId)
+ {
+ return true;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function sessionGc($lifetime)
+ {
+ return true;
Tobias Schultze Collaborator
Tobion added a note

According to http://www.php.net/manual/en/sessionhandler.gc.php the return value of all session handler functions is:

The return value of the session storage (usually 0 on success, 1 on failure).

But you seem to always return a boolean and true for success which is the opposite.
I don't know what the real deal is.

Drak
drak added a note

For us this is not relevant because the class belongs to PHP 5.4 but in any case it looks like another PHP 5.4 bug since save handlers have always returned true or false except for the read handler which must always return a string.
See http://www.php.net/manual/en/function.session-set-save-handler.php for details.

Lukas Kahwe Smith Collaborator
lsmith77 added a note

Can you report this as a bug with php.net?

Drak
drak added a note

I already emailed the PHP internal list and filed a bug report too https://bugs.php.net/bug.php?id=60640 and http://news.php.net/php.internals/57169

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Tobias Schultze
Collaborator

From what I read from the IRC chat logs, the main concern of @fabpot is whether we really need multiple flash messages per type. I'm in favor of this PR and just want to add one point to this discussion.
At the moment you can add multiple flash messages of different type/category/identifier. For example you can specify one error message and one info message after an operation. I think most agree that this can be usefull.
But then it makes semantically no sense that you currently cannot add 2 info messages. This approach feels a bit half-done.
So I think this PR eliminates this paradox.

...ymfony/Component/HttpFoundation/FlashBagInterface.php
((45 lines not shown))
+ *
+ * @return array
+ */
+ function get($type);
+
+ /**
+ * Pops and clears flashes from the stack.
+ *
+ * @param string $type
+ *
+ * @return array
+ */
+ function pop($type);
+
+ /**
+ * Pops all flashes from the stacl and clears flashes.
Tobias Schultze Collaborator
Tobion added a note

typo: stack

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Drak

For reference there is also a discussion started by @lsmith77 on the mailing list at https://groups.google.com/forum/#!topic/symfony-devs/cy4wokD0mQI

Florin Patan

@drak I could also add the next scenario that I currently have to live with, in addition to @lsmith77 ones.

I had this issue while working on our shopping cart implementation for a customer where the customer wanted to show the unavailability of the items as different lines in the 'flash-error' section of the cart. We had to set an array as the 'flash message' in order to display that information.

So in this case for example having the flash messages types as array would actually make more sense that sending an array to the flasher. Plus the the other issue we had was that we also wanted to add another error in the message but we had to do a check to see if the flash message is an array already or we need to make it an array.

I think it's better not to impose a limit of this sort and let the users be able to handle every scenario, even if some are rare, rather that forcing users to overcome limitations such as these.

I really hope this PR gets approved faster and thanks everyone for their hard work :)

Tobias Schultze
Collaborator

@dlsniper I think you misinterpreted my point.

Florin Patan

@Tobion I'm sorry I did that, I'll edit the message asap. Seems no sleep in 26 hours can cause brain not to function as intended :)

Lukas Kahwe Smith
Collaborator

FYI the drupal guys are liking this PR (including the flash changes):
http://drupal.org/node/335411

Drak

@lsmith77 Fabien asked me to remove the changes to the flash messages so that they are as before - i.e. only one flash per name/type /cc @fabpot

Fabien Potencier
Owner

To be clear, I've asked to split this PR in two parts:

  • one about the session refactoring (which is non-controversial and should be merged ASAP)
  • this one with only the flash refactoring
...pFoundation/SessionStorage/MemcacheSessionStorage.php
((101 lines not shown))
+ /**
+ * {@inheritdoc}
+ */
+ public function sessionClose()
+ {
+ return $this->memcache->close();
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function sessionRead($sessionId)
+ {
+ return $this->memcache->get($this->prefix.$sessionId) ?: '';
+
+ return ($result) ? $result : '';
Deni
yethee added a note

Duplicate return statement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...dation/SessionStorage/SessionSaveHandlerInterface.php
((54 lines not shown))
+ */
+interface SessionSaveHandlerInterface
+{
+ /**
+ * Open session.
+ *
+ * This method is for internal use by PHP and must not be called manually.
+ *
+ * @param string $savePath Save path.
+ * @param string $sessionName Session Name.
+ *
+ * @throws \RuntimeException If something goes wrong starting the session.
+ *
+ * @return boolean
+ */
+ function sessionOpen($savePath, $sessionName);
Bernhard Schussek Collaborator

Shouldn't this and the following methods be called openSession, closeSession etc. to be consistent with the naming in the rest of the framework?

Drak
drak added a note

It was to make this clear that these are for the save_handlers and are for internal use.

Bernhard Schussek Collaborator

Why should naming conventions differ for internally used methods?

Fabien Potencier Owner
fabpot added a note

I agree with @bschussek.

Christophe Coevoet Collaborator
stof added a note

@drak internal depends of the level you work. For someone implementing the interface, they are not internal methods. They are the interface.

Drak
drak added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...pFoundation/SessionStorage/AbstractSessionStorage.php
((88 lines not shown))
+ *
+ * @param AttributeBagInterface $attributes An AttributeBagInterface instance, (defaults null for default AttributeBag)
+ * @param FlashBagInterface $flashes A FlashBagInterface instance (defaults null for default FlashBag)
+ * @param array $options Session configuration options.
+ */
+ public function __construct(AttributeBagInterface $attributes = null, FlashBagInterface $flashes = null, array $options = array())
+ {
+ $this->attributeBag = $attributes ?: new AttributeBag();
+ $this->flashBag = $flashes ?: new FlashBag();
+ $this->setOptions($options);
+ $this->registerSaveHandlers();
+ $this->registerShutdownFunction();
+ }
+
+ /**
+ * {@inheritdoc}
Bernhard Schussek Collaborator

This method is not present in the interface, so nothing to inherit.

Drak
drak added a note

It's in SessionStorageInterface

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...pFoundation/SessionStorage/AbstractSessionStorage.php
((100 lines not shown))
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getFlashes()
+ {
+ if ($this->options['auto_start'] && !$this->started) {
+ $this->start();
+ }
+
+ return $this->flashBag;
+ }
+
+ /**
+ * {@inheritdoc}
Bernhard Schussek Collaborator

No present in the interface either.

Drak
drak added a note

It's in SessionStorageInterface

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Drak

@fabpot this is ready to be merged now. I will open a separate PR later today for the flash messages as a bucket.

Fabien Potencier
Owner

I must have missed something, but I still see a lot of changes related to the flash messages.

Drak

When I spoke to you you said you wanted to make the commit with flash messages with one message per name/type rather than multiple. The old flash messages behaviour is 100% maintained in AutoExpireFlashBag which can be the default in the framework if you wish. The FlashBag implementation makes Symfony2 ESI compatible.

Christophe Coevoet
Collaborator

@drak splitting into 2 PRs means you should not refactor the flash messages in this one but in the dedicated one.

Drak
drak and others added some commits
Drak drak [HttpFoundation] Added AttributesInterface and AttributesBagInterface…
… and concrete implementations.

This commit outsources session attribute storage to it's own class.
There are two concrete implementations, one with structured namespace storage and the other
without.
39288bc
Drak drak [HttpFoundation] Added FlashBagInterface and concrete implementation.
This commit outsources the flash message processing to it's own interface.

Overall flash messages now can have multiple flash types and each type can
store multiple messages.  For convenience there are now four flash types
by default, INFO, NOTICE, WARNING and ERROR.

There are two concrete implementations: one preserving the old behaviour of
flash messages expiring exactly after one page load, regardless of being
displayed or not; and the other where flash messages persist until explicitly
popped.
c969423
Drak drak [HttpFoundation] Introduced session storage base class and interfaces.
Session object now implements SessionInterface to make it more portable.

AbstractSessionStorage and SessionSaveHandlerInterface now makes implementation
of session storage drivers simple and easy to write for both custom save handlers
and native php save handlers and respect the PHP session workflow.
3a263dc
Drak drak [HttpFoundation] Added unit and functional testing session storage ob…
…jects.
57ef984
Drak drak [HttpFoundation] Added drivers for PHP native session save handlers, …
…files, sqlite, memcache and memcached.
85b5c43
Drak drak [HttpFoundation] Refactored component for session workflow. e185c8d
Drak drak [HttpFoundation] Added pure Memcache, Memcached and Null storage driv…
…ers.
669bc96
Drak drak [FrameworkBundle] Refactored code for changes to HttpFoundation compo…
…nent.

Native PHP sessions stored to file are done with session.storage.native_file
Functional testing is done with session.storage.mock_file

Default flash message implementation done with FlashBag (session.flash_bag)
Default attribute storage implementation with AttributeBag (session.attribute_bag)

Services added: session.storage.native_file, session.storage.native_memcache, session.storage.native_memcache,
session.storage.native_sqlite, session.storage.memcache, session.storage.memcached, session.storage.null,
session.storage.mock_file, session.flash_bag, session.attribute_bag

Services removed: session.storage.native, session.storage.filesystem
7aaf024
Drak drak [DoctribeBridge][SecurityBundle][WebProfiler] Refactor code for HttpF…
…oundation changes.
1ed6ee3
Drak drak Documentation, changelogs and coding standards. 9dd4dbe
Drak drak [HttpFoundation] Refactor for DRY code.
Rename ArraySessionStorage to make it clear the session is a mock for testing purposes only.
Has BC class for ArraySessionStorage
Added sanity check when starting the session.
Fixed typos and incorrect php extension test method
session_module_name() also sets session.save_handler, so must use extension_loaded() to check if module exist
or not.
Respect autostart settings.
f98f9ae
Drak drak [HttpFoundation] Reworked flashes to maintain same behaviour as in Sy…
…mfony 2.0
398acc9
Drak drak Fixed formatting. f9951a3
Drak drak [DoctrineBridge] Refactored driver for changed interface. d64939a
Drak drak [HttpFoundation] Free bags from session storage and move classes to t…
…heir own namespaces.
4683915
Drak drak [HttpFoundation] Moved session related classes to own sub-namespace. 27530cb
Drak drak [HttpFoundation] Simplify session storage class names now we have a s…
…eparate namespace for sessions.
5b7ef11
Drak drak [HttpFoundation] Add back get defaults and small clean-up.
Changed read-only method names from get*() to peek*()

Typo
dad60ef
Drak drak [HttpFoundation] Remove constants from FlashBagInterface
As requested by fabpot.
Corrected a few mistakes in the documentation.
0d2745f
Fabien Potencier fabpot [HttpFoundation] renamed pop() to all() and getAll() to all() 7878a0a
Fabien Potencier fabpot removed unused use statements 0494250
Fabien Potencier fabpot [HttpFoundation] changed default flash bag to auto-expires to keep BC 91f4f8a
Fabien Potencier fabpot reverted 5b7ef11 (Simplify session
storage class names now we have a separate namespace for sessions)
74ccf70
Fabien Potencier fabpot [HttpFoundation] removed configuration for session storages in sessio…
…n.xml as we cannot provide a way to configure them (like before this PR anyway)
93d81a1
Fabien Potencier fabpot [FrameworkBundle] added some service aliases to avoid some BC breaks 146a502
Fabien Potencier fabpot [HttpFoundation] added some method for a better BC 0f6c50a
Fabien Potencier fabpot updated CHANGELOG for 2.1 282d3ae
Fabien Potencier fabpot renamed getFlashes() to getFlashBag() to avoid clashes 8a01dd5
Christophe Coevoet

shouldn't it be $session->getFlashBag() instanceof AutoExpireFlashBag ?

Drak

I am not entirely convinced about this because close() is only used in unit tests. Given this method dosn't do anything now I think it should be removed. I understand about keeping BC, but things should break on an occasion like this to allow refactoring. That is far more reliable than code continuing to work but in a now unexpected way. Overall, since it only would break tests, I think it should be removed.

Or possibly throw a exception and mark it deprecated

Collaborator

throwing an exception would also be a BC break

Sure - I am saying that we need to clearly show this method now does not do anything. Since it would only break tests anyhow, I think it's better to remove this method entirely.

Owner

ok, I've removed the method.

Drak

Could we set them for removal in 2.2 instead?

Fabien Potencier fabpot referenced this pull request from a commit
Fabien Potencier fabpot merged branch drak/session_refactor (PR #2853)
Commits
-------

cb6fdb1 [HttpFoundation] removed Session::close()
c59d880 Docblocks.
b8df162 Correct instanceof condition.
8a01dd5 renamed getFlashes() to getFlashBag() to avoid clashes
282d3ae updated CHANGELOG for 2.1
0f6c50a [HttpFoundation] added some method for a better BC
146a502 [FrameworkBundle] added some service aliases to avoid some BC breaks
93d81a1 [HttpFoundation] removed configuration for session storages in session.xml as we cannot provide a way to configure them (like before this PR anyway)
74ccf70 reverted 5b7ef11 (Simplify session storage class names now we have a separate namespace for sessions)
91f4f8a [HttpFoundation] changed default flash bag to auto-expires to keep BC
0494250 removed unused use statements
7878a0a [HttpFoundation] renamed pop() to all() and getAll() to all()
0d2745f [HttpFoundation] Remove constants from FlashBagInterface
dad60ef [HttpFoundation] Add back get defaults and small clean-up.
5b7ef11 [HttpFoundation] Simplify session storage class names now we have a separate namespace for sessions.
27530cb [HttpFoundation] Moved session related classes to own sub-namespace.
4683915 [HttpFoundation] Free bags from session storage and move classes to their own namespaces.
d64939a [DoctrineBridge] Refactored driver for changed interface.
f9951a3 Fixed formatting.
398acc9 [HttpFoundation] Reworked flashes to maintain same behaviour as in Symfony 2.0
f98f9ae [HttpFoundation] Refactor for DRY code.
9dd4dbe Documentation, changelogs and coding standards.
1ed6ee3 [DoctribeBridge][SecurityBundle][WebProfiler] Refactor code for HttpFoundation changes.
7aaf024 [FrameworkBundle] Refactored code for changes to HttpFoundation component.
669bc96 [HttpFoundation] Added pure Memcache, Memcached and Null storage drivers.
e185c8d [HttpFoundation] Refactored component for session workflow.
85b5c43 [HttpFoundation] Added drivers for PHP native session save handlers, files, sqlite, memcache and memcached.
57ef984 [HttpFoundation] Added unit and functional testing session storage objects.
3a263dc [HttpFoundation] Introduced session storage base class and interfaces.
c969423 [HttpFoundation] Added FlashBagInterface and concrete implementation.
39288bc [HttpFoundation] Added AttributesInterface and AttributesBagInterface and concrete implementations.

Discussion
----------

[2.1][HttpFoundation] Refactor session handling and flash messages

Bug fix: yes
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #2607, #2591, #2717, #2773
References the following tickets: #2592, #2543, #2541, #2510, #2714, #2684
Todo: -

__Introduction__

This extensive PR is a refactor with minimal BC breaks of the `[HttpFoundation]` component's session management which fixes several issues in the current implementation.  This PR includes all necessary changes to other bundles and components is documented in the `CHANGELOG-2.1` and `UPGRADING-2.1`.

__Summary of Changes__

__Session:__
  - Session object now implements `SessionInterface`

__Attributes:__
  - Attributes now handled by `AttributeBagInterface`
  - Added two AttributeBag implementations: `AttributeBag` replicates the current Symfony2 attributes behaviour, and the second, `NamespacedAttributeBag` introduces structured namespaced representation using '/' in the key.  Both are BC.  `FrameworkBundle` defaults to the old behaviour.

__Flash messages:__
  - Flash messages now handled by `FlashBagInterface`
  - Introduced `FlashBag` which changes the way flash messages expire, they now expire on use rather than automatically, useful for ESI.
  - Introduced `AutoExpireFlashBag` (default) which replicates the old automatic expiry behaviour of flash messages.

__Session Storage:__
  - Introduced a base object, `AbstractSessionStorage` for session storage drivers
  - Introduced a `SessionSaveHandlerInterface` when using custom session save handlers
  - Introduced a `NullSessionStorage` driver which allows for unsaved sessions
  - Introduced new session storage drivers for Memcache and Memcached
  - Introduced new session storage drivers for PHP's native SQLite, Memcache and Memcached support

__General:__
  - Fixed bugs where session attributes are not saved and all cases where flash messages would get lost
  - Wrote new tests, refactored related existing tests and increased test coverage extensively.

__Rationale/Details__

I'll explain more detail in the following sections.

__Unit Tests__

All unit and functional tests pass.

__Note on Functional Testing__

I've introduced `MockFileSessionStorage` which replaces `FilesystemSessionStorage` to emulate a PHP session for functional testing.  Essentially the same methodology of functional testing has been maintained but without interrupting the other session storage drivers interaction with real PHP sessions.  The service is now called `session.storage.mock_file`.

__Session Workflow__

PHP sessions follow a specific workflow which is not being followed by the current session management implementation and is responsible for some unpredictable bugs and behaviours.

Basically, PHP session workflow is as follows: `open`, `read`, `write`, `close`.  In between these can occur, `destroy` and `garbage collection`.  These actions are handled by `session save handlers` and one is always registered in all cases.  By default, the `files` save handler (internally to PHP) is registered by PHP at the start of code execution.

PHP offers the possibility to change the save handler to another internal type, for example one provided by a PHP extension (think SQLite, Memcache etc), or you can register a user type and set your own handlers.  However __in all cases__ PHP requires the handlers.

The handlers are called when the following things occur:

  - `open` and `read` when `session_start()` or the session autostarts when PHP outputs some display
  - `destroy` when `session_regenerate_id(true)` is called
  - `write` and `close` when PHP shuts down or when `session_write_close()` is called
  - `garbage collection` is called randomly according to configurable probability

The next very important aspect of this PR is that `$_SESSION` plays an important part in this workflow because the contents of the $_SESSION is populated __after__ the `read` handler returns the previously saved serialised session data.  The `write` handler is sent the serialised `$_SESSION` contents for save.  Please note the serialisation is a different format to `serialize()`.

For this reason, any session implementation cannot get rid of using `$_SESSION`.

I wrote more details on this issue [here](#2607 (comment))

In order to make writing session storage drivers simple, I created a light base class `AbstractSessionStorage` and the `SessionSaveHandlerInterface` which allows you to quickly write native and custom save handler drivers.

__Flash Messages [BC BREAK]__

Flash messages currently allow representation of a single message per `$name`.  Fabien designed the original system so that `$name` was equivalent to flash message type.  The current PR changes the fact that Flash messages expire explicitly when retrieved for display to the user as opposed to immediately on the next page load.

The last issue fixes potential cases when flash messages are lost due to an unexpected intervening page-load (an error for example).  The API `get()` has a flag which allows you to override the `clear()` action.

__Flash message translation__

This PR does not cover translation of flash messages because  messages should be translated before calling the flash message API.  This is because flash messages are used to present messages to the user after a specific action, and in any case, immediately on the next page load.  Since we know the locale of the request in every case we can translate the message before storing.  Secondly, translation is simply a string manipulation.  Translation API calls should always have the raw untranslated string present because it allows for extraction of translation catalogs.  For a complete answer see my answer [here](#2543 (comment))

__Session attribute and structured namespacing__

__This has been implemented without changing the current default behaviour__ but details are below for the alternative:

Attributes are currently stored in a flat array which limits the potential of session attributes:

Here are some examples to see why this 'structured namespace' methodology is extremely convenient over using a flat system.  Let's look at an example with csrf tokens.  Let's say we have multiple csrftokens stored by form ID (allowing multiple forms on the page and tabbed browsing).

If we're using a flat system, you might have

    'tokens' => array('a' => 'a6c1e0b6',
                      'b' => 'f4a7b1f3')

With a flat system when you get the key `tokens`, you will get back an array, so now you have to analyse the array.  So if you simply want to add another token, you have to follow three steps: get the session attribute `tokens`, have to add to the array, and lastly set the entire array back to the session.

    $tokens = $session->get('tokens');
    $tokens['c'] = $value;
    $session->set('tokens', $tokens);

Doable, but you can see it's pretty long winded.

With structured namespacing you can simply do:

    $session->set('c', $value, '/tokens');

There are several ways to implement this, either with an additional `$namespace` argument, or by treating a character in the `$key` as a namespacer.  `NamespacedAttributeBag` treats `/` as a namespacer so you can represent `user.tokens/a` for example.  The namespace character is configurable in `NamespacedAttributeBag`.

---------------------------------------------------------------------------

by marijn at 2011-12-18T15:43:17Z

I haven't read the code yet but the description from this PR and your line of thought seem very well structured.
Seems like a big +1 for me.

---------------------------------------------------------------------------

by lsmith77 at 2011-12-19T16:01:19Z

@deviantintegral could you look over this to see if it really addresses everything you wanted with PR #2510 ?

---------------------------------------------------------------------------

by deviantintegral at 2011-12-24T20:12:03Z

I've read through the documentation and upgrade notes, and I can't see anything that's obviously missing from #2510. Being able to support multiple flashes per type is the most important, and the API looks reasonable to me. Drupal does support supressing repeat messages, but that can easily be implemented in our code unless there's a compelling case for it to be a part of Symfony.

I wonder if PHP memcache support is required in Symfony given the availability of memcached. I'm not familiar with how other parts of Symfony handle it, but there is often quite a bit of confusion between the two PHP extensions. It could be simpler to remove one, or add a bit of info describing or linking to why there are two nearly identical classes.

Is it possible to make one class inherit from the other (memcached is a child of memcache)?

---------------------------------------------------------------------------

by Fristi at 2011-12-24T20:29:46Z

Interesting, maybe add: session events as I did with the current impl: https://github.com/Fristi/SessionBundle

---------------------------------------------------------------------------

by drak at 2011-12-25T00:50:03Z

@deviantintegral - I agree about the confusion between memcache and memcached but actually, it is necessary to support both because `memcached` is not available everywhere.  For example on Debian Lenny and RHEL/CentOS 5, only memcache is available by default.  This would preclude a massive amount of shared hosting environments.  Also, it is not possible to inherit one from the other, they are completely different drivers.

@Fristi - I also thought about the events, but they do not belong as part of the standalone component as this would create a coupling to the event dispatcher.  The way you have done it, ie, in a bundle is the right way to achieve it.

---------------------------------------------------------------------------

by matheo at 2011-12-25T01:12:00Z

Impressive work, looks like a big improvement and deserves a big +1

---------------------------------------------------------------------------

by datiecher at 2011-12-26T11:57:12Z

Took some time to grok all the changes in this PR but all in all it is a keeper. Specially the new flash message API, it's really nicer to work with it then the previous one.

Nicely done @drak!

---------------------------------------------------------------------------

by lsmith77 at 2012-01-02T15:00:00Z

@fabpot did you have time to review this yet? with all the work @drak has done its important that he gets some feedback soon. its clear this PR breaks BC in ways we never wanted to allow. but i think this PR also clearly explains why its necessary none the less.

---------------------------------------------------------------------------

by drak at 2012-01-02T15:41:53Z

@fabpot - I have removed the WIP status from this PR now and rebased against the current master branch.

---------------------------------------------------------------------------

by Tobion at 2012-01-07T07:13:38Z

From what I read from the IRC chat logs, the main concern of @fabpot is whether we really need multiple flash messages per type. I'm in favor of this PR and just want to add one point to this discussion.
At the moment you can add multiple flash messages of different type/category/identifier. For example you can specify one error message and one info message after an operation. I think most agree that this can be usefull.
But then it makes semantically no sense that you currently cannot add 2 info messages. This approach feels a bit half-done.
So I think this PR eliminates this paradox.

---------------------------------------------------------------------------

by drak at 2012-01-07T09:11:07Z

For reference there is also a discussion started by @lsmith77 on the mailing list at https://groups.google.com/forum/#!topic/symfony-devs/cy4wokD0mQI

---------------------------------------------------------------------------

by dlsniper at 2012-01-07T16:02:15Z

@drak I could also add the next scenario that I currently have to live with, in addition to @lsmith77 ones.

I had this issue while working on our shopping cart implementation for a customer where the customer wanted to show the unavailability of the items as different lines in the 'flash-error' section of the cart. We had to set an array as the 'flash message' in order to display that information.

So in this case for example having the flash messages types as array would actually make more sense that sending an array to the flasher. Plus the the other issue we had was that we also wanted to add another error in the message but we had to do a check to see if the flash message is an array already or we need to make it an array.

I think it's better not to impose a limit of this sort and let the users be able to handle every scenario, even if some are rare, rather that forcing users to overcome limitations such as these.

I really hope this PR gets approved faster and thanks everyone for their hard work :)

---------------------------------------------------------------------------

by Tobion at 2012-01-07T21:01:07Z

@dlsniper I think you misinterpreted my point.

---------------------------------------------------------------------------

by dlsniper at 2012-01-07T21:04:04Z

@Tobion I'm sorry I did that, I'll edit the message asap. Seems no sleep in 26 hours can cause brain not to function as intended :)

---------------------------------------------------------------------------

by lsmith77 at 2012-02-01T14:38:52Z

FYI the drupal guys are liking this PR (including the flash changes):
http://drupal.org/node/335411

---------------------------------------------------------------------------

by drak at 2012-02-01T14:51:33Z

@lsmith77 Fabien asked me to remove the changes to the flash messages so that they are as before - i.e. only one flash per name/type /cc @fabpot

---------------------------------------------------------------------------

by fabpot at 2012-02-01T14:58:23Z

To be clear, I've asked to split this PR in two parts:

 * one about the session refactoring (which is non-controversial and should be merged ASAP)
 * this one with only the flash refactoring

---------------------------------------------------------------------------

by drak at 2012-02-02T11:29:26Z

@fabpot this is ready to be merged now.  I will open a separate PR later today for the flash messages as a bucket.

---------------------------------------------------------------------------

by fabpot at 2012-02-02T11:34:39Z

I must have missed something, but I still see a lot of changes related to the flash messages.

---------------------------------------------------------------------------

by drak at 2012-02-02T11:39:10Z

When I spoke to you you said you wanted to make the commit with flash messages with one message per name/type rather than multiple.  The old flash messages behaviour is 100% maintained in `AutoExpireFlashBag` which can be the default in the framework if you wish.  The `FlashBag` implementation makes Symfony2 ESI compatible.

---------------------------------------------------------------------------

by stof at 2012-02-02T11:47:38Z

@drak splitting into 2 PRs means you should not refactor the flash messages in this one but in the dedicated one.

---------------------------------------------------------------------------

by drak at 2012-02-02T12:29:43Z

@stof Yes. I discussed with Fabien over chat there are basically no changes
to flashes in `FlashBag` and `AutoExpireFlashBag` maintains the exact
behaviour as before.  The FlashBag just introduces ESI compatible flashes.
 There is no way to refactor the sessions without moving the flash messages
to their own bag.  The next PR will propose the changes to flashes that
allow multiple messages per name/type.  I can size the PR down a little
more removing the new storage drivers and so on to make the PR smaller but
that's really as far as I can go.  To be clear, while the API has changed a
little for flashes, the behaviour is the same.
4841400
Fabien Potencier fabpot merged commit cb6fdb1 into from
Christophe Coevoet stof commented on the diff
UPGRADE-2.1.md
((26 lines not shown))
+ <div class="flash-$type">
+ <?php echo $flash; ?>
+ </div>
+ <?php endforeach; ?>
+
+ Before (Twig):
+
+ {% if app.session.hasFlash('notice') %}
+ <div class="flash-notice">
+ {{ app.session.flash('notice') }}
+ </div>
+ {% endif %}
+
+ After (Twig):
+
+ {% if app.session.flashes.has('notice') %}
Christophe Coevoet Collaborator
stof added a note

@fabpot shouldn't it be flashBag instead of flashes as you renamed it ?

Fabien Potencier Owner
fabpot added a note

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Christophe Coevoet stof commented on the diff
...workBundle/DependencyInjection/FrameworkExtension.php
@@ -301,7 +301,7 @@ private function registerSessionConfiguration(array $config, ContainerBuilder $c
$this->addClassesToCompile(array(
'Symfony\\Bundle\\FrameworkBundle\\EventListener\\SessionListener',
- 'Symfony\\Component\\HttpFoundation\\SessionStorage\\SessionStorageInterface',
+ 'Symfony\\Component\\HttpFoundation\\Session\\Storage\\SessionStorageInterface',
Christophe Coevoet Collaborator
stof added a note

shouldn't you add the abstract storage too ?

Fabien Potencier Owner
fabpot added a note

added. thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Christophe Coevoet stof commented on the diff
UPGRADE-2.1.md
((6 lines not shown))
+
+ Before (PHP):
+
+ <?php if ($view['session']->hasFlash('notice')): ?>
+ <div class="flash-notice">
+ <?php echo $view['session']->getFlash('notice') ?>
+ </div>
+ <?php endif; ?>
+
+ After (PHP):
+
+ <?php if ($view['session']->getFlashBag()->has('notice')): ?>
+ <div class="flash-notice">
+ <?php echo $view['session']->getFlashBag()->get('notice') ?>
+ </div>
+ <?php endif; ?>
Christophe Coevoet Collaborator
stof added a note

this is wrong. $view['session'] is not the Session object in the PHP templates but the SessionHelper which is BC.

The Session object is available through $app->getSession() (equivalent to the Twig way to access it)

Fabien Potencier Owner
fabpot added a note

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Christophe Coevoet stof commented on the diff
src/Symfony/Component/HttpFoundation/Session/Session.php
((196 lines not shown))
+ *
+ * @api
+ */
+ public function getId()
+ {
+ return $this->storage->getId();
+ }
+
+ /**
+ * Implements the \Serialize interface.
+ *
+ * @return SessionStorageInterface
+ */
+ public function serialize()
+ {
+ return serialize($this->storage);
Christophe Coevoet Collaborator
stof added a note

Is it right ? AFAICS, nothing enforces the storage to be serializable (and I don't see how a PDOSessionStorage could be serializable)

Drak
drak added a note

@stof already being discussed in #3000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jordi Boggiano

I believe it would make more sense if this would just match http://ch.php.net/manual/en/class.sessionhandlerinterface.php since this will be in 5.4 - it might make stuff compatible by default.

I was thinking about that, I've made a PR. Please note the documentation is wrong at php.net for the interface, I've an open ticket on it.

This was done in #3342 ad #3334

Fabien Potencier fabpot referenced this pull request from a commit
Fabien Potencier fabpot merged branch drak/session_flashmessages (PR #3267)
Commits
-------

5ae76f1 [HttpFoundation] Update documentation.
910b5c7 [HttpFoudation] CS, more tests and some optimization.
b0466e8 [HttpFoundation] Refactored BC Session class methods.
84c2e3c [HttpFoundation] Allow flash messages to have multiple messages per type.

Discussion
----------

[2.1][HttpFoundation] Multiple session flash messages

Bug fix: no
Feature addition: yes
Backwards compatibility break: yes, but this already happened in #2583.  BC `Session` methods remain unbroken.
Symfony2 tests pass: yes
Fixes the following tickets: #1863
References the following tickets: #2714, #2753, #2510, #2543, #2853
Todo: -

This PR alters flash messages so that it is possible to store more than one message per flash type using the `add()` method or by passing an array of messages to `set()`.

__NOTES ABOUT BC__

This PR maintains BC behaviour with the `Session` class in that the old Symfony 2.0 methods will continue to work as before.

---------------------------------------------------------------------------

by drak at 2012-02-13T06:28:33Z

I think this is ready for review @fabpot @lsmith77

---------------------------------------------------------------------------

by lsmith77 at 2012-02-14T19:30:39Z

the FlashBag vs. AutoExpireFlashBag behavior and setup difference should probably also be explained in the upgrading log

---------------------------------------------------------------------------

by drak at 2012-02-15T04:43:14Z

@lsmith77 Those differences are explained already in the changelog

 * Added `FlashBag`. Flashes expire when retrieved by `get()` or `all()`.
   This makes the implementation ESI compatible.
 * Added `AutoExpireFlashBag` (default) to replicate Symfony 2.0.x auto expire behaviour of messages auto expiring
   after one page page load.  Messages must be retrived by `get()` or `all()`.

---------------------------------------------------------------------------

by Crell at 2012-02-19T17:35:34Z

Drak asked me to weigh in here with use cases.  Drupal currently has a similar session-stored-messaging system in place that I'd like to be able to replace with Flash messages.  We frequently have multiple messages within a single request, however, so this change is critical to our being able to do so.

For instance, when saving an article in Drupal there is, by default, a "yay, you saved an article!" type message that gets displayed.  If you also have the site configured to send email when a post is updated, you may see a "email notifications sent" message (depending on your access level).  If you have a Solr server setup for search, and you're in debug mode, there will also be a "record ID X added to Solr, it should update in 2 minutes" message.  And if there's a bug somewhere, you'll also get, as an error message rather than notice message, a "Oops, E_NOTICE on line 54" message.

Form validation is another case.  If you have multiple errors in a single form, we prefer to list all of them.  So if you screw up 4 times on a form, you may get 4 different error messages showing what you screwed up so you can fix it in one go instead of several.

Now sure, one could emulate that by building a multi-message layer on top of single-layer messages, but, really, why?  "One is a special case of many", and there are many many cases where you'll want to post multiple messages.  Like, most of Drupal. :-)

---------------------------------------------------------------------------

by lsmith77 at 2012-03-06T20:55:51Z

@fabpot is there any information you still need before merging this? do you want more discussion in which case you might want to take this to the mailing list ..

---------------------------------------------------------------------------

by drak at 2012-03-08T18:54:13Z

Another plus for this PR is that it requires no extra lines of code in templates etc to display the flashes, see https://github.com/symfony/symfony/pull/3267/files#diff-1

---------------------------------------------------------------------------

by drak at 2012-03-15T06:38:21Z

Rebased against current `master`, should be mergeable again..

---------------------------------------------------------------------------

by evillemez at 2012-03-17T03:08:41Z

+1 to this, I have an extended version of HttpFoundation just for this... would love to get rid of it.
09ab643
mmucklo mmucklo referenced this pull request from a commit
Fabien Potencier fabpot merged branch drak/session_refactor (PR #2853)
Commits
-------

cb6fdb1 [HttpFoundation] removed Session::close()
c59d880 Docblocks.
b8df162 Correct instanceof condition.
8a01dd5 renamed getFlashes() to getFlashBag() to avoid clashes
282d3ae updated CHANGELOG for 2.1
0f6c50a [HttpFoundation] added some method for a better BC
146a502 [FrameworkBundle] added some service aliases to avoid some BC breaks
93d81a1 [HttpFoundation] removed configuration for session storages in session.xml as we cannot provide a way to configure them (like before this PR anyway)
74ccf70 reverted 5b7ef11 (Simplify session storage class names now we have a separate namespace for sessions)
91f4f8a [HttpFoundation] changed default flash bag to auto-expires to keep BC
0494250 removed unused use statements
7878a0a [HttpFoundation] renamed pop() to all() and getAll() to all()
0d2745f [HttpFoundation] Remove constants from FlashBagInterface
dad60ef [HttpFoundation] Add back get defaults and small clean-up.
5b7ef11 [HttpFoundation] Simplify session storage class names now we have a separate namespace for sessions.
27530cb [HttpFoundation] Moved session related classes to own sub-namespace.
4683915 [HttpFoundation] Free bags from session storage and move classes to their own namespaces.
d64939a [DoctrineBridge] Refactored driver for changed interface.
f9951a3 Fixed formatting.
398acc9 [HttpFoundation] Reworked flashes to maintain same behaviour as in Symfony 2.0
f98f9ae [HttpFoundation] Refactor for DRY code.
9dd4dbe Documentation, changelogs and coding standards.
1ed6ee3 [DoctribeBridge][SecurityBundle][WebProfiler] Refactor code for HttpFoundation changes.
7aaf024 [FrameworkBundle] Refactored code for changes to HttpFoundation component.
669bc96 [HttpFoundation] Added pure Memcache, Memcached and Null storage drivers.
e185c8d [HttpFoundation] Refactored component for session workflow.
85b5c43 [HttpFoundation] Added drivers for PHP native session save handlers, files, sqlite, memcache and memcached.
57ef984 [HttpFoundation] Added unit and functional testing session storage objects.
3a263dc [HttpFoundation] Introduced session storage base class and interfaces.
c969423 [HttpFoundation] Added FlashBagInterface and concrete implementation.
39288bc [HttpFoundation] Added AttributesInterface and AttributesBagInterface and concrete implementations.

Discussion
----------

[2.1][HttpFoundation] Refactor session handling and flash messages

Bug fix: yes
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #2607, #2591, #2717, #2773
References the following tickets: #2592, #2543, #2541, #2510, #2714, #2684
Todo: -

__Introduction__

This extensive PR is a refactor with minimal BC breaks of the `[HttpFoundation]` component's session management which fixes several issues in the current implementation.  This PR includes all necessary changes to other bundles and components is documented in the `CHANGELOG-2.1` and `UPGRADING-2.1`.

__Summary of Changes__

__Session:__
  - Session object now implements `SessionInterface`

__Attributes:__
  - Attributes now handled by `AttributeBagInterface`
  - Added two AttributeBag implementations: `AttributeBag` replicates the current Symfony2 attributes behaviour, and the second, `NamespacedAttributeBag` introduces structured namespaced representation using '/' in the key.  Both are BC.  `FrameworkBundle` defaults to the old behaviour.

__Flash messages:__
  - Flash messages now handled by `FlashBagInterface`
  - Introduced `FlashBag` which changes the way flash messages expire, they now expire on use rather than automatically, useful for ESI.
  - Introduced `AutoExpireFlashBag` (default) which replicates the old automatic expiry behaviour of flash messages.

__Session Storage:__
  - Introduced a base object, `AbstractSessionStorage` for session storage drivers
  - Introduced a `SessionSaveHandlerInterface` when using custom session save handlers
  - Introduced a `NullSessionStorage` driver which allows for unsaved sessions
  - Introduced new session storage drivers for Memcache and Memcached
  - Introduced new session storage drivers for PHP's native SQLite, Memcache and Memcached support

__General:__
  - Fixed bugs where session attributes are not saved and all cases where flash messages would get lost
  - Wrote new tests, refactored related existing tests and increased test coverage extensively.

__Rationale/Details__

I'll explain more detail in the following sections.

__Unit Tests__

All unit and functional tests pass.

__Note on Functional Testing__

I've introduced `MockFileSessionStorage` which replaces `FilesystemSessionStorage` to emulate a PHP session for functional testing.  Essentially the same methodology of functional testing has been maintained but without interrupting the other session storage drivers interaction with real PHP sessions.  The service is now called `session.storage.mock_file`.

__Session Workflow__

PHP sessions follow a specific workflow which is not being followed by the current session management implementation and is responsible for some unpredictable bugs and behaviours.

Basically, PHP session workflow is as follows: `open`, `read`, `write`, `close`.  In between these can occur, `destroy` and `garbage collection`.  These actions are handled by `session save handlers` and one is always registered in all cases.  By default, the `files` save handler (internally to PHP) is registered by PHP at the start of code execution.

PHP offers the possibility to change the save handler to another internal type, for example one provided by a PHP extension (think SQLite, Memcache etc), or you can register a user type and set your own handlers.  However __in all cases__ PHP requires the handlers.

The handlers are called when the following things occur:

  - `open` and `read` when `session_start()` or the session autostarts when PHP outputs some display
  - `destroy` when `session_regenerate_id(true)` is called
  - `write` and `close` when PHP shuts down or when `session_write_close()` is called
  - `garbage collection` is called randomly according to configurable probability

The next very important aspect of this PR is that `$_SESSION` plays an important part in this workflow because the contents of the $_SESSION is populated __after__ the `read` handler returns the previously saved serialised session data.  The `write` handler is sent the serialised `$_SESSION` contents for save.  Please note the serialisation is a different format to `serialize()`.

For this reason, any session implementation cannot get rid of using `$_SESSION`.

I wrote more details on this issue [here](#2607 (comment))

In order to make writing session storage drivers simple, I created a light base class `AbstractSessionStorage` and the `SessionSaveHandlerInterface` which allows you to quickly write native and custom save handler drivers.

__Flash Messages [BC BREAK]__

Flash messages currently allow representation of a single message per `$name`.  Fabien designed the original system so that `$name` was equivalent to flash message type.  The current PR changes the fact that Flash messages expire explicitly when retrieved for display to the user as opposed to immediately on the next page load.

The last issue fixes potential cases when flash messages are lost due to an unexpected intervening page-load (an error for example).  The API `get()` has a flag which allows you to override the `clear()` action.

__Flash message translation__

This PR does not cover translation of flash messages because  messages should be translated before calling the flash message API.  This is because flash messages are used to present messages to the user after a specific action, and in any case, immediately on the next page load.  Since we know the locale of the request in every case we can translate the message before storing.  Secondly, translation is simply a string manipulation.  Translation API calls should always have the raw untranslated string present because it allows for extraction of translation catalogs.  For a complete answer see my answer [here](#2543 (comment))

__Session attribute and structured namespacing__

__This has been implemented without changing the current default behaviour__ but details are below for the alternative:

Attributes are currently stored in a flat array which limits the potential of session attributes:

Here are some examples to see why this 'structured namespace' methodology is extremely convenient over using a flat system.  Let's look at an example with csrf tokens.  Let's say we have multiple csrftokens stored by form ID (allowing multiple forms on the page and tabbed browsing).

If we're using a flat system, you might have

    'tokens' => array('a' => 'a6c1e0b6',
                      'b' => 'f4a7b1f3')

With a flat system when you get the key `tokens`, you will get back an array, so now you have to analyse the array.  So if you simply want to add another token, you have to follow three steps: get the session attribute `tokens`, have to add to the array, and lastly set the entire array back to the session.

    $tokens = $session->get('tokens');
    $tokens['c'] = $value;
    $session->set('tokens', $tokens);

Doable, but you can see it's pretty long winded.

With structured namespacing you can simply do:

    $session->set('c', $value, '/tokens');

There are several ways to implement this, either with an additional `$namespace` argument, or by treating a character in the `$key` as a namespacer.  `NamespacedAttributeBag` treats `/` as a namespacer so you can represent `user.tokens/a` for example.  The namespace character is configurable in `NamespacedAttributeBag`.

---------------------------------------------------------------------------

by marijn at 2011-12-18T15:43:17Z

I haven't read the code yet but the description from this PR and your line of thought seem very well structured.
Seems like a big +1 for me.

---------------------------------------------------------------------------

by lsmith77 at 2011-12-19T16:01:19Z

@deviantintegral could you look over this to see if it really addresses everything you wanted with PR #2510 ?

---------------------------------------------------------------------------

by deviantintegral at 2011-12-24T20:12:03Z

I've read through the documentation and upgrade notes, and I can't see anything that's obviously missing from #2510. Being able to support multiple flashes per type is the most important, and the API looks reasonable to me. Drupal does support supressing repeat messages, but that can easily be implemented in our code unless there's a compelling case for it to be a part of Symfony.

I wonder if PHP memcache support is required in Symfony given the availability of memcached. I'm not familiar with how other parts of Symfony handle it, but there is often quite a bit of confusion between the two PHP extensions. It could be simpler to remove one, or add a bit of info describing or linking to why there are two nearly identical classes.

Is it possible to make one class inherit from the other (memcached is a child of memcache)?

---------------------------------------------------------------------------

by Fristi at 2011-12-24T20:29:46Z

Interesting, maybe add: session events as I did with the current impl: https://github.com/Fristi/SessionBundle

---------------------------------------------------------------------------

by drak at 2011-12-25T00:50:03Z

@deviantintegral - I agree about the confusion between memcache and memcached but actually, it is necessary to support both because `memcached` is not available everywhere.  For example on Debian Lenny and RHEL/CentOS 5, only memcache is available by default.  This would preclude a massive amount of shared hosting environments.  Also, it is not possible to inherit one from the other, they are completely different drivers.

@Fristi - I also thought about the events, but they do not belong as part of the standalone component as this would create a coupling to the event dispatcher.  The way you have done it, ie, in a bundle is the right way to achieve it.

---------------------------------------------------------------------------

by matheo at 2011-12-25T01:12:00Z

Impressive work, looks like a big improvement and deserves a big +1

---------------------------------------------------------------------------

by datiecher at 2011-12-26T11:57:12Z

Took some time to grok all the changes in this PR but all in all it is a keeper. Specially the new flash message API, it's really nicer to work with it then the previous one.

Nicely done @drak!

---------------------------------------------------------------------------

by lsmith77 at 2012-01-02T15:00:00Z

@fabpot did you have time to review this yet? with all the work @drak has done its important that he gets some feedback soon. its clear this PR breaks BC in ways we never wanted to allow. but i think this PR also clearly explains why its necessary none the less.

---------------------------------------------------------------------------

by drak at 2012-01-02T15:41:53Z

@fabpot - I have removed the WIP status from this PR now and rebased against the current master branch.

---------------------------------------------------------------------------

by Tobion at 2012-01-07T07:13:38Z

From what I read from the IRC chat logs, the main concern of @fabpot is whether we really need multiple flash messages per type. I'm in favor of this PR and just want to add one point to this discussion.
At the moment you can add multiple flash messages of different type/category/identifier. For example you can specify one error message and one info message after an operation. I think most agree that this can be usefull.
But then it makes semantically no sense that you currently cannot add 2 info messages. This approach feels a bit half-done.
So I think this PR eliminates this paradox.

---------------------------------------------------------------------------

by drak at 2012-01-07T09:11:07Z

For reference there is also a discussion started by @lsmith77 on the mailing list at https://groups.google.com/forum/#!topic/symfony-devs/cy4wokD0mQI

---------------------------------------------------------------------------

by dlsniper at 2012-01-07T16:02:15Z

@drak I could also add the next scenario that I currently have to live with, in addition to @lsmith77 ones.

I had this issue while working on our shopping cart implementation for a customer where the customer wanted to show the unavailability of the items as different lines in the 'flash-error' section of the cart. We had to set an array as the 'flash message' in order to display that information.

So in this case for example having the flash messages types as array would actually make more sense that sending an array to the flasher. Plus the the other issue we had was that we also wanted to add another error in the message but we had to do a check to see if the flash message is an array already or we need to make it an array.

I think it's better not to impose a limit of this sort and let the users be able to handle every scenario, even if some are rare, rather that forcing users to overcome limitations such as these.

I really hope this PR gets approved faster and thanks everyone for their hard work :)

---------------------------------------------------------------------------

by Tobion at 2012-01-07T21:01:07Z

@dlsniper I think you misinterpreted my point.

---------------------------------------------------------------------------

by dlsniper at 2012-01-07T21:04:04Z

@Tobion I'm sorry I did that, I'll edit the message asap. Seems no sleep in 26 hours can cause brain not to function as intended :)

---------------------------------------------------------------------------

by lsmith77 at 2012-02-01T14:38:52Z

FYI the drupal guys are liking this PR (including the flash changes):
http://drupal.org/node/335411

---------------------------------------------------------------------------

by drak at 2012-02-01T14:51:33Z

@lsmith77 Fabien asked me to remove the changes to the flash messages so that they are as before - i.e. only one flash per name/type /cc @fabpot

---------------------------------------------------------------------------

by fabpot at 2012-02-01T14:58:23Z

To be clear, I've asked to split this PR in two parts:

 * one about the session refactoring (which is non-controversial and should be merged ASAP)
 * this one with only the flash refactoring

---------------------------------------------------------------------------

by drak at 2012-02-02T11:29:26Z

@fabpot this is ready to be merged now.  I will open a separate PR later today for the flash messages as a bucket.

---------------------------------------------------------------------------

by fabpot at 2012-02-02T11:34:39Z

I must have missed something, but I still see a lot of changes related to the flash messages.

---------------------------------------------------------------------------

by drak at 2012-02-02T11:39:10Z

When I spoke to you you said you wanted to make the commit with flash messages with one message per name/type rather than multiple.  The old flash messages behaviour is 100% maintained in `AutoExpireFlashBag` which can be the default in the framework if you wish.  The `FlashBag` implementation makes Symfony2 ESI compatible.

---------------------------------------------------------------------------

by stof at 2012-02-02T11:47:38Z

@drak splitting into 2 PRs means you should not refactor the flash messages in this one but in the dedicated one.

---------------------------------------------------------------------------

by drak at 2012-02-02T12:29:43Z

@stof Yes. I discussed with Fabien over chat there are basically no changes
to flashes in `FlashBag` and `AutoExpireFlashBag` maintains the exact
behaviour as before.  The FlashBag just introduces ESI compatible flashes.
 There is no way to refactor the sessions without moving the flash messages
to their own bag.  The next PR will propose the changes to flashes that
allow multiple messages per name/type.  I can size the PR down a little
more removing the new storage drivers and so on to make the PR smaller but
that's really as far as I can go.  To be clear, while the API has changed a
little for flashes, the behaviour is the same.
24dc082
mmucklo mmucklo referenced this pull request from a commit
Fabien Potencier fabpot merged branch drak/session_flashmessages (PR #3267)
Commits
-------

5ae76f1 [HttpFoundation] Update documentation.
910b5c7 [HttpFoudation] CS, more tests and some optimization.
b0466e8 [HttpFoundation] Refactored BC Session class methods.
84c2e3c [HttpFoundation] Allow flash messages to have multiple messages per type.

Discussion
----------

[2.1][HttpFoundation] Multiple session flash messages

Bug fix: no
Feature addition: yes
Backwards compatibility break: yes, but this already happened in #2583.  BC `Session` methods remain unbroken.
Symfony2 tests pass: yes
Fixes the following tickets: #1863
References the following tickets: #2714, #2753, #2510, #2543, #2853
Todo: -

This PR alters flash messages so that it is possible to store more than one message per flash type using the `add()` method or by passing an array of messages to `set()`.

__NOTES ABOUT BC__

This PR maintains BC behaviour with the `Session` class in that the old Symfony 2.0 methods will continue to work as before.

---------------------------------------------------------------------------

by drak at 2012-02-13T06:28:33Z

I think this is ready for review @fabpot @lsmith77

---------------------------------------------------------------------------

by lsmith77 at 2012-02-14T19:30:39Z

the FlashBag vs. AutoExpireFlashBag behavior and setup difference should probably also be explained in the upgrading log

---------------------------------------------------------------------------

by drak at 2012-02-15T04:43:14Z

@lsmith77 Those differences are explained already in the changelog

 * Added `FlashBag`. Flashes expire when retrieved by `get()` or `all()`.
   This makes the implementation ESI compatible.
 * Added `AutoExpireFlashBag` (default) to replicate Symfony 2.0.x auto expire behaviour of messages auto expiring
   after one page page load.  Messages must be retrived by `get()` or `all()`.

---------------------------------------------------------------------------

by Crell at 2012-02-19T17:35:34Z

Drak asked me to weigh in here with use cases.  Drupal currently has a similar session-stored-messaging system in place that I'd like to be able to replace with Flash messages.  We frequently have multiple messages within a single request, however, so this change is critical to our being able to do so.

For instance, when saving an article in Drupal there is, by default, a "yay, you saved an article!" type message that gets displayed.  If you also have the site configured to send email when a post is updated, you may see a "email notifications sent" message (depending on your access level).  If you have a Solr server setup for search, and you're in debug mode, there will also be a "record ID X added to Solr, it should update in 2 minutes" message.  And if there's a bug somewhere, you'll also get, as an error message rather than notice message, a "Oops, E_NOTICE on line 54" message.

Form validation is another case.  If you have multiple errors in a single form, we prefer to list all of them.  So if you screw up 4 times on a form, you may get 4 different error messages showing what you screwed up so you can fix it in one go instead of several.

Now sure, one could emulate that by building a multi-message layer on top of single-layer messages, but, really, why?  "One is a special case of many", and there are many many cases where you'll want to post multiple messages.  Like, most of Drupal. :-)

---------------------------------------------------------------------------

by lsmith77 at 2012-03-06T20:55:51Z

@fabpot is there any information you still need before merging this? do you want more discussion in which case you might want to take this to the mailing list ..

---------------------------------------------------------------------------

by drak at 2012-03-08T18:54:13Z

Another plus for this PR is that it requires no extra lines of code in templates etc to display the flashes, see https://github.com/symfony/symfony/pull/3267/files#diff-1

---------------------------------------------------------------------------

by drak at 2012-03-15T06:38:21Z

Rebased against current `master`, should be mergeable again..

---------------------------------------------------------------------------

by evillemez at 2012-03-17T03:08:41Z

+1 to this, I have an extended version of HttpFoundation just for this... would love to get rid of it.
b6677f2
Daniel Platt hackzilla referenced this pull request from a commit
Fabien Potencier fabpot merged branch drak/session_flashmessages (PR #3267)
Commits
-------

5ae76f1 [HttpFoundation] Update documentation.
910b5c7 [HttpFoudation] CS, more tests and some optimization.
b0466e8 [HttpFoundation] Refactored BC Session class methods.
84c2e3c [HttpFoundation] Allow flash messages to have multiple messages per type.

Discussion
----------

[2.1][HttpFoundation] Multiple session flash messages

Bug fix: no
Feature addition: yes
Backwards compatibility break: yes, but this already happened in #2583.  BC `Session` methods remain unbroken.
Symfony2 tests pass: yes
Fixes the following tickets: #1863
References the following tickets: #2714, #2753, #2510, #2543, #2853
Todo: -

This PR alters flash messages so that it is possible to store more than one message per flash type using the `add()` method or by passing an array of messages to `set()`.

__NOTES ABOUT BC__

This PR maintains BC behaviour with the `Session` class in that the old Symfony 2.0 methods will continue to work as before.

---------------------------------------------------------------------------

by drak at 2012-02-13T06:28:33Z

I think this is ready for review @fabpot @lsmith77

---------------------------------------------------------------------------

by lsmith77 at 2012-02-14T19:30:39Z

the FlashBag vs. AutoExpireFlashBag behavior and setup difference should probably also be explained in the upgrading log

---------------------------------------------------------------------------

by drak at 2012-02-15T04:43:14Z

@lsmith77 Those differences are explained already in the changelog

 * Added `FlashBag`. Flashes expire when retrieved by `get()` or `all()`.
   This makes the implementation ESI compatible.
 * Added `AutoExpireFlashBag` (default) to replicate Symfony 2.0.x auto expire behaviour of messages auto expiring
   after one page page load.  Messages must be retrived by `get()` or `all()`.

---------------------------------------------------------------------------

by Crell at 2012-02-19T17:35:34Z

Drak asked me to weigh in here with use cases.  Drupal currently has a similar session-stored-messaging system in place that I'd like to be able to replace with Flash messages.  We frequently have multiple messages within a single request, however, so this change is critical to our being able to do so.

For instance, when saving an article in Drupal there is, by default, a "yay, you saved an article!" type message that gets displayed.  If you also have the site configured to send email when a post is updated, you may see a "email notifications sent" message (depending on your access level).  If you have a Solr server setup for search, and you're in debug mode, there will also be a "record ID X added to Solr, it should update in 2 minutes" message.  And if there's a bug somewhere, you'll also get, as an error message rather than notice message, a "Oops, E_NOTICE on line 54" message.

Form validation is another case.  If you have multiple errors in a single form, we prefer to list all of them.  So if you screw up 4 times on a form, you may get 4 different error messages showing what you screwed up so you can fix it in one go instead of several.

Now sure, one could emulate that by building a multi-message layer on top of single-layer messages, but, really, why?  "One is a special case of many", and there are many many cases where you'll want to post multiple messages.  Like, most of Drupal. :-)

---------------------------------------------------------------------------

by lsmith77 at 2012-03-06T20:55:51Z

@fabpot is there any information you still need before merging this? do you want more discussion in which case you might want to take this to the mailing list ..

---------------------------------------------------------------------------

by drak at 2012-03-08T18:54:13Z

Another plus for this PR is that it requires no extra lines of code in templates etc to display the flashes, see https://github.com/symfony/symfony/pull/3267/files#diff-1

---------------------------------------------------------------------------

by drak at 2012-03-15T06:38:21Z

Rebased against current `master`, should be mergeable again..

---------------------------------------------------------------------------

by evillemez at 2012-03-17T03:08:41Z

+1 to this, I have an extended version of HttpFoundation just for this... would love to get rid of it.
3380153
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 11, 2012
  1. Drak

    [HttpFoundation] Added AttributesInterface and AttributesBagInterface…

    drak authored
    … and concrete implementations.
    
    This commit outsources session attribute storage to it's own class.
    There are two concrete implementations, one with structured namespace storage and the other
    without.
  2. Drak

    [HttpFoundation] Added FlashBagInterface and concrete implementation.

    drak authored
    This commit outsources the flash message processing to it's own interface.
    
    Overall flash messages now can have multiple flash types and each type can
    store multiple messages.  For convenience there are now four flash types
    by default, INFO, NOTICE, WARNING and ERROR.
    
    There are two concrete implementations: one preserving the old behaviour of
    flash messages expiring exactly after one page load, regardless of being
    displayed or not; and the other where flash messages persist until explicitly
    popped.
  3. Drak

    [HttpFoundation] Introduced session storage base class and interfaces.

    drak authored
    Session object now implements SessionInterface to make it more portable.
    
    AbstractSessionStorage and SessionSaveHandlerInterface now makes implementation
    of session storage drivers simple and easy to write for both custom save handlers
    and native php save handlers and respect the PHP session workflow.
  4. Drak
  5. Drak

    [HttpFoundation] Added drivers for PHP native session save handlers, …

    drak authored
    …files, sqlite, memcache and memcached.
  6. Drak
  7. Drak
  8. Drak

    [FrameworkBundle] Refactored code for changes to HttpFoundation compo…

    drak authored
    …nent.
    
    Native PHP sessions stored to file are done with session.storage.native_file
    Functional testing is done with session.storage.mock_file
    
    Default flash message implementation done with FlashBag (session.flash_bag)
    Default attribute storage implementation with AttributeBag (session.attribute_bag)
    
    Services added: session.storage.native_file, session.storage.native_memcache, session.storage.native_memcache,
    session.storage.native_sqlite, session.storage.memcache, session.storage.memcached, session.storage.null,
    session.storage.mock_file, session.flash_bag, session.attribute_bag
    
    Services removed: session.storage.native, session.storage.filesystem
  9. Drak
  10. Drak
  11. Drak

    [HttpFoundation] Refactor for DRY code.

    drak authored
    Rename ArraySessionStorage to make it clear the session is a mock for testing purposes only.
    Has BC class for ArraySessionStorage
    Added sanity check when starting the session.
    Fixed typos and incorrect php extension test method
    session_module_name() also sets session.save_handler, so must use extension_loaded() to check if module exist
    or not.
    Respect autostart settings.
  12. Drak
  13. Drak

    Fixed formatting.

    drak authored
  14. Drak
  15. Drak
  16. Drak
  17. Drak

    [HttpFoundation] Simplify session storage class names now we have a s…

    drak authored
    …eparate namespace for sessions.
  18. Drak

    [HttpFoundation] Add back get defaults and small clean-up.

    drak authored
    Changed read-only method names from get*() to peek*()
    
    Typo
  19. Drak

    [HttpFoundation] Remove constants from FlashBagInterface

    drak authored
    As requested by fabpot.
    Corrected a few mistakes in the documentation.
  20. Fabien Potencier
  21. Fabien Potencier

    removed unused use statements

    fabpot authored
  22. Fabien Potencier
  23. Fabien Potencier

    reverted 5b7ef11 (Simplify session

    fabpot authored
    storage class names now we have a separate namespace for sessions)
  24. Fabien Potencier

    [HttpFoundation] removed configuration for session storages in sessio…

    fabpot authored
    …n.xml as we cannot provide a way to configure them (like before this PR anyway)
  25. Fabien Potencier
  26. Fabien Potencier
  27. Fabien Potencier

    updated CHANGELOG for 2.1

    fabpot authored
  28. Fabien Potencier
  29. Drak

    Correct instanceof condition.

    drak authored
  30. Drak

    Docblocks.

    drak authored
  31. Fabien Potencier
Something went wrong with that request. Please try again.