[Twig Bridge] A simpler way to retrieve flash messages #21819

javiereguiluz · 2017-03-01T16:57:32Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

Getting flash messages in templates is more complex than it could be. Main problems:

It's too low level: you need to get the "flash bag" (and first, learn what a "flash bag" is) and then you need to call the internal method: all(), get(), etc.
You need to be careful because the session will start automatically when you ask for flashes (even if there are no flashes). You can prevent this with the {% if app.session is not null and app.session.started %} code, but it's boring to always use that.

So, I propose to add a new app.flashes helper that works as follows.

Get all the flash messages

Before

{% if app.session is not null and app.session.started %}
    {% for label, messages in app.session.flashbag.all %}
        {% for message in messages %}
            <div class="alert alert-{{ label }}">
                {{ message }}
            </div>
        {% endfor %}
    {% endfor %}
{% endif %}

After

{% for label, messages in app.flashes %}
    {% for message in messages %}
        <div class="alert alert-{{ label }}">
            {{ message }}
        </div>
    {% endfor %}
{% endfor %}

Get only the flashes of type `notice`

{% if app.session is not null and app.session.started %}
    {% for message in app.session.flashbag.get('notice') %}
        <div class="alert alert-notice">
            {{ message }}
        </div>
    {% endfor %}
{% endif %}

After

{% for message in app.flashes('notice') %}
    <div class="alert alert-notice">
        {{ message }}
    </div>
{% endfor %}

As an added bonus, you can get any number of flash messages because the method allows to pass an array of flash types:

{% for label, messages in app.flashes(['warning', 'error']) %}
    {% for message in messages %}
        <div class="alert alert-{{ label }}">
            {{ message }}
        </div>
    {% endfor %}
{% endfor %}

stof · 2017-03-01T17:11:06Z

Having a simpler API is a good idea.

But I'm really not a fan of the magic API, which returns a nested array when you give 0 arguments or >2, but a flat array when you pass 1 argument. This is too magic.

yceruto · 2017-03-01T17:16:18Z

src/Symfony/Bridge/Twig/AppVariable.php

+     * Returns some or all the existing flash messages. The method is variadic:
+     * if no arguments are passed, all flashes are returned; if one or more
+     * arguments are passed, only the flashes that belong to that category are
+     * returned; e.g. getFlashes('notice') or getFlashes('notice', 'error').


Also we could support array of categories? i.e: getFlashes(['notice', 'error']), sometimes this information is configurable.

stof · 2017-03-01T17:19:05Z

btw, to get some types only, an array would be better than a variadic function IMO. Twig does not have the spread operator, and does not have a call_user_func_array equivalent to allow building arguments dynamically

javiereguiluz · 2017-03-02T21:49:25Z

I've replaced the variadic arguments by an array:

Code	Result
`app.flashes`	nested array with all flashes
`app.flashes('notice')`	simple array with flashes of that type
`app.flashes(['notice'])`	nested array with type => messages
`app.flashes(['notice', 'warning'])`	nested array with types => messages

@stof I take your comment into consideration, but in this case, instead of "magic API" I think it's "smart/flexible API". It's similar to when Symfony lets you pass a string to a config option that needs an array and does the string -> array conversion automatically.

I think that returning a nested array when you ask for the flash messages of a single type is cumbersome:

