Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 4 commits intosymfony:masterfromunknown repositoryMar 23, 2012
Merged

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

fabpot merged 4 commits intosymfony:masterfromunknown repositoryMar 23, 2012

Conversation

@ghost
Copy link

Bug fix: no
Feature addition: yes
Backwards compatibility break: yes, but this already happened in#2583. BCSession 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 theadd() method or by passing an array of messages toset().

NOTES ABOUT BC

This PR maintains BC behaviour with theSession class in that the old Symfony 2.0 methods will continue to work as before.

@ghost
Copy link
Author

I think this is ready for review@fabpot@lsmith77

Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, ugly. done.

@lsmith77
Copy link
Contributor

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

@ghost
Copy link
Author

@lsmith77 Those differences are explained already in the changelog

  • AddedFlashBag. Flashes expire when retrieved byget() orall().
    This makes the implementation ESI compatible.
  • AddedAutoExpireFlashBag (default) to replicate Symfony 2.0.x auto expire behaviour of messages auto expiring
    after one page page load. Messages must be retrived byget() orall().

Copy link
Contributor

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.

Copy link
Member

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ah good. Just checking.

Copy link
Member

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
Copy link
Contributor

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

Copy link
Contributor

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();

Copy link
Author

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
Copy link
Contributor

@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
Copy link
Author

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

@ghost
Copy link
Author

Rebased against currentmaster, should be mergeable again..

@evillemez
Copy link

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

Copy link
Member

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.

fabpot added a commit that referenced this pull requestMar 23, 2012
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.
@fabpotfabpot merged commit5ae76f1 intosymfony:masterMar 23, 2012
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@lsmith77@Crell@evillemez@fabpot@stloyd@docteurklein@Seldaek@deviantintegral@stof

[8]ページ先頭

©2009-2025 Movatter.jp