[2.1][HttpFoundation] Multiple session flash messages #3267

Merged
merged 4 commits into from Mar 23, 2012

Conversation

Projects
None yet
9 participants
@ghost

ghost commented Feb 4, 2012

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.

@ghost

ghost commented Feb 13, 2012

I think this is ready for review @fabpot @lsmith77

@stloyd stloyd and 1 other commented on an outdated diff Feb 13, 2012

src/Symfony/Component/HttpFoundation/Session/Session.php
@@ -45,8 +45,8 @@ class Session implements SessionInterface
public function __construct(SessionStorageInterface $storage, AttributeBagInterface $attributes = null, FlashBagInterface $flashes = null)
{
$this->storage = $storage;
- $this->registerBag($attributes ?: new AttributeBag());
- $this->registerBag($flashes ?: new FlashBag());
+ $this->registerBag($attributes ? : new AttributeBag());
+ $this->registerBag($flashes ? : new FlashBag());
@stloyd

stloyd Feb 13, 2012

Contributor

IMO this should be reverted.

@ghost

ghost Feb 13, 2012

Yes, ugly. done.

@stloyd stloyd and 1 other commented on an outdated diff Feb 13, 2012

src/Symfony/Component/HttpFoundation/Session/Session.php
@@ -192,7 +192,16 @@ public function getFlashBag()
*/
public function getFlashes()
{
- return $this->getBag('flashes')->all();
+ $all = $this->getBag('flashes')->all();
+
+ $return = array();
+ if ($all) {
+ foreach ($all as $name => $array) {
+ $return[$name] = $array[0];
@stloyd

stloyd Feb 13, 2012

Contributor

current($array) or reset($array) ?

@ghost

ghost Feb 13, 2012

Same difference.

@stloyd stloyd and 1 other commented on an outdated diff Feb 13, 2012

src/Symfony/Component/HttpFoundation/Session/Session.php
@@ -215,7 +226,9 @@ public function setFlashes($values)
*/
public function getFlash($name, $default = null)
{
- return $this->getBag('flashes')->get($name, $default);
+ $return = $this->getBag('flashes')->get($name, is_null($default) ? array() : (array)$default);
@stloyd

stloyd Feb 13, 2012

Contributor

Should be: null === $default or even better IMO:

$return = $this->getBag('flashes')->get($name);

return null === $return ? $default : reset($return);
@ghost

ghost Feb 13, 2012

This isn't quite correct since the bag will return an array() as default, but yes good tweak.

@stloyd stloyd and 1 other commented on an outdated diff Feb 13, 2012

src/Symfony/Component/HttpFoundation/Session/Session.php
}
+
@stloyd

stloyd Feb 13, 2012

Contributor

Unnecessary new line.

@stloyd stloyd and 1 other commented on an outdated diff Feb 13, 2012

...y/Component/HttpFoundation/Session/Flash/FlashBag.php
{
- $this->flashes[$type] = $message;
+ $messages = (array)$messages;
+ $this->flashes[$type] = $messages;
@stloyd

stloyd Feb 13, 2012

Contributor

This could be written in one line ;-)

@stloyd stloyd and 1 other commented on an outdated diff Feb 13, 2012

UPGRADE-2.1.md
</div>
- {% endif %}
+ {% endforeach %}
@stloyd

stloyd Feb 13, 2012

Contributor

AFAIK there is no such tag like: {% endforeach %} but there is: {% endfor %}

@ghost

ghost Feb 13, 2012

Right enough, I'll change this.

@lsmith77 lsmith77 and 1 other commented on an outdated diff Feb 14, 2012

...ests/Component/HttpFoundation/Session/SessionTest.php
@@ -155,13 +155,17 @@ public function testGetSetFlashes()
$this->session->setFlashes($array);
$this->assertEquals($array, $this->session->getFlashes());
$this->assertEquals(array(), $this->session->getFlashes());
+ $this->session->getFlashBag()->add('notice', 'foo');
+ $this->session->getFlashBag()->add('notice', 'foo2'); // this tests the BC works by only retrieving the first added.
@lsmith77

lsmith77 Feb 14, 2012

Contributor

"this tests that BC works"

some below. plus i would move the comment to a new line

Contributor

lsmith77 commented Feb 14, 2012

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

@ghost

ghost commented Feb 15, 2012

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

@Crell Crell commented on the diff Feb 19, 2012

...t/HttpFoundation/Session/Flash/AutoExpireFlashBag.php
@@ -75,7 +75,15 @@ public function initialize(array &$flashes)
/**
* {@inheritdoc}
*/
- public function peek($type, $default = null)
+ public function add($type, $message)
+ {
+ $this->flashes['new'][$type][] = $message;
@Crell

Crell Feb 19, 2012

Contributor

Will this generate a notice is flashes['new'][$type] is not yet defined? I seem to recall it doing so for me in the past.

@Seldaek

Seldaek Feb 19, 2012

Member

It doesn't. $foo['bar']['baz'] = true; is valid for example

@Crell

Crell Feb 19, 2012

Contributor

Ah good. Just checking.

@stof

stof Feb 19, 2012

Member

reading throw a notice. Writing does not

Contributor

Crell commented Feb 19, 2012

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

@deviantintegral deviantintegral commented on the diff Feb 29, 2012

src/Symfony/Component/HttpFoundation/Session/Session.php
@@ -192,7 +192,16 @@ public function getFlashBag()
*/
public function getFlashes()
{
- return $this->getBag('flashes')->all();
+ $all = $this->getBag('flashes')->all();
+
+ $return = array();
+ if ($all) {
+ foreach ($all as $name => $array) {
+ $return[$name] = reset($array);
+ }
+ }
+
+ return $return;
@deviantintegral

deviantintegral Feb 29, 2012

Can this be simplified? It's not clear why we have to manually build out the return value.

is_array($all) ? return $all : return array();

@ghost

ghost Feb 29, 2012

No no, this API is designed to replicate the old functionality (no arrays). All this API is already deprecated.

@lsmith77 lsmith77 and 1 other commented on an outdated diff Mar 6, 2012

...y/Component/HttpFoundation/Session/Flash/FlashBag.php
{
- $this->flashes[$type] = $message;
+ $this->flashes[$type] = (array)$messages;
@lsmith77

lsmith77 Mar 6, 2012

Contributor

afaik there should be a space after the (array)

@ghost

ghost Mar 8, 2012

OK I have fixed this and pushed.

Contributor

lsmith77 commented Mar 6, 2012

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

@ghost

ghost commented Mar 8, 2012

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

Drak added some commits Feb 11, 2012

@ghost

ghost commented Mar 15, 2012

Rebased against current master, should be mergeable again..

Contributor

evillemez commented Mar 17, 2012

+1 to this, I have an extended version of HttpFoundation just for this... would love to get rid of it.

Unknown referenced this pull request in symfony/symfony-docs Mar 21, 2012

Merged

[HttpFoundation] Update for flash message API. #1179

@fabpot fabpot commented on the diff Mar 23, 2012

UPGRADE-2.1.md
@@ -307,30 +307,31 @@ UPGRADE FROM 2.0 to 2.1
Before:
```
- {% if app.session.hasFlash('notice') %}
@fabpot

fabpot Mar 23, 2012

Owner

You should keep it like it was as you need to keep how it was done in Symfony 2.0, not before your patch.

@fabpot fabpot added a commit that referenced this pull request Mar 23, 2012

@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

fabpot merged commit 5ae76f1 into symfony:master Mar 23, 2012

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