{# you would need to use this... #}
{% for message in app.flashes('notice')['notice'] %}
    <div class="alert alert-notice">
        {{ message }}
    </div>
{% endfor %}

{# ... or this ... #}
{% for type, messages in app.flashes('notice') %}
    {% for message in messages %}
        <div class="alert alert-notice">
            {{ message }}
        </div>
    {% endfor %}
{% endfor %}

{# ... instead of this: #}
{% for message in app.flashes('notice') %}
    <div class="alert alert-notice">
        {{ message }}
    </div>
{% endfor %}

But I'm open to other comments and opinions about this. Let's work together to make a great API that doesn't feel magic! Thanks.

HeahDude · 2017-03-02T22:02:15Z

src/Symfony/Bridge/Twig/AppVariable.php

+        if (null === $types) {
+            return $session->getFlashBag()->all();
+        }
+


You should cast $types to an array first:

$type = (array) $type;

chalasr · 2017-03-02T22:09:12Z

src/Symfony/Bridge/Twig/AppVariable.php

+
+        if (1 === count($types)) {
+            return $session->getFlashBag()->get($types[0]);
+        }


Calling this method with a string like "notice" as argument would make this check pass and get('n') to be called

oops, @HeahDude commented meanwhile :)

Either way.. offset 0 may not exist :) What about ->get(reset($types))?

Oh and it violates the api (more or less) with getFlashes(['notice']) (i'd expect a nested array... but you get a flat one).

edit: yeah.. already pointed out by @stof , but it's now advertised with

app.flashes(['notice']) nested array with type => messages

@ro0NL good catch about the potential non-existent 0 offset. I've added ... || empty($types) condition to return all the flashes in that case.

ro0NL · 2017-03-03T19:10:36Z

Not sure about the mixed api.. i could live with it i guess. In twig context it's probably less an issue, and once you know it you're good to go :)

Some other thoughts

getFlashes($type = null) either all types nested, or a single type flat
getFlashes(array $types = null) for nested / getFlashesFor(array $types = null) for flat
{{ message.type ~ message.value }} flat with a nested message structure

ro0NL · 2017-03-03T19:16:08Z

src/Symfony/Bridge/Twig/AppVariable.php

+            if (null !== $session && !$session->isStarted()) {
+                return array();
+            }
+        } catch (\RuntimeException $e) {


This may be too broad, only to bypass The "app.session" variable is not available.. I guess it's an edge case :) but maybe do a quick return array() as well if there's no request stack and drop the try/catch.

Yes ... it's tricky. But avoiding the try...catch would require to duplicate some of the code of this very same class.

I would check requestStack here, and rely on getSession to be available otherwise.

ro0NL · 2017-03-04T12:08:41Z

src/Symfony/Bridge/Twig/AppVariable.php

+            return $session->getFlashBag()->all();
+        }
+
+        $types = (array) $types;


Again.. offset 0 may not exist ;-) also the API is still inconsistent with flashes(['single']) vs. flashes('single'). Both return flat, not just the latter. Like @stof mentioned.. this is too magic.

What about

if (null === $types || array() === $types) { return $session->getFlashBag()->all(); } if (!is_array($types)) { return $session->getFlashBag()->get($types); } // intersect

The empty check is too magic (for me) as well; passing "0", "", etc. should be passed to get() IMO.

I agree that's not very intuitive. Imagine the case where the exact parameter passed to the Twig function is dynamic. You then don't know beforehand whether or not you will get a flat or a nested array. What we could however is to always return the nested array if you pass an array as the argument (no matter the number of arguments) and get the flat result when a string is given.

javiereguiluz · 2017-03-04T14:11:03Z

Hopefully I have the code right this time. The idea is:

If you pass a string, you get a simple array with just those messages
If you pass an array, you get an array of messages
Otherwise, you get all messages ... except when the session hasn't started, where you get an empty array

fabpot · 2017-03-23T14:59:10Z

src/Symfony/Bridge/Twig/AppVariable.php

+            return array();
+        }
+
+        if (empty($types)) {


null !== $type || array() === $types to allow 0 as a type.

fabpot · 2017-03-23T15:03:31Z

👍

fabpot · 2017-03-23T15:48:21Z

Thank you @javiereguiluz.

This PR was squashed before being merged into the master branch (closes #7684). Discussion ---------- #7676 - Flash messages This pull request fixes #7676 that documents the flash messages that are been changed in symfony/symfony#21819. Commits ------- 467de14 #7676 - Flash messages

Albert221 · 2017-11-26T16:32:47Z

Hey @javiereguiluz, can you explain to me, why starting session once getting flash messages is not an intended behaviour, please?

I had given scenario: on 1st request, flash message and redirect user; on 2nd request, I have app.flashes('message') in Twig, but it returns nothing, because I do not start session anywhere, I initially want it to exist, since I want to retrieve this flash, right?

sroze · 2017-11-26T18:00:04Z

@Albert221 this has been fixed in this PR: https://github.com/symfony/symfony/pull/25077/files

[Twig Bridge] A simpler way to retrieve flash messages

8ab885d

javiereguiluz added DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature TwigBridge labels Mar 1, 2017

carsonbot added the Status: Needs Review label Mar 1, 2017

javiereguiluz added 2 commits March 1, 2017 18:00

Added the case for just 1 flash message key

9d471dc

Fixed code syntax

e05144d

yceruto reviewed Mar 1, 2017

View reviewed changes

Simplified everything

e42bd57

HeahDude reviewed Mar 2, 2017

View reviewed changes

chalasr reviewed Mar 2, 2017

View reviewed changes

ro0NL reviewed Mar 3, 2017

View reviewed changes

nicolas-grekas added this to the 3.x milestone Mar 4, 2017

Fixes

27709f3

javiereguiluz added Status: Reviewed and removed Status: Needs Review labels Mar 4, 2017

ro0NL reviewed Mar 4, 2017

View reviewed changes

Fixed code and added tests

3a0b379

Fixed code standard

7acb794

fabpot reviewed Mar 23, 2017

View reviewed changes

javiereguiluz added 3 commits March 23, 2017 16:04

Don't use the empty() PHP function

16a794d

Fixed the condition

c916aee

Added another condition needed when replacing empty()

20eba3f

fabpot closed this in db8d87d Mar 23, 2017

xabbuh mentioned this pull request Mar 23, 2017

Improved flash messages handling in Twig templates symfony/symfony-docs#7676

Closed

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

ghost mentioned this pull request Mar 25, 2017

#7676 - Flash messages symfony/symfony-docs#7684

Closed

King2500 mentioned this pull request Mar 26, 2017

[Twig] Wrong type hint for app variable in Symfony 2.7+ and 3.x Haehnchen/idea-php-symfony2-plugin#903

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

JulienChavee mentioned this pull request May 30, 2017

Upgrade symfony JulienChavee/bruminator2.0#94

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Twig Bridge] A simpler way to retrieve flash messages #21819

[Twig Bridge] A simpler way to retrieve flash messages #21819

javiereguiluz commented Mar 1, 2017 •

edited

Loading

stof commented Mar 1, 2017

yceruto Mar 1, 2017

stof commented Mar 1, 2017

javiereguiluz commented Mar 2, 2017

HeahDude Mar 2, 2017

chalasr Mar 2, 2017

chalasr Mar 2, 2017 •

edited

Loading

ro0NL Mar 3, 2017

ro0NL Mar 3, 2017 •

edited

Loading

javiereguiluz Mar 4, 2017

ro0NL commented Mar 3, 2017 •

edited

Loading

ro0NL Mar 3, 2017 •

edited

Loading

javiereguiluz Mar 4, 2017

ro0NL Mar 4, 2017

ro0NL Mar 4, 2017

xabbuh Mar 4, 2017

javiereguiluz commented Mar 4, 2017

fabpot Mar 23, 2017

fabpot commented Mar 23, 2017

fabpot commented Mar 23, 2017

Albert221 commented Nov 26, 2017

sroze commented Nov 26, 2017

[Twig Bridge] A simpler way to retrieve flash messages #21819

[Twig Bridge] A simpler way to retrieve flash messages #21819

Conversation

javiereguiluz commented Mar 1, 2017 • edited Loading

Get all the flash messages

Before

After

Get only the flashes of type notice

After

stof commented Mar 1, 2017

Choose a reason for hiding this comment

stof commented Mar 1, 2017

javiereguiluz commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL commented Mar 3, 2017 • edited Loading

ro0NL Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Mar 4, 2017

Choose a reason for hiding this comment

fabpot commented Mar 23, 2017

fabpot commented Mar 23, 2017

Albert221 commented Nov 26, 2017

sroze commented Nov 26, 2017

javiereguiluz commented Mar 1, 2017 •

edited

Loading

Get only the flashes of type `notice`

chalasr Mar 2, 2017 •

edited

Loading

ro0NL Mar 3, 2017 •

edited

Loading

ro0NL commented Mar 3, 2017 •

edited

Loading

ro0NL Mar 3, 2017 •

edited

Loading