Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[2.1][HttpFoundation] Multiple session flash messages#3267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ghost commentedFeb 13, 2012
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMO this should be reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, ugly. done.
lsmith77 commentedFeb 14, 2012
the FlashBag vs. AutoExpireFlashBag behavior and setup difference should probably also be explained in the upgrading log |
ghost commentedFeb 15, 2012
@lsmith77 Those differences are explained already in the changelog
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It doesn't.$foo['bar']['baz'] = true; is valid for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah good. Just checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
reading throw a notice. Writing does not
Crell commentedFeb 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. :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No no, this API is designed to replicate the old functionality (no arrays). All this API is already deprecated.
lsmith77 commentedMar 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 commentedMar 8, 2012
Another plus for this PR is that it requires no extra lines of code in templates etc to display the flashes, seehttps://github.com/symfony/symfony/pull/3267/files#diff-1 |
If code has not be refactored for the new API's thenyou would still be using the API with one message per $name.
ghost commentedMar 15, 2012
Rebased against current |
evillemez commentedMar 17, 2012
+1 to this, I have an extended version of HttpFoundation just for this... would love to get rid of it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You should keep it like it was as you need to keep how it was done in Symfony 2.0, not before your patch.
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 messagesBug fix: noFeature addition: yesBackwards compatibility break: yes, but this already happened in#2583. BC `Session` methods remain unbroken.Symfony2 tests pass: yesFixes the following tickets:#1863References the following tickets:#2714,#2753,#2510,#2543,#2853Todo: -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:33ZI think this is ready for review@fabpot@lsmith77---------------------------------------------------------------------------by lsmith77 at 2012-02-14T19:30:39Zthe 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:34ZDrak 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:13ZAnother plus for this PR is that it requires no extra lines of code in templates etc to display the flashes, seehttps://github.com/symfony/symfony/pull/3267/files#diff-1---------------------------------------------------------------------------by drak at 2012-03-15T06:38:21ZRebased 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.
Bug fix: no
Feature addition: yes
Backwards compatibility break: yes, but this already happened in#2583. BC
Sessionmethods 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 toset().NOTES ABOUT BC
This PR maintains BC behaviour with the
Sessionclass in that the old Symfony 2.0 methods will continue to work as before.