Skip to content
This repository

[WIP][HttpFoundation] Move flash messages out of Session class and change to bucket system. #2592

Closed
wants to merge 5 commits into from

8 participants

Drak Christophe Coevoet Fabien Potencier Lukas Kahwe Smith Marijn Huizendveld Oleg Stepura Andrew Berry Henrik Bjørnskov
Drak
Collaborator

Bug fix: no
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: TODO
Fixes the following tickets: #1863
References: #2510, #2543
Todo: write tests, update changelog, update documentation

The main rationale for this PR can be found at #2543 (comment)

Summary: In this PR we move the flash messages to it's own driver, and we change flash messages to a bucket system. This PR clarifies Fabien's original intention of using $name as a type category for the flash message, but it's now extended to allow multiple messages per type. Renamed $name to $type for clarity.

Lukas Kahwe Smith

big question is if there should be some "default" types provided as class constants

You need to check if $this->flashes[$type] is initialized as an array instead of just assuming it.

Collaborator

PHP doesn't throw a notice in this case, it just populates the array. Try with this test code:

error_reporting(E_ALL);
$flashes = array();
$flashes['foo'][] = 'bar';

Still it should be explicit and is done everywhere else where it is needed.

Collaborator

Checks are necessary in a read context because otherwise PHP will create a key and then return null. PHP throws a notice to warn you it has done this because we need to know the array key was not initialised and could be the cause of an error.

In a write context, PHP creates the key and writes the value. No notice is emitted by PHP in the write context because we (and PHP both) expect the key to be created.

Still should be explicit as it is in other places of the Symfony code.

indeed we usually make sure that arrays are initialized as such.

ok .. checked for similar places.
so when we have a one level array like the following we initialize for obvious reasons:
$array[] = 'foo';

However in the case as above where we have a subelement we indeed do not seem to initialize the subelements:
https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/AddCacheWarmerPass.php#L37
https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/FormPass.php#L40

Collaborator

Please grep the entire Symfony codebase for ][] - you will see this write context used everywhere.

I'm just to point out a fact of how the PHP engine works and why E_NOTICES are emitted. Defensive coding is essential, but isset/array_key_exists checks are for read context where you are asking for a value that might not actually exist. E_NOTICES are emitted when PHP has to create a variable or array key that doesnt exist when it's asked to read that value. See the examples below:

echo $a; // E_NOTICE - because no $a exists (read context)
$a = 0; // no E_NOTICE - because we are telling PHP to create $a

$a = array();
echo $a['hello']; // E_NOTICE - because no 'hello' key exists (read context)

$a = array();
$a['hello'] = 'world'; // no E_NOTICE because we are telling PHP to create this key.
$a['hello']['again'] = 'world'; // no E_NOTICE because we are telling PHP to create this structure

We must always use isset() checks, not specifically to avoid the E_NOTICE, but to avoid the issues behind them, i.e. we cannot read what doesnt exist and in order to continue execution, PHP creates the value for us and let's us know it did so.

Beyond this, PHP doesnt have very strict typing anyhow - I mean, if we really want to get into details here, when we expect a string, we should be testing $value and $type are strings and throwing an exception if not, simply because PHP doesnt have a string typecasting available in the method signature.

The following isset check is completely unnecessary since PHP creates this automatically and is expected behaviour - that is why no E_NOTICE is emitted and there for, there are no issues we need to trap.

public function add($type, $message)
{
    if (!isset($this->flashes[$type]) {
        $this->flashes[$type] = array();
    }
    $this->flashes[$type][] = $message;

FYI .. my findings are supporting your point of view @drak :)

src/Symfony/Component/HttpFoundation/FlashBagInterface.php
((9 lines not shown))
  9
+ * file that was distributed with this source code.
  10
+ */
  11
+
  12
+namespace Symfony\Component\HttpFoundation;
  13
+
  14
+/**
  15
+ * 
  16
+ */
  17
+interface FlashBagInterface
  18
+{
  19
+    /**
  20
+     * Initializes the FlashBag.
  21
+     * 
  22
+     * @param array $flashes 
  23
+     */
  24
+    public function initialize(array $flashes);
1
Christophe Coevoet Collaborator
stof added a note November 09, 2011

the public keyword should be omitted for interfaces

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

It makes for a much better API if developers don't have to know the internal of the class, ie, to know that they should be using 'status' or something as keys - that's where class constants are useful. status and error could be sensible defaults. I wont voice a strong opinion about this because my main concern is that Symfony2 session handling has flexible message handling as discussed in 2543 comment-2654648

Should I continue and flesh out some unit tests?

src/Symfony/Component/HttpFoundation/Session.php
((12 lines not shown))
30 35
     protected $closed;
31  
-
  36
+    
6
Christophe Coevoet Collaborator
stof added a note November 09, 2011

you should not add trailing slashes here

Drak Collaborator
drak added a note November 09, 2011

I dont see any trailing slashes, what am I missing? I think my IDE must have truncated the line or changed the line ending?

Christophe Coevoet Collaborator
stof added a note November 09, 2011

sorry, not slashes but spaces

Christophe Coevoet Collaborator
stof added a note November 09, 2011

and it applies to all the blank lines of your patch as most of them are not really blank lines.

Drak Collaborator
drak added a note November 09, 2011

I think this is my IDE stripping spaces from empty lines.

Christophe Coevoet Collaborator
stof added a note November 09, 2011

no, it is your IDE adding them. Stripping them is what you should do to follow the coding standards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/HttpFoundation/Session.php
((21 lines not shown))
36 42
      */
37  
-    public function __construct(SessionStorageInterface $storage)
  43
+    public function __construct(SessionStorageInterface $storage, FlashBagInterface $flashes)
2
Christophe Coevoet Collaborator
stof added a note November 09, 2011

we may want to make the FlashBag optionnal here for people that don't need it. @fabpot thoughts ?

Drak Collaborator
drak added a note November 09, 2011

Sure, we could default it to FlashBagInterface $flashes = null, and then throw an exception in getFlashes() if that's preferred.

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

@drak I think this change makes the API cleaner. So I would vote :+1:

The issue is that it break BC and the component was announced as being stable so we need @fabpot approval here. but you can start writing tests as they would be needed before merging the PR anyway.

Fabien Potencier
Owner

I like the move of flash management outside of the session, even if we keep the current API.

But as @lsmith77 suggested in the other PR about the same topic, we first need to define the use cases for flashes (and which problems it solve?) to be sure that we are all on the same page. As of today, I still don't understand why you would want to store more than one thing for a given flash name.

I think everybody has the same technical definition: A flash is something that is stored to be displayed on the very next request and must be removed automatically after that.

But beside this technical definition, when is it useful?

Here is my intended use case (actually it's not mine, it's the one envisioned by Ruby on Rails which AFAIK introduced this concept some years ago.): forms submission.

When a user submit a form, you want to display the result of the submission on the next page (this is needed because you redirect to a GET after a POST instead of displaying the result directly in response to the form submission). And that's it. Nothing more. For this use case, you don't need an array as you only have one result to display and you can only submit one form per request.

Lukas Kahwe Smith
Marijn Huizendveld

I'm sorry @lsmith77 but the use cases you describe sound more like a notification center to me. Agreed that flash messages should be a part of it, it's a rather broad spectrum approach and I'm not sure that it should be coupled to the session. Rather, the session should be able to send notifications, some of which could be sticky and some that would act like flashes.

Anyway, I'm not sure we need that in the core.

Lukas Kahwe Smith

Sure you can argue that it shouldn't be in core.
However we need the common infrastructure in core:
1) make it extensible (which this PR does)
2) define a set of standard "types"

Once that is in place, people can use the flash messages just like they did before (more or less since they now would get an array for each key). Not sure if it would make sense to have a FlashBag with the old behavior and a FlashStackBag with the new, since it wouldn't be possible to have the same API for reading.

Marijn Huizendveld

I guess I didn't express myself clearly, my apologies.

Aside from the fact what the API for the new flash messages should look like, I agree with @fabpot that we need to define some use cases first before deciding upon the API. In my opinion your use cases are not completely relevant to the topic.

As much as I like and agree with the need for your more extensive approach to user notifications (like you described, a system that allows for event listeners and cross-application notifying), I think the responsibility for such a behavior is not the within the scope of a flash messages API. Therefor we should probably think of other possible use cases where multiple messages are indeed possible.

Lukas Kahwe Smith

Even if you ignore the use case for the listener, the rest of my use case is standard and should definitely be handled by core imho.

Drak
Collaborator

There are many use cases when you develop complex applications. I can give you examples from Zikula. You might have an event handler (from a plugin or something) that needs to communicate something important to the user (a security warning for example). Flashes provide a perfect way to do this.

In another example we have a system of installable modules. The list of modules needs to be regenerated so we can provide a list of installed and installable modules. It might be someone installs an invalid module type, so during the regenerate loop, we can throw up an error like Invalid module type for $foo, since this is a loop, there might be multiple instances of this.

There are also tasks which a controller might call APIs or trigger hooks/events that have no way of communicating an error to the user other than by flashes.

You can argue that this should be done another way (collect API responses/trap exceptions and build a single flash in the controller), but the fact is, flashes provide a very convenient way of dealing with this. Several big systems use this kind of messaging:, Drupal is one of them. We cannot expect people to buy into Symfony2 components if they have to spend their time customising them for basic functionality which is expected and used extensively elsewhere.

As for a standard for flash message types, I'd say there are two options, 2, or 4. STATUS/ERROR, or INFO, STATUS, WARNING, ERROR. Using constants is good programming practice. Afterall, processing of these will be up to the one implementing the view.

Oleg Stepura

As for me personally - I had to implement my own Messenger class just to be able to add flush messages of different kind (error, notices) in the amount of more than 1 message. The PR seem to be a good alternative.

Drak
Collaborator

I have one caveat for defaults - it's bad practice to add defaults to a method signature if arguments after that are required

e.g. This sort of thing is bad practice:

function foo($name = 'default', $value)

Given the addFlash() method has ($type, $value) as the signature, I think we should not add a default to $type. We could still add a class constant though.

Lukas Kahwe Smith

agreed. but imho we should offer some default type strings as class constants

Oleg Stepura

@drak, what is wrong with changing the order of arguments? addFlash($value, $type = 'default')

Drak
Collaborator

@olegstepura: I don't know, is it logical to change the order? My own preference is to have $type first because it's explicit, if we change it then all other methods would have to be changed too and they look weird to me.

Oleg Stepura

@drak, it's logical to add the defaults. I'm lazy enough to add a simple info flush message without any extra arguments via addFlash('hello') and add an extra argument for critical messages which have to be displayed on client side with extra formatting (in red), eg. addFlush('Ahtung!', FlashBag::CRITICAL).

I am talking about API, not implementation details

And what's the case of not adding a default? Only the arguments order? the only bad example of moving !optional arguments which can have defaults to the end was new ZF2 Cache PR when adding to cache was made by add($data, $key = 'some defaults') which is not intuitive. This class is not the case.

Andrew Berry

A few notes after reading through this.

I think everybody has the same technical definition: A flash is something that is stored to be displayed on the very next request and must be removed automatically after that.
(this is needed because you redirect to a GET after a POST instead of displaying the result directly in response to the form submission)

In Drupal this is not the only case for session messages. We use the same interface for showing form validation errors, which are returned in the POST response with the rebuilt form. We also only do one POST request for each step in a multistep form.

As for a standard for flash message types, I'd say there are two options, 2, or 4. STATUS/ERROR, or INFO, STATUS, WARNING, ERROR. Using constants is good programming practice.

In Drupal, we combine the info and status levels, but as long as Symfony offers at least those three levels we'll be able to use this component.

Andrew Berry deviantintegral commented on the diff November 13, 2011
src/Symfony/Component/HttpFoundation/FlashBag.php
((40 lines not shown))
  40
+     * 
  41
+     * @param array $flashes 
  42
+     */
  43
+    public function initialize(array $flashes)
  44
+    {
  45
+        if ($this->initialized) {
  46
+            return;
  47
+        }
  48
+        
  49
+        $this->flashes = $flashes;
  50
+        $this->oldFlashes = $flashes;
  51
+        $this->initialized = true;
  52
+    }
  53
+
  54
+    /**
  55
+     * Adds a flash to the stack for a given type.
1

This comment is confusing as we aren't actually storing messages in a stack and they can be retrieved by index.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Andrew Berry deviantintegral commented on the diff November 13, 2011
src/Symfony/Component/HttpFoundation/FlashBagInterface.php
((14 lines not shown))
  14
+/**
  15
+ * FlashBagInterface.
  16
+ * 
  17
+ * @author Drak <drak@zikula.org>
  18
+ */
  19
+interface FlashBagInterface
  20
+{
  21
+    /**
  22
+     * Initializes the FlashBag.
  23
+     * 
  24
+     * @param array $flashes 
  25
+     */
  26
+    function initialize(array $flashes);
  27
+
  28
+    /**
  29
+     * Adds a flash to the stack for a given type.
1

Same thing here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Andrew Berry deviantintegral commented on the diff November 13, 2011
src/Symfony/Component/HttpFoundation/FlashBagInterface.php
((34 lines not shown))
  34
+     * Gets flash messages for a given type.
  35
+     * 
  36
+     * @return array
  37
+     */
  38
+    function get($type);
  39
+    
  40
+    /**
  41
+     * Sets an array of flash messages for a given type.
  42
+     * 
  43
+     * @param string $type
  44
+     * @param array  $array 
  45
+     */
  46
+    function set($type, array $array);
  47
+    
  48
+    /**
  49
+     * Hass flash messages for a given type?
1

Typo here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/Symfony/Component/HttpFoundation/Session.php
((22 lines not shown))
36 42
      */
37  
-    public function __construct(SessionStorageInterface $storage)
  43
+    public function __construct(SessionStorageInterface $storage, FlashBagInterface $flashBag = null)
1

This indicates that flashBag's are optional, but it's not clear what the expected behaviour is if it's null. The rest of the code appears to assume that flashBag is always set, and I don't see any guards or exceptions thrown.

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

FYI - I'm going AFK for about 5 or so days, I will try to work more on this offline and update you when I get back.

Drak [HttpFoundation] Added some sensible defaults for flash types.
Changed the order of addFlash() a sensible default.
91342f7
Drak drak closed this November 27, 2011
Drak
Collaborator

Replaced by PR #2714

Fabien Potencier fabpot referenced this pull request from a commit February 11, 2012
Fabien Potencier 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
mmucklo mmucklo referenced this pull request from a commit February 11, 2012
Fabien Potencier 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
1  .gitignore
... ...
@@ -1,3 +1,4 @@
1 1
 phpunit.xml
2 2
 autoload.php
3 3
 /vendor/
  4
+/nbproject/private/
4  CHANGELOG-2.1.md
Source Rendered
@@ -88,6 +88,8 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
88 88
  * removed the ContentTypeMimeTypeGuesser class as it is deprecated and never used on PHP 5.3
89 89
  * added ResponseHeaderBag::makeDisposition() (implements RFC 6266)
90 90
  * made mimetype to extension conversion configurable
  91
+ * [BC BREAK] Flashes are now stored as a bucket of messages per $type. Moved flash messages 
  92
+   out of the session class.  Must use $session->getFlashBag() to get FlashBag instance.
91 93
 
92 94
 ### HttpKernel
93 95
 
@@ -117,4 +119,4 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
117 119
  * added a Size validator
118 120
  * added a SizeLength validator
119 121
  * improved the ImageValidator with min width, max width, min height, and max height constraints
120  
- * added support for MIME with wildcard in FileValidator
  122
+ * added support for MIME with wildcard in FileValidator
156  src/Symfony/Component/HttpFoundation/FlashBag.php
... ...
@@ -0,0 +1,156 @@
  1
+<?php
  2
+
  3
+/*
  4
+ * This file is part of the Symfony package.
  5
+ *
  6
+ * (c) Fabien Potencier <fabien@symfony.com>
  7
+ *
  8
+ * For the full copyright and license information, please view the LICENSE
  9
+ * file that was distributed with this source code.
  10
+ */
  11
+
  12
+namespace Symfony\Component\HttpFoundation;
  13
+
  14
+/**
  15
+ * FlashBag flash message container.
  16
+ */
  17
+class FlashBag implements FlashBagInterface
  18
+{
  19
+    const STATUS = 'status';
  20
+    const ERROR = 'error';
  21
+    
  22
+    /**
  23
+     * Flash messages.
  24
+     * 
  25
+     * @var array
  26
+     */
  27
+    private $flashes = array();
  28
+    
  29
+    /**
  30
+     * Old flash messages to be purged.
  31
+     * 
  32
+     * @var array
  33
+     */
  34
+    private $oldFlashes = array();
  35
+    
  36
+    /**
  37
+     * @var boolean
  38
+     */
  39
+    private $initialized = false;
  40
+    
  41
+    /**
  42
+     * Initializes the FlashBag.
  43
+     * 
  44
+     * @param array $flashes 
  45
+     */
  46
+    public function initialize(array $flashes)
  47
+    {
  48
+        if ($this->initialized) {
  49
+            return;
  50
+        }
  51
+        
  52
+        $this->flashes = $flashes;
  53
+        $this->oldFlashes = $flashes;
  54
+        $this->initialized = true;
  55
+    }
  56
+
  57
+    /**
  58
+     * Adds a flash to the stack for a given type.
  59
+     * 
  60
+     * @param string $message Message.
  61
+     * @param string $type    Message category
  62
+     */
  63
+    public function add($message, $type = self::STATUS)
  64
+    {
  65
+        $this->flashes[$type][] = $message;
  66
+    }
  67
+    
  68
+    /**
  69
+     * Gets flashes for a given type.
  70
+     * 
  71
+     * @return array
  72
+     */
  73
+    public function get($type)
  74
+    {
  75
+        if (!$this->has($type)) {
  76
+            throw new \InvalidArgumentException(sprintf('Specified $type %s does not exist', $type));
  77
+        }
  78
+        
  79
+        return $this->flashes[$type];
  80
+    }
  81
+    
  82
+    /**
  83
+     * Sets an array of flash messages for a given type.
  84
+     * 
  85
+     * @param string $type
  86
+     * @param array  $array 
  87
+     */
  88
+    public function set($type, array $array)
  89
+    {
  90
+        $this->flashes[$type] = $array;
  91
+    }
  92
+    
  93
+    /**
  94
+     * Has messages for a given type?
  95
+     * 
  96
+     * @return boolean
  97
+     */
  98
+    public function has($type)
  99
+    {
  100
+        return array_key_exists($type, $this->flashes);
  101
+    }
  102
+    
  103
+    /**
  104
+     * Returns a list of all defined types.
  105
+     * 
  106
+     * @return array
  107
+     */
  108
+    public function getTypes()
  109
+    {
  110
+        return array_keys($this->flashes);
  111
+    }
  112
+
  113
+    /**
  114
+     * Gets all flashes.
  115
+     * 
  116
+     * @return array
  117
+     */
  118
+    public function all()
  119
+    {
  120
+        return $this->flashes;
  121
+    }
  122
+    
  123
+    /**
  124
+     * Clears flash messages for a given type.
  125
+     */
  126
+    public function clear($type)
  127
+    {
  128
+        if (isset($this->flashes[$type])) {
  129
+            unset($this->flashes[$type]);
  130
+        }
  131
+        
  132
+        if (isset($this->oldFlashes[$type])) {
  133
+            unset($this->oldFlashes[$type]);
  134
+        }
  135
+    }
  136
+    
  137
+    /**
  138
+     * Clears all flash messages.
  139
+     */
  140
+    public function clearAll()
  141
+    {
  142
+        $this->flashes = array();
  143
+        $this->oldFlashes = array();
  144
+    }
  145
+    
  146
+    /**
  147
+     * Removes flash messages set in a previous request.
  148
+     */
  149
+    public function purgeOldFlashes()
  150
+    {
  151
+        foreach ($this->oldFlashes as $type => $flashes) {
  152
+            $this->flashes[$type] = array_diff($this->flashes[$type], $flashes);
  153
+        }
  154
+    }
  155
+    
  156
+}
83  src/Symfony/Component/HttpFoundation/FlashBagInterface.php
... ...
@@ -0,0 +1,83 @@
  1
+<?php
  2
+
  3
+/*
  4
+ * This file is part of the Symfony package.
  5
+ *
  6
+ * (c) Fabien Potencier <fabien@symfony.com>
  7
+ *
  8
+ * For the full copyright and license information, please view the LICENSE
  9
+ * file that was distributed with this source code.
  10
+ */
  11
+
  12
+namespace Symfony\Component\HttpFoundation;
  13
+
  14
+/**
  15
+ * FlashBagInterface.
  16
+ * 
  17
+ * @author Drak <drak@zikula.org>
  18
+ */
  19
+interface FlashBagInterface
  20
+{
  21
+    /**
  22
+     * Initializes the FlashBag.
  23
+     * 
  24
+     * @param array $flashes 
  25
+     */
  26
+    function initialize(array $flashes);
  27
+
  28
+    /**
  29
+     * Adds a flash to the stack for a given type.
  30
+     */
  31
+    function add($type, $message);
  32
+    
  33
+    /**
  34
+     * Gets flash messages for a given type.
  35
+     * 
  36
+     * @return array
  37
+     */
  38
+    function get($type);
  39
+    
  40
+    /**
  41
+     * Sets an array of flash messages for a given type.
  42
+     * 
  43
+     * @param string $type
  44
+     * @param array  $array 
  45
+     */
  46
+    function set($type, array $array);
  47
+    
  48
+    /**
  49
+     * Hass flash messages for a given type?
  50
+     * 
  51
+     * @return boolean
  52
+     */
  53
+    function has($type);
  54
+
  55
+    /**
  56
+     * Returns a list of all defined types.
  57
+     * 
  58
+     * @return array
  59
+     */
  60
+    function getTypes();
  61
+
  62
+    /**
  63
+     * Gets all flash messages.
  64
+     * 
  65
+     * @return array
  66
+     */
  67
+    function all();
  68
+    
  69
+    /**
  70
+     * Clears flash messages for a given type.
  71
+     */
  72
+    function clear($type);
  73
+    
  74
+    /**
  75
+     * Clears all flash messages.
  76
+     */
  77
+    function clearAll();
  78
+    
  79
+    /**
  80
+     * Removes flash messages set in a previous request.
  81
+     */
  82
+    function purgeOldFlashes();
  83
+}
142  src/Symfony/Component/HttpFoundation/Session.php
@@ -25,24 +25,43 @@ class Session implements \Serializable
25 25
     protected $storage;
26 26
     protected $started;
27 27
     protected $attributes;
28  
-    protected $flashes;
29  
-    protected $oldFlashes;
  28
+    
  29
+    /**
  30
+     * Flash message container.
  31
+     * 
  32
+     * @var \Symfony\Component\HttpFoundation\FlashBagInterface
  33
+     */
  34
+    protected $flashBag;
30 35
     protected $closed;
31  
-
  36
+    
32 37
     /**
33 38
      * Constructor.
34 39
      *
35  
-     * @param SessionStorageInterface $storage A SessionStorageInterface instance
  40
+     * @param SessionStorageInterface $storage  A SessionStorageInterface instance.
  41
+     * @param FlashBagInterface       $flashBag A FlashBagInterface instance.
36 42
      */
37  
-    public function __construct(SessionStorageInterface $storage)
  43
+    public function __construct(SessionStorageInterface $storage, FlashBagInterface $flashBag)
38 44
     {
39 45
         $this->storage = $storage;
40  
-        $this->flashes = array();
41  
-        $this->oldFlashes = array();
  46
+        $this->flashBag = $flashBag;
42 47
         $this->attributes = array();
43 48
         $this->started = false;
44 49
         $this->closed = false;
45 50
     }
  51
+    
  52
+    /**
  53
+     * Get the flashbag driver.
  54
+     * 
  55
+     * @return FlashBagInterface
  56
+     */
  57
+    public function getFlashBag()
  58
+    {
  59
+        if (false === $this->started) {
  60
+            $this->start();
  61
+        }
  62
+        
  63
+        return $this->flashBag;
  64
+    }
46 65
 
47 66
     /**
48 67
      * Starts the session storage.
@@ -61,10 +80,7 @@ public function start()
61 80
 
62 81
         if (isset($attributes['attributes'])) {
63 82
             $this->attributes = $attributes['attributes'];
64  
-            $this->flashes = $attributes['flashes'];
65  
-
66  
-            // flag current flash messages to be removed at shutdown
67  
-            $this->oldFlashes = $this->flashes;
  83
+            $this->flashBag->initialize($attributes['flashes']);
68 84
         }
69 85
 
70 86
         $this->started = true;
@@ -174,7 +190,7 @@ public function clear()
174 190
         }
175 191
 
176 192
         $this->attributes = array();
177  
-        $this->flashes = array();
  193
+        $this->flashBag->clearAll();
178 194
     }
179 195
 
180 196
     /**
@@ -215,114 +231,18 @@ public function getId()
215 231
         return $this->storage->getId();
216 232
     }
217 233
 
218  
-    /**
219  
-     * Gets the flash messages.
220  
-     *
221  
-     * @return array
222  
-     */
223  
-    public function getFlashes()
224  
-    {
225  
-        return $this->flashes;
226  
-    }
227  
-
228  
-    /**
229  
-     * Sets the flash messages.
230  
-     *
231  
-     * @param array $values
232  
-     */
233  
-    public function setFlashes($values)
234  
-    {
235  
-        if (false === $this->started) {
236  
-            $this->start();
237  
-        }
238  
-
239  
-        $this->flashes = $values;
240  
-        $this->oldFlashes = array();
241  
-    }
242  
-
243  
-    /**
244  
-     * Gets a flash message.
245  
-     *
246  
-     * @param string      $name
247  
-     * @param string|null $default
248  
-     *
249  
-     * @return string
250  
-     */
251  
-    public function getFlash($name, $default = null)
252  
-    {
253  
-        return array_key_exists($name, $this->flashes) ? $this->flashes[$name] : $default;
254  
-    }
255  
-
256  
-    /**
257  
-     * Sets a flash message.
258  
-     *
259  
-     * @param string $name
260  
-     * @param string $value
261  
-     */
262  
-    public function setFlash($name, $value)
263  
-    {
264  
-        if (false === $this->started) {
265  
-            $this->start();
266  
-        }
267  
-
268  
-        $this->flashes[$name] = $value;
269  
-        unset($this->oldFlashes[$name]);
270  
-    }
271  
-
272  
-    /**
273  
-     * Checks whether a flash message exists.
274  
-     *
275  
-     * @param string $name
276  
-     *
277  
-     * @return Boolean
278  
-     */
279  
-    public function hasFlash($name)
280  
-    {
281  
-        if (false === $this->started) {
282  
-            $this->start();
283  
-        }
284  
-
285  
-        return array_key_exists($name, $this->flashes);
286  
-    }
287  
-
288  
-    /**
289  
-     * Removes a flash message.
290  
-     *
291  
-     * @param string $name
292  
-     */
293  
-    public function removeFlash($name)
294  
-    {
295  
-        if (false === $this->started) {
296  
-            $this->start();
297  
-        }
298  
-
299  
-        unset($this->flashes[$name]);
300  
-    }
301  
-
302  
-    /**
303  
-     * Removes the flash messages.
304  
-     */
305  
-    public function clearFlashes()
306  
-    {
307  
-        if (false === $this->started) {
308  
-            $this->start();
309  
-        }
310  
-
311  
-        $this->flashes = array();
312  
-        $this->oldFlashes = array();
313  
-    }
314  
-
315 234
     public function save()
316 235
     {
317 236
         if (false === $this->started) {
318 237
             $this->start();
319 238
         }
320 239
 
321  
-        $this->flashes = array_diff_key($this->flashes, $this->oldFlashes);
  240
+        // expire old flashes
  241
+        $this->flashBag->purgeOldFlashes();
322 242
 
323 243
         $this->storage->write('_symfony2', array(
324 244
             'attributes' => $this->attributes,
325  
-            'flashes'    => $this->flashes,
  245
+            'flashes'    => $this->flashBag->all(),
326 246
         ));
327 247
     }
328 248
 
122  tests/Symfony/Tests/Component/HttpFoundation/FlashBagTest.php
... ...
@@ -0,0 +1,122 @@
  1
+<?php
  2
+
  3
+/*
  4
+ * This file is part of the Symfony package.
  5
+ *
  6
+ * (c) Fabien Potencier <fabien@symfony.com>
  7
+ *
  8
+ * For the full copyright and license information, please view the LICENSE
  9
+ * file that was distributed with this source code.
  10
+ */
  11
+
  12
+namespace Symfony\Tests\Component\HttpFoundation;
  13
+
  14
+use Symfony\Component\HttpFoundation\FlashBag;
  15
+use Symfony\Component\HttpFoundation\FlashBagInterface;
  16
+
  17
+/**
  18
+ * FlashBagTest
  19
+ *
  20
+ * @author Drak <drak@zikula.org>
  21
+ */
  22
+class FlashBagTest extends \PHPUnit_Framework_TestCase
  23
+{
  24
+    /**
  25
+     * @var \Symfony\Component\HttpFoundation\FlashBagInterface
  26
+     */
  27
+    private $flashBag;
  28
+    
  29
+    public function setUp()
  30
+    {
  31
+        parent::setUp();
  32
+        $this->flashBag = new FlashBag();
  33
+        $this->flashBag->initialize(array('status' => array('A previous flash message')));
  34
+    }
  35
+    
  36
+    public function tearDown()
  37
+    {
  38
+        $this->flashBag = null;
  39
+        parent::tearDown();
  40
+    }
  41
+    
  42
+    public function testInitialize()
  43
+    {
  44
+        $this->flashBag->initialize(array());
  45
+        $this->flashBag->initialize(array());
  46
+    }
  47
+
  48
+    /**
  49
+     * @todo Implement testAdd().
  50
+     */
  51
+    public function testAdd()
  52
+    {
  53
+        $this->flashBag->add('Something new', FlashBag::STATUS);
  54
+        $this->flashBag->add('Smile, it might work next time', FlashBag::ERROR);
  55
+        $this->assertEquals(array('A previous flash message', 'Something new'), $this->flashBag->get(FlashBag::STATUS));
  56
+        $this->assertEquals(array('Smile, it might work next time'), $this->flashBag->get(FlashBag::ERROR));
  57
+    }
  58
+
  59
+    public function testGet()
  60
+    {
  61
+        $this->assertEquals(array('A previous flash message'), $this->flashBag->get(FlashBag::STATUS));
  62
+    }
  63
+    
  64
+    /**
  65
+     * @expectedException \InvalidArgumentException
  66
+     */
  67
+    public function testGetException()
  68
+    {
  69
+        $bang = $this->flashBag->get('bang');
  70
+    }
  71
+
  72
+    public function testSet()
  73
+    {
  74
+        $this->flashBag->set(FlashBag::STATUS, array('Foo', 'Bar'));
  75
+        $this->assertEquals(array('Foo', 'Bar'), $this->flashBag->get(FlashBag::STATUS));
  76
+    }
  77
+
  78
+    public function testHas()
  79
+    {
  80
+        $this->assertFalse($this->flashBag->has('nothing'));
  81
+        $this->assertTrue($this->flashBag->has(FlashBag::STATUS));
  82
+    }
  83
+
  84
+    /**
  85
+     * @todo Implement testGetTypes().
  86
+     */
  87
+    public function testGetTypes()
  88
+    {
  89
+        $this->assertEquals(array(FlashBag::STATUS), $this->flashBag->getTypes());
  90
+    }
  91
+
  92
+    public function testAll()
  93
+    {
  94
+        // nothing to do here
  95
+    }
  96
+
  97
+    public function testClear()
  98
+    {
  99
+        $this->assertTrue($this->flashBag->has(FlashBag::STATUS));
  100
+        $this->flashBag->clear(FlashBag::STATUS);
  101
+        $this->assertFalse($this->flashBag->has(FlashBag::STATUS));
  102
+    }
  103
+
  104
+    public function testClearAll()
  105
+    {
  106
+        $this->assertTrue($this->flashBag->has(FlashBag::STATUS));
  107
+        $this->flashBag->add('Smile, it might work next time', FlashBag::ERROR);
  108
+        $this->assertTrue($this->flashBag->has(FlashBag::ERROR));
  109
+        $this->flashBag->clearAll();
  110
+        $this->assertFalse($this->flashBag->has(FlashBag::STATUS));
  111
+        $this->assertFalse($this->flashBag->has(FlashBag::ERROR));
  112
+    }
  113
+
  114
+    public function testPurgeOldFlashes()
  115
+    {
  116
+        $this->flashBag->add('Foo', FlashBag::STATUS);
  117
+        $this->flashBag->add('Bar', FlashBag::ERROR);
  118
+        $this->flashBag->purgeOldFlashes();
  119
+        $this->assertEquals(array(1 => 'Foo'), $this->flashBag->get(FlashBag::STATUS));
  120
+    }
  121
+        
  122
+}
63  tests/Symfony/Tests/Component/HttpFoundation/SessionTest.php
@@ -12,6 +12,8 @@
12 12
 namespace Symfony\Tests\Component\HttpFoundation;
13 13
 
14 14
 use Symfony\Component\HttpFoundation\Session;
  15
+use Symfony\Component\HttpFoundation\FlashBag;
  16
+use Symfony\Component\HttpFoundation\FlashBagInterface;
15 17
 use Symfony\Component\HttpFoundation\SessionStorage\ArraySessionStorage;
16 18
 
17 19
 /**
@@ -24,54 +26,29 @@ class SessionTest extends \PHPUnit_Framework_TestCase
24 26
 {
25 27
     protected $storage;
26 28
     protected $session;
  29
+    
  30
+    /**
  31
+     * @var \Symfony\Component\HttpFoundation\FlashBagInterface
  32
+     */
  33
+    protected $flashBag;
27 34
 
28 35
     public function setUp()
29 36
     {
30 37
         $this->storage = new ArraySessionStorage();
  38
+        $this->flashBag = new FlashBag();
31 39
         $this->session = $this->getSession();
32 40
     }
33 41
 
34 42
     protected function tearDown()
35 43
     {
36 44
         $this->storage = null;
  45
+        $this->flashBag = null;
37 46
         $this->session = null;
38 47
     }
39  
-
40  
-    public function testFlash()
  48
+    
  49
+    public function getFlashBag()
41 50
     {
42  
-        $this->session->clearFlashes();
43  
-
44  
-        $this->assertSame(array(), $this->session->getFlashes());
45  
-
46  
-        $this->assertFalse($this->session->hasFlash('foo'));
47  
-
48  
-        $this->session->setFlash('foo', 'bar');
49  
-
50  
-        $this->assertTrue($this->session->hasFlash('foo'));
51  
-        $this->assertSame('bar', $this->session->getFlash('foo'));
52  
-
53  
-        $this->session->removeFlash('foo');
54  
-
55  
-        $this->assertFalse($this->session->hasFlash('foo'));
56  
-
57  
-        $flashes = array('foo' => 'bar', 'bar' => 'foo');
58  
-
59  
-        $this->session->setFlashes($flashes);
60  
-
61  
-        $this->assertSame($flashes, $this->session->getFlashes());
62  
-    }
63  
-
64  
-    public function testFlashesAreFlushedWhenNeeded()
65  
-    {
66  
-        $this->session->setFlash('foo', 'bar');
67  
-        $this->session->save();
68  
-
69  
-        $this->session = $this->getSession();
70  
-        $this->assertTrue($this->session->hasFlash('foo'));
71  
-        $this->session->save();
72  
-
73  
-        $this->session = $this->getSession();
74  
-        $this->assertFalse($this->session->hasFlash('foo'));
  51
+        $this->assetTrue($this->getFlashBag() instanceof FlashBagInterface);
75 52
     }
76 53
 
77 54
     public function testAll()
@@ -109,26 +86,26 @@ public function testAll()
109 86
     public function testMigrateAndInvalidate()
110 87
     {
111 88
         $this->session->set('foo', 'bar');
112  
-        $this->session->setFlash('foo', 'bar');
  89
+        $this->session->getFlashBag()->set('foo', array('bar'));
113 90
 
114 91
         $this->assertSame('bar', $this->session->get('foo'));
115  
-        $this->assertSame('bar', $this->session->getFlash('foo'));
  92
+        $this->assertEquals(array('bar'), $this->session->getFlashBag()->get('foo'));
116 93
 
117 94
         $this->session->migrate();
118 95
 
119 96
         $this->assertSame('bar', $this->session->get('foo'));
120  
-        $this->assertSame('bar', $this->session->getFlash('foo'));
  97
+        $this->assertEquals(array('bar'), $this->session->getFlashBag()->get('foo'));
121 98
 
122 99
         $this->session = $this->getSession();
123 100
         $this->session->invalidate();
124 101
 
125 102
         $this->assertSame(array(), $this->session->all());
126  
-        $this->assertSame(array(), $this->session->getFlashes());
  103
+        $this->assertEquals(array(), $this->session->getFlashBag()->all());
127 104
     }
128 105
 
129 106
     public function testSerialize()
130 107
     {
131  
-        $this->session = new Session($this->storage);
  108
+        $this->session = new Session($this->storage, $this->flashBag);
132 109
 
133 110
         $compare = serialize($this->storage);
134 111
 
@@ -145,7 +122,7 @@ public function testSerialize()
145 122
     public function testSave()
146 123
     {
147 124
         $this->storage = new ArraySessionStorage();
148  
-        $this->session = new Session($this->storage);
  125
+        $this->session = new Session($this->storage, $this->flashBag);
149 126
         $this->session->set('foo', 'bar');
150 127
 
151 128
         $this->session->save();
@@ -167,7 +144,7 @@ public function testStart()
167 144
     {
168 145
         $this->session->start();
169 146
 
170  
-        $this->assertSame(array(), $this->session->getFlashes());
  147
+        $this->assertSame(array(), $this->session->getFlashBag()->all());
171 148
         $this->assertSame(array(), $this->session->all());
172 149
     }
173 150
 
@@ -227,6 +204,6 @@ public function testStorageRemove()
227 204
 
228 205
     protected function getSession()
229 206
     {
230  
-        return new Session($this->storage);
  207
+        return new Session($this->storage, $this->flashBag);
231 208
     }
232 209
 }
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note