Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[WIP][HttpFoundation] Improve flash messages (replaces PR #2510) #2543

Closed
wants to merge 4 commits into from

6 participants

@ghost

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1863, #2510

Adds a "type" parameter for flash messages. By default, status messages are placed in the "status" bucket, but type could be 'error', 'warning', etc. An addFlash() method is also added to easily add flash messages without needing to specify a type or a name.

NB This PR replaces #2510, that is, it integrates the original PR plus all the comments in the thread.

@ghost Unknown referenced this pull request
Closed

1863 improve flash #2510

@lsmith77
Collaborator

see discussion from todays meeting:
https://gist.github.com/1337068

CC @deviantintegral

@ghost

After discussing this on IRC, @fabpot said that really $name was originally intended as a type. With this in mind, we could do something different than this PR and probably make it a bit more simple. The basic thing that is missing from the current implementation in the master branch is that you cannot have more than one message per $name (which is really a category type).

@stof
Collaborator

The issue with using the name as type is that the name must be unique. so a given request can only have one notice. So if a listener sets a flash message and the controller sets another one, only the last one will be kept. @fabpot Can you explain us how we should handle that ?

@lsmith77
Collaborator

I really do not understand yet how @fabpot uses flash messages. If its really just being able to send one message to the next request, why even have a key?

Imho we should really specify the use case(s) we want to cover and then think about the API. right now we have a feature that seems overly complex for solving the use case @fabpot seems to intend and too "wild west" for the use case that @drak, @rande and I see.

@ghost

For the record, I don't like the implementation in this PR. There was a suggestion on IRC last night to move flash messages out of the session class entirely so I'm working on a draft for you guys to look at. I'll try to push something today - it will be much more the way Fabien does things but allows multiple categorised flashes and the API is a lot more simple, as it should be.

@ghost

This stuff really needs to work out of the box - we shouldnt be expected to write stuff on top, or manage things via events/DIC because HttpFoundation is a component that might be used outside of a Symfony2 style application - that's the whole point of the components right? For that reason, flash messages really does need to be more complete than offering a single key => string solution

After talking with Fabien, it's clear he intended $name as a category/type which is great, but the value needs to be an array rather than a string as part of the API. This brings us to a question of BC.

The current PR is fine, it works - but it's ugly because it's trying to maintain BC in any way possible to get the new feature in, and in doing so, the API becomes overly complex and messy. The fact that $name is supposed to be a category type in the first place but is then we have this extra third parameter $type which makes $name pointless: none of this justice to Symfony2 - several people have already commented that "what is $name for when you have $type" - clearly it doesnt make sense when reading the code - exactly what Fabien said.

The problem is changing this requires an API change since getFlash() returns a string and we really need it to be an array (so that multiple messages can be stored against a key/category. Doing this we can maintain the simplicity of the API.

I've made a draft that breaks BC. I chose to outsource it to a separate class because it makes the method names more Symfony2 styled but it's just a suggestion, we could just rename methods in Session: I believe that if we are going to break BC, then don't do it by half, break it and make the new solution clean. There are no tests yet because this is of course a draft. The break is clean and could be well documented.

Please take a look at drak/symfony@7390a1e and give your thoughts.

HttpFoundation needs a this change to the session class because it's such a core and fundamental class that should be fully featured out of the box. Now is a good time to make a change like this for 2.1.

@lsmith77
Collaborator

i personally like this approach. @schmittjoh IIRC during the meeting you hacked up some code for a message class .. could you try and put that code in a PR so that we could compare?

@mvrhov

I believe lsmith is looking for this: https://gist.github.com/e8e11830614ba53661fa
BTW: this is not the only problem with flash messages. If messages are supposed to be translated in template and that message has parameters, then those additional parameters should also be attached alongside with translation key.

@ghost

@mvrhov - The flash should be translated before being set. Obviously if the current request is in German, then the subsequent request will also be in German. For example something like this:-

$session->setFlash('login', translate('Invalid username or password));
@stof
Collaborator

@drak The flash messages are for the next request. The locale is not handled by the request. If the user was on example.com/en/ where a flash message is set and then do the next request to example.com/fr/, he will expect seeing the message in French (i.e. translated when displaying it), not in English (i.e. translated when setting it).

@ghost

@stof - that's not logical. If we display a form in French, then any error messages or OK messages are going to be presented on a French page. Flashes are designed to be shown after a redirect, so there is no logical reason the redirect would be to a locale other than the one we just submitted the form on.

Think of this also from the point of generating translation catalogs (xliff, po etc). You scan your code for instances of translate() and then you can extract the catalog of strings. For this reason, anything you want translated needs to be encased in the translation command (whatever it is, e.g. with Gettext it's _($string).

@stof
Collaborator

@drak The error message of a form are translated in the template rendering the form, not in the Form object (which don't know about the Translator at all)

@inmarelibero

Why don't let twig manage the translation? with something like:
{% trans %}%flash_message%{% endtrans %}
or
{{ flash_message | trans }}

Having set a fallback locale in app/config/config.yml, it would be possible to set the flash message always in the default locale (eg. english) and then translate the string in the template after the eventual change of locale

@rande

The message must be translated when the notice is created. The rendered context does not nothing about the translation context : locale and catalogue.

@mvrhov

@inmarelibero: please don't forget that each flash message could have parameters which need to be passed along!

@rande. For me the flash message is a part of the view and view should decide about its translation. What you are saying is , not logical and could lead to WTFs. Because now you have strings that are displayed to users translated at two places.. flash messages in the controller or whenever they are generated and the other parts of the template in the template itself.

@ghost

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.

@ghost

I'll use Zikula as the basis of my examples, but from what I can see in trh Symfony2 documentation, the methodology is quite aligned. I wrote the i18n implementation in Zikula i18n using Gettext which is one of the most mature and well used translation methods (php-gettext sucks by the way which is why we use our own implementation. Please note the links to example are for Zikula.

Please consider the following:

  • It's illogical that the flashes system should have to store multiple translations because of how flashes work. They are a per session phenomenon and they exist for one page load only. In fact, the way they are used, the form submits, the controller processes, then immediately redirects and a page is then displayed with the flashes. It make no sense that a form would be in German and then after pressing the submit button would redirect to French for display. The flash message can therefor be translated and stored $session->addFlash('status', translate('Your answer is correct'); at the time of creating the flash as in #2592.

  • Your application already knows it's locale before the view is processed and you can easily translate your flash message.

  • Translation is not part of the view - translation is simply a string manipulation. You need translations potentially everywhere, including, system and code level error messages and exceptions. In Zikula we translate everything exception fatal programmer related errors/exceptions that would result in fatal PHP errors. This view is in line with the Symfony2 documentation - http://symfony.com/doc/2.0/book/translation.html where translations are made in a variety of places, in controllers, view and elsewhere.

  • The ideal translation system, is one where you can gather the translation catalog directly from the code and from templates where translatable strings appear - that's how Gettext works. You simply scan your PHP files and whatever else you have translatable strings in for the translation commands. In Zikula it's __('Some text'); in PHP, and {gt text="Some other text"} in our view (using Smarty). You can see some examples of view templates here and examples in the code here. Interestingly enough, gettext scanners will even extract comments proceeding the translation command directly from the code so you can give notes to translators for potentially ambiguous language. We use standard linux tools xgettext to extract from PHP and a simple process to compile our templates to PHP so they can be read by xgettext.

  • If you have things in the view which just do {{$string | trans}} then you have already lost the ability to auto-find your translation strings- `{{'Some text'|trans}} is quite ok, but using a variable is suspicious. In Zikula we have something like 2000 strings which we auto-extract (using gettext) to build the translation catalog which translators then translate. Finding and updating those manually would be an almost impossible task.

  • There is no reason in the world you cannot have a translation service, or make your controllers translation aware - interface example and controller base.

I'm closing this PR since all the discussions so far favour outsourcing the flashes to a flashbag e.g. in #2592

@ghost ghost closed this
@fabpot fabpot referenced this pull request from a commit
@fabpot 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
@fabpot fabpot referenced this pull request from a commit
@fabpot 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
@fabpot 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
@fabpot 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
@hackzilla hackzilla referenced this pull request from a commit
@fabpot 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
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 2, 2011
  1. @deviantintegral

    Updated session flashes to allow multiple types of flash message and

    deviantintegral authored Drak committed
    updated tests refs #2510 refs #1863
  2. Use constant for default flash type.

    Drak authored
    This is much more IDE friendly.
This page is out of date. Refresh to see the latest.
View
128 src/Symfony/Component/HttpFoundation/Session.php
@@ -22,6 +22,10 @@
*/
class Session implements \Serializable
{
+ const FLASH_INFO = 'info';
+ const FLASH_WARNING = 'warning';
+ const FLASH_ERROR = 'error';
+
protected $storage;
protected $started;
protected $attributes;
@@ -37,8 +41,8 @@ class Session implements \Serializable
public function __construct(SessionStorageInterface $storage)
{
$this->storage = $storage;
- $this->flashes = array();
- $this->oldFlashes = array();
+ $this->flashes = array(self::FLASH_INFO => array());
+ $this->oldFlashes = array(self::FLASH_INFO => array());
$this->attributes = array();
$this->started = false;
$this->closed = false;
@@ -174,7 +178,7 @@ public function clear()
}
$this->attributes = array();
- $this->flashes = array();
+ $this->flashes = array(self::FLASH_INFO => array());
}
/**
@@ -216,28 +220,62 @@ public function getId()
}
/**
+ * Gets the flash messages of a given type.
+ *
+ * @param string $type
+ *
+ * @throws \InvalidArgumentException If non-existing $type is requested.
+ *
+ * @return array
+ */
+ public function getFlashes($type = self::FLASH_INFO)
+ {
+ if (!isset($this->flashes[$type])) {
+ throw new \InvalidArgumentException(sprintf('Flash message type %s does not exist', $type));
+ }
+
+ return $this->flashes[$type];
+ }
+
+ /**
* Gets the flash messages.
*
* @return array
*/
- public function getFlashes()
+ public function getAllFlashes()
{
return $this->flashes;
}
/**
+ * Sets the flash messages of a specific type.
+ *
+ * @param array $values
+ * @param string $type
+ */
+ public function setFlashes($values, $type = self::FLASH_INFO)
+ {
+ if (false === $this->started) {
+ $this->start();
+ }
+
+ $this->flashes[$type] = $values;
+ $this->oldFlashes = array(self::FLASH_INFO => array());
+ }
+
+ /**
* Sets the flash messages.
*
* @param array $values
*/
- public function setFlashes($values)
+ public function setAllFlashes($values)
{
if (false === $this->started) {
$this->start();
}
$this->flashes = $values;
- $this->oldFlashes = array();
+ $this->oldFlashes = array(self::FLASH_INFO => array());
}
/**
@@ -245,12 +283,18 @@ public function setFlashes($values)
*
* @param string $name
* @param string|null $default
+ *
+ * @throws InvalidArgumentException If unknown type specified.
*
- * @return string
+ * @return mixed
*/
- public function getFlash($name, $default = null)
+ public function getFlash($name, $default = null, $type = self::FLASH_INFO)
{
- return array_key_exists($name, $this->flashes) ? $this->flashes[$name] : $default;
+ if (!isset($this->flashes[$type])) {
+ throw new \InvalidArgumentException(sprintf('Unknown flash message type %s', $type));
+ }
+
+ return array_key_exists($name, $this->flashes[$type]) ? $this->flashes[$type][$name] : $default;
}
/**
@@ -258,31 +302,39 @@ public function getFlash($name, $default = null)
*
* @param string $name
* @param string $value
+ * @param string $type
*/
- public function setFlash($name, $value)
+ public function setFlash($name, $value, $type = self::FLASH_INFO)
{
if (false === $this->started) {
$this->start();
}
- $this->flashes[$name] = $value;
- unset($this->oldFlashes[$name]);
+ $this->flashes[$type][$name] = $value;
+ unset($this->oldFlashes[$type][$name]);
}
/**
* Checks whether a flash message exists.
*
* @param string $name
+ * @param string $type
+ *
+ * @throws InvalidArgumentException If unknown type specified.
*
* @return Boolean
*/
- public function hasFlash($name)
+ public function hasFlash($name, $type = self::FLASH_INFO)
{
if (false === $this->started) {
$this->start();
}
+
+ if (!isset($this->flashes[$type])) {
+ throw new \InvalidArgumentException(sprintf('Unknown flash message type %s', $type));
+ }
- return array_key_exists($name, $this->flashes);
+ return array_key_exists($name, $this->flashes[$type]);
}
/**
@@ -290,13 +342,15 @@ public function hasFlash($name)
*
* @param string $name
*/
- public function removeFlash($name)
+ public function removeFlash($name, $type = self::FLASH_INFO)
{
if (false === $this->started) {
$this->start();
}
- unset($this->flashes[$name]);
+ if (isset($this->flashes[$type])) {
+ unset($this->flashes[$type][$name]);
+ }
}
/**
@@ -308,17 +362,33 @@ public function clearFlashes()
$this->start();
}
- $this->flashes = array();
- $this->oldFlashes = array();
+ $this->flashes = array(self::FLASH_INFO => array());
+ $this->oldFlashes = array(self::FLASH_INFO => array());
+ }
+
+ /**
+ * Adds a generic flash message to the session.
+ *
+ * @param string $value
+ * @param string $type
+ */
+ public function addFlash($value, $type = self::FLASH_INFO)
+ {
+ $this->flashes[$type][] = $value;
}
+ /**
+ * Save session.
+ */
public function save()
{
if (false === $this->started) {
$this->start();
}
- $this->flashes = array_diff_key($this->flashes, $this->oldFlashes);
+ foreach ($this->flashes as $type => $flashes) {
+ $this->flashes[$type] = array_diff_key($flashes, $this->oldFlashes[$type]);
+ }
$this->storage->write('_symfony2', array(
'attributes' => $this->attributes,
@@ -327,6 +397,8 @@ public function save()
}
/**
+ * Close the session without writing saving it.
+ *
* This method should be called when you don't want the session to be saved
* when the Session object is garbaged collected (useful for instance when
* you want to simulate the interaction of several users/sessions in a single
@@ -344,14 +416,30 @@ public function __destruct()
}
}
+ /**
+ * Serialize the session storage driver.
+ *
+ * @return string
+ */
public function serialize()
{
return serialize($this->storage);
}
+ /**
+ * Unserialize the SessionStorage.
+ *
+ * @param type $serialized
+ *
+ * @throws \InvalidArgumentException If serialized string is not an instance of SessionStorageInterface
+ */
public function unserialize($serialized)
{
- $this->storage = unserialize($serialized);
+ $storage = unserialize($serialized);
+ if (!$storage instanceof SessionStorageInterface) {
+ throw new \InvalidArgumentException('Serialized string passed does not unserialze to an instance of Symfony\\Component\\HttpFoundation\\SessionStorageInterface');
+ }
+ $this->storage = $storage;
$this->attributes = array();
$this->started = false;
}
View
71 tests/Symfony/Tests/Component/HttpFoundation/SessionTest.php
@@ -22,7 +22,14 @@
*/
class SessionTest extends \PHPUnit_Framework_TestCase
{
+ /**
+ * @var SessionStorage
+ */
protected $storage;
+
+ /**
+ * @var Session
+ */
protected $session;
public function setUp()
@@ -59,6 +66,55 @@ public function testFlash()
$this->session->setFlashes($flashes);
$this->assertSame($flashes, $this->session->getFlashes());
+
+ $this->session->clearFlashes();
+ $this->session->addFlash('foo');
+ $compare = $this->session->getFlashes();
+ $this->assertSame($compare, array(0 => 'foo'));
+ }
+
+ /**
+ * @expectedException InvalidArgumentException
+ */
+ public function testGetFlashException()
+ {
+ $this->session->getFlash('foo', null, 'notexisting');
+ }
+
+ /**
+ * @expectedException InvalidArgumentException
+ */
+ public function testHasFlashException()
+ {
+ $this->session->hasFlash('foo', 'notexisting');
+ }
+
+ public function testGetFlashes()
+ {
+ $this->assertEquals(array(), $this->session->getFlashes(Session::FLASH_INFO));
+ $this->session->setFlash(Session::FLASH_INFO, 'hello');
+ $this->assertEquals(array(Session::FLASH_INFO => 'hello'), $this->session->getFlashes(Session::FLASH_INFO));
+ }
+
+ /**
+ * @expectedException InvalidArgumentException
+ */
+ public function testGetFlashesException()
+ {
+ $this->session->getFlashes('notexisting');
+ }
+
+ public function testSetFlashes()
+ {
+ $this->session->setFlashes(array('hello', 'world'));
+ $this->assertEquals(array('hello', 'world'), $this->session->getFlashes(Session::FLASH_INFO));
+ }
+
+ public function testAllFlashes()
+ {
+ $this->assertEquals(array(Session::FLASH_INFO => array()), $this->session->getAllFlashes());
+ $this->session->setAllFlashes(array(Session::FLASH_INFO => array('hello', 'world')));
+ $this->assertEquals(array(Session::FLASH_INFO => array('hello', 'world')), $this->session->getAllFlashes());
}
public function testFlashesAreFlushedWhenNeeded()
@@ -141,6 +197,15 @@ public function testSerialize()
$this->assertEquals($_storage->getValue($this->session), $this->storage, 'storage match');
}
+
+ /**
+ * @expectedException InvalidArgumentException
+ */
+ public function testUnserializeException()
+ {
+ $serialized = serialize('not an instance of SessionStorage');
+ $this->session->unserialize($serialized);
+ }
public function testSave()
{
@@ -149,7 +214,7 @@ public function testSave()
$this->session->set('foo', 'bar');
$this->session->save();
- $compare = array('_symfony2' => array('attributes' => array('foo' => 'bar'), 'flashes' => array()));
+ $compare = array('_symfony2' => array('attributes' => array('foo' => 'bar'), 'flashes' => array(Session::FLASH_INFO => array())));
$r = new \ReflectionObject($this->storage);
$p = $r->getProperty('data');
@@ -179,7 +244,7 @@ public function testSavedOnDestruct()
$expected = array(
'attributes'=>array('foo'=>'bar'),
- 'flashes'=>array(),
+ 'flashes'=>array(Session::FLASH_INFO => array()),
);
$saved = $this->storage->read('_symfony2');
$this->assertSame($expected, $saved);
@@ -195,7 +260,7 @@ public function testSavedOnDestructAfterManualSave()
$expected = array(
'attributes'=>array('foo'=>'bar'),
- 'flashes'=>array(),
+ 'flashes'=>array(Session::FLASH_INFO => array()),
);
$saved = $this->storage->read('_symfony2');
$this->assertSame($expected, $saved);
Something went wrong with that request. Please try again.