Improving flash messages #1863

Closed
stof opened this Issue Jul 29, 2011 · 18 comments

Projects

None yet

9 participants

@stof
Member
stof commented Jul 29, 2011

Currently, flash messages are stored as a simple key/value pair. But this implementation has many drawbacks:

  • The key has to be unique, thus making it impossible to use it to indicate a category of flash message (error, notice, success...) as you could have only one for each category. @lsmith77 suggested to allow having an array of flash messages for each key.
  • There is no way to know the adequate translation domain. This makes their internationalization pretty hard as you cannot easily use the translations provided by a shared bundle for its own flash message. IMO we should have the possibility to store the translation domain in each flash message.

@fabpot what do you think about a refactoring of the flash messages for 2.1 to improve these points, making them far easier to use ?

@stof
Member
stof commented Jul 29, 2011

Here are the link to some discussions about this subject in FOSUserBundle: FriendsOfSymfony/FOSUserBundle#246, FriendsOfSymfony/FOSUserBundle#132 and FriendsOfSymfony/FOSUserBundle@3c735ad#comments

@brikou
brikou commented Jul 29, 2011

It could be great to be able to loop over flash message in the template, and also get the severity or priority (fe. notice, warn, info...) in order to easily assign the correct class of the displaying div.

// using flash as a stack

$this->addFlash('error', 'Oops! something went wrong...');

$this->setFlashes('error', array('Oops #1', 'Oops #2', 'Oops #3');
$this->getFlashes('error');
@lsmith77
Collaborator

sure this could be another possibility that flash messages optionally allow a "type" so you can do setFlash('key', 'value', 'notice') etc.

@henrikbjorn

@brikou the first parameter is essentially the level atm.

@weaverryan
Member

I'm particularly interested in the second issue that @stof pointed out. At the end, we need a best-practice that all bundles use, so that the end-user can render flash messages consistently in their one base template. From a technical perspective, having flash messages stored in many translation domains makes this currently not possible.

@deviantintegral

We're looking at using the Session class as a part of the WSCCI initiative over at drupal.org. For the most part, flash messages in the Session class mirror what our current drupal_set_message() implementation is, minus a type parameter. So, if we could get a type parameter added, it'd help us quite a bit. I've started a branch working on this here:

https://github.com/deviantintegral/symfony/tree/1863-improve-flash

Unfortunately, I'm having trouble getting tests to pass, even without my changes. So, not sure if there's any critical breakage or not in the commit.

@lsmith77
Collaborator

think in some thread on drupal.org we discussed moving the flash handling to another class. that would make it cleaner to customize the logic.

On 22.10.2011, at 02:11, deviantintegral reply@reply.github.com wrote:

We're looking at using the Session class as a part of the WSCCI initiative over at drupal.org. For the most part, flash messages in the Session class mirror what our current drupal_set_message() implementation is, minus a type parameter. So, if we could get a type parameter added, it'd help us quite a bit. I've started a branch working on this here:

https://github.com/deviantintegral/symfony/tree/1863-improve-flash

Unfortunately, I'm having trouble getting tests to pass, even without my changes. So, not sure if there's any critical breakage or not in the commit.

Reply to this email directly or view it on GitHub:
#1863 (comment)

@stof
Member
stof commented Oct 22, 2011

@deviantintegral I don't know the drupal implementation but your request looks like the idea of category I suggested here. Am I right ?

@fabpot what do you think about this ? You were quite reluctant to change flash messages when we were in beta IIRC and you hasn't given your mind here yet.

@grayside

I worked with deviantintegral on this. Our thinking was the existence of this issue implied a community interest in Symfony to functional convergence. The suggested change was done in a backwards compatible fashion to avoid any breakages.

@ghost
ghost commented Oct 25, 2011

I'd just like to voice support for this. We've been looking at using HttpFoundation for Zikula. We currently have the ability to have multiple flashes in different categories (notice, warning, error). It's pretty essential. Basically two things are currently missing, firstly categories, and secondly, ability to have multiple flashes in each category using something like addFlash(). I would happily make a PR for this.

@deviantintegral

@stof Yep, we need a category or type for each message we set.

@drak The current patch stores flashes in an array keyed first by type, then by name. So, if you wanted to add a flash to a specific category, you could call:

setFlash('login', 'Your password was slightly incorrect.', 'error');

or simply

setFlash('login', 'Your username was not found.');

to match the existing API without a type.

@ghost
ghost commented Oct 28, 2011

Yes, but this method requires a specific key $name. What if you need to issue multiple flashes, you have to assign a unique key each time. There should be an addFlash($message) which does

$this->flashes[$type][] = $value;
@deviantintegral

I've fixed a few test failures, and added an addFlash function. It'd be great if someone else could review it before I submit a pull request.

@lsmith77
Collaborator

@deviantintegral just send the PR .. if you are unsure add [WIP] as a prefix to the title of the PR. this makes it easier for everybody to provide feedback. please also include the following checklist in your PR description:
http://symfony.com/doc/current/contributing/code/patches.html#check-list

@ghost
ghost commented Oct 28, 2011

@deviantintegral - You can also add "refs #1863" in the PR and it will reference in this ticket so both will be linked.

@fabpot fabpot added a commit that referenced this issue 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 fabpot closed this Mar 23, 2012
@dTHQb
dTHQb commented Feb 4, 2014

Ok so if I can summarize this we can now add multiple flash messages of the same type ie 'info' ? And is it at all possible to prioritize the flash messages as enquired about by @brikou ? @fabpot @drak @lsmith77 @stof ??

I am using symfony 2.3.9 and adding flash messages as follow:
$session->getFlashBag()->add($type, $message);

As far as I can tell is if I add 'error', 'info', 'success' it is displayed as 'error', 'info', 'success' and if I change the order it displays the same order it was set. And from what I can gather in the FlashBag class and FlashBagInterface there is no priority.

And I don't mean make it an error 'error', I mean saying the order of flash messages is success then error then warning then info, or this is priorit one it should be at the top?

Is there a work around or how do I prioritize flash messages?

Example of where this might come in handy. @stof in the FOS\UserBundle lets say regastration success the event listener adds the flash message, now in our process we add a flash message ('info', 'You can continue browsing and searching but to make suggestions you have to confirm your account first'), then when the whole process is complete the success message from FOS is at the bottom while the info from us is at the top(because that is the order in which these events occurred). It would make sense to have the success message at the top rather than the info?

Does anybody have a similar situation with a solution or am I trying to implement the flash messages wrong when it comes to prioritizing them?

@stof
Member
stof commented Feb 4, 2014

@dTHQb yes you can have several falsh messages of the same type. There is no priority of flash messages. There is a separate list for each type of flash message. there order of the types between each other depends of your rendering logic

@dTHQb
dTHQb commented Feb 4, 2014

@stof ok cool. I see that Mopa\BootstrapBundle just display them as is. So basically I need to override the template that adds the flash messages itself to achieve that. Or I need use $session->getFlashBag()->all(); sort it in the priority I want and then re-add it to the flashbag. Technically both methods will work but which of these options are better? Considering that if there is a point where 3 flash messages are displayed at once then something else went horribly wrong on the site..

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