[symfony/twig-bundle] add flash messages to `base.html.twig` #1070

kbond · 2022-03-29T12:50:59Z

Q	A
License	MIT
Doc issue/PR	n/a

This adds out of the box flash message support to base.html.twig. If there is no session or no flashes, this block will be ignored.

github-actions · 2022-03-29T12:51:21Z

Thanks for the PR 😍

How to test these changes in your application

Define the SYMFONY_ENDPOINT environment variable:

# On Unix-like (BSD, Linux and macOS)
export SYMFONY_ENDPOINT=https://raw.githubusercontent.com/symfony/recipes/flex/pull-1070/index.json
# On Windows
SET SYMFONY_ENDPOINT=https://raw.githubusercontent.com/symfony/recipes/flex/pull-1070/index.json

Install the package(s) related to this recipe:

composer req 'symfony/flex:^1.16'
composer req 'symfony/twig-bundle:^5.4'

Don't forget to unset the SYMFONY_ENDPOINT environment variable when done:

# On Unix-like (BSD, Linux and macOS)
unset SYMFONY_ENDPOINT
# On Windows
SET SYMFONY_ENDPOINT=

Diff between recipe versions

In order to help with the review stage, I'm in charge of computing the diff between the various versions of patched recipes.
I'm going keep this comment up to date with any updates of the attached patch.

symfony/twig-bundle

3.3 vs 4.4

diff --git a/symfony/twig-bundle/3.3/config/packages/twig.yaml b/symfony/twig-bundle/4.4/config/packages/twig.yaml
index d1582a2..6403e6a 100644
--- a/symfony/twig-bundle/3.3/config/packages/twig.yaml
+++ b/symfony/twig-bundle/4.4/config/packages/twig.yaml
@@ -2,3 +2,4 @@ twig:
     default_path: '%kernel.project_dir%/templates'
     debug: '%kernel.debug%'
     strict_variables: '%kernel.debug%'
+    exception_controller: null
diff --git a/symfony/twig-bundle/3.3/config/routes/dev/twig.yaml b/symfony/twig-bundle/3.3/config/routes/dev/twig.yaml
deleted file mode 100644
index f4ee839..0000000
--- a/symfony/twig-bundle/3.3/config/routes/dev/twig.yaml
+++ /dev/null
@@ -1,3 +0,0 @@
-_errors:
-    resource: '@TwigBundle/Resources/config/routing/errors.xml'
-    prefix: /_error

4.4 vs 5.0

diff --git a/symfony/twig-bundle/4.4/config/packages/twig.yaml b/symfony/twig-bundle/5.0/config/packages/twig.yaml
index 6403e6a..b3cdf30 100644
--- a/symfony/twig-bundle/4.4/config/packages/twig.yaml
+++ b/symfony/twig-bundle/5.0/config/packages/twig.yaml
@@ -1,5 +1,2 @@
 twig:
     default_path: '%kernel.project_dir%/templates'
-    debug: '%kernel.debug%'
-    strict_variables: '%kernel.debug%'
-    exception_controller: null

5.0 vs 5.3

diff --git a/symfony/twig-bundle/5.0/config/packages/test/twig.yaml b/symfony/twig-bundle/5.0/config/packages/test/twig.yaml
deleted file mode 100644
index 8c6e0b4..0000000
--- a/symfony/twig-bundle/5.0/config/packages/test/twig.yaml
+++ /dev/null
@@ -1,2 +0,0 @@
-twig:
-    strict_variables: true
diff --git a/symfony/twig-bundle/5.0/config/packages/twig.yaml b/symfony/twig-bundle/5.3/config/packages/twig.yaml
index b3cdf30..f9f4cc5 100644
--- a/symfony/twig-bundle/5.0/config/packages/twig.yaml
+++ b/symfony/twig-bundle/5.3/config/packages/twig.yaml
@@ -1,2 +1,6 @@
 twig:
     default_path: '%kernel.project_dir%/templates'
+
+when@test:
+    twig:
+        strict_variables: true
diff --git a/symfony/twig-bundle/5.0/manifest.json b/symfony/twig-bundle/5.3/manifest.json
index 30d5643..c4835ac 100644
--- a/symfony/twig-bundle/5.0/manifest.json
+++ b/symfony/twig-bundle/5.3/manifest.json
@@ -5,5 +5,8 @@
     "copy-from-recipe": {
         "config/": "%CONFIG_DIR%/",
         "templates/": "templates/"
+    },
+    "conflict": {
+        "symfony/framework-bundle": "<5.3"
     }
 }

5.3 vs 5.4

diff --git a/symfony/twig-bundle/5.3/templates/base.html.twig b/symfony/twig-bundle/5.4/templates/base.html.twig
index 16d7273..03befc1 100644
--- a/symfony/twig-bundle/5.3/templates/base.html.twig
+++ b/symfony/twig-bundle/5.4/templates/base.html.twig
@@ -3,17 +3,29 @@
     <head>
         <meta charset="UTF-8">
         <title>{% block title %}Welcome!{% endblock %}</title>
-        {# Run `composer require symfony/webpack-encore-bundle`
-           and uncomment the following Encore helpers to start using Symfony UX #}
+        <link rel="icon" href="data:image/svg+xml,<svg xmlns=%22http://www.w3.org/2000/svg%22 viewBox=%220 0 128 128%22><text y=%221.2em%22 font-size=%2296%22>⚫️</text></svg>">
+        {# Run `composer require symfony/webpack-encore-bundle` to start using Symfony UX #}
         {% block stylesheets %}
-            {#{{ encore_entry_link_tags('app') }}#}
+            {{ encore_entry_link_tags('app') }}
         {% endblock %}
 
         {% block javascripts %}
-            {#{{ encore_entry_script_tags('app') }}#}
+            {{ encore_entry_script_tags('app') }}
         {% endblock %}
     </head>
     <body>
+        {% block flashes %}
+            {% if app.request.hasPreviousSession %}
+                {% for type, messages in app.flashes %}
+                    {% for message in messages %}
+                        <div class="alert alert-{{ type }}" role="alert">
+                            {{ message }}
+                        </div>
+                    {% endfor %}
+                {% endfor %}
+            {% endif %}
+        {% endblock %}
+
         {% block body %}{% endblock %}
     </body>
 </html>

symfony/twig-bundle/5.4/templates/base.html.twig

nicolas-grekas · 2022-03-29T13:01:10Z

This means opening the session on every single page. I don't think that's a good default for this reason...

kbond · 2022-03-29T13:03:44Z

From what I am seeing, app.flashes is an empty array of there is no session: https://github.com/symfony/symfony/blob/6.1/src/Symfony/Bridge/Twig/AppVariable.php#L136

Edit: think I'm wrong about this: https://github.com/symfony/symfony/blob/6.1/src/Symfony/Component/HttpFoundation/Request.php#L729. We'd need to pass true to Request::hasSession() to have this behaviour, correct? But then, the flashes wouldn't show unless the session was previously initialized...

symfony/twig-bundle/5.4/templates/base.html.twig

kbond · 2022-03-29T16:56:16Z

Is there a way to check if a request has a "previous" session without initializing it? If not, should there be? What about a SessionFactory::getName() method that just returns the expected session name. Then, in Request::hasPreviousSession(), if the session isn't initialized, we check if a cookie with this name exists.

symfony/twig-bundle/5.4/templates/base.html.twig

nicolas-grekas · 2022-04-29T16:27:31Z

I'm sorry but my previous comment will always be true: no matter how you write it, there is no way to have both flash messages and http caching when the session is used as the messaging protocol.

The current logic preserves http caching when no session is used on ANY page of the app. This covers an extremely narrow set of Symfony apps: static websites.

We'd better close this PR as this approach will never work.

We need to look for a different protocol if we want to reconciliate flashes and caching.

nicolas-grekas · 2022-04-29T16:40:45Z

Another opinion on this one: let's say we merge this. It'd mean we'd make it harder to use http caching since ppl would first need to figure out the issue and then work around it.

We might be fine making everything state-full by default in favor of DX, but then we need to not make the DX too much worse for ppl that want to achieve caching.

That gives us two very different ways forward IMHO :

Look for another protocol (will require JS on the client side for sure)
Make things state-full by default but figure out an easy/flexible path to opt-out from it.

kbond · 2022-04-29T16:58:08Z

Make things state-full by default but figure out an easy/flexible path to opt-out from

I guess having the user disable the session globally is not what you mean here?

nicolas-grekas · 2022-04-29T17:02:41Z

You're right, disabling the session is just 🙈 :)
This concern happens on a page by page basis, so we could look for a way to display/ignore flashes page by page.

gnito-org · 2022-04-29T17:17:07Z

I use the Request::hasPreviousSession() approach simply to prevent sessions from being created by unauthenticated visitors. It works, because there is no cookie created until such time that the user logs in.

kbond · 2022-04-29T17:25:22Z

This concern happens on a page by page basis, so we could look for a way to display/ignore flashes page by page.

What about:

{% block flashes %}
    {% if app.request.hasPreviousSession %}
        {% for type, messages in app.flashes %}
            {% for message in messages %}
                <div class="alert alert-{{ type }}" role="alert">
                    {{ message }}
                </div>
            {% endfor %}
        {% endfor %}
    {% endif %}
{% endblock %}

The flashes could be disabled on a page-by-page bases or in a sub-layout (no_flash_base.html.twig).

gnito-org · 2022-04-29T17:31:47Z

Hmm... upon further testing:

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

also does not create a cookie for unauthenticated users.

Perhaps this is something that changed in Symfony 6.x? A few years ago I added the Request::hasPreviousSession() because without it a session was created for unauthenticated users when you accessed the flashbag. At that time my sessions were stored in MySQL and it was severely impacting performance of the home page.

dunglas · 2022-04-30T15:08:28Z

We could at least put that in a render_esi() call. It will work by default for everybody, but will stay HTTP cache-friendly (you'll just have no cache on this specific ESI block).

kbond · 2022-05-01T01:11:08Z

We could at least put that in a render_esi() call. It will work by default for everybody, but will stay HTTP cache-friendly (you'll just have no cache on this specific ESI block).

Won't that require a controller/route/another template?

dunglas · 2022-05-01T06:14:59Z

Just another template. You can render it using the builtin TemplateController with something like this: render_esi(controller('Symfony\Bundle\FrameworkBundle\Controller\TemplateController', {template: 'flashes.html.twig'}))

gnito-org · 2022-05-01T09:00:57Z

Flashes are usually specific to the authenticated user, or can contain a mixture of public and private alerts. Should those be http-cached?

kbond · 2022-05-01T11:32:28Z

Just another template.

Oh right, that would work.

Should those be http-cached?

They wouldn't be if using render_esi - they'd be rendered in a sub-request that wouldn't be cached.

nicolas-grekas · 2022-05-19T13:36:41Z

I thought about it and I'm 👎
Instead, I think we should promote flash messages based on turbo-frames.

kbond · 2022-05-19T13:40:50Z

Ok fair enough. Closing.

gnito-org · 2022-05-19T15:24:24Z

Doesn't that decision assume that everyone is going the Single Page App route?

Personally I hate anything that breaks the functionality of the browser Back button. Isn't that what happens when you include a bunch of independent frames in the web page?

From https://turbo.hotwired.dev/handbook/introduction#turbo-frames-decompose-complex-pages:

With Turbo Frames, you can place those independent segments inside frame elements that can scope their navigation and be lazily loaded. Scoped navigation means all interaction within a frame, like clicking links or submitting forms, happens within that frame, keeping the rest of the page from changing or reloading.

So, if something in a Turbo frame changes, the back button is not going to restore the page to its previous state, right?

weaverryan · 2022-05-19T15:29:07Z

Yea, I'm not sure I follow the turbo frame idea either.

I understand that we may not be able to put flash messages into base.html.twig by default, but it's still what I'll be recommending, unless something else comes along. I don't see a downside for most apps.

fabpot · 2022-05-19T15:40:34Z

Reopening as Turbo cannot be the "solution" for everyone.

gnito-org · 2022-05-19T17:22:04Z

In flash messages I often include a link to more information in a help article. With Turbo frames that link will open in the flash frame, or you need to force a new window. Both options are bad.

github-actions bot enabled auto-merge (squash) March 29, 2022 12:51

Guite reviewed Mar 29, 2022

View reviewed changes

symfony/twig-bundle/5.4/templates/base.html.twig Outdated Show resolved Hide resolved

kbond force-pushed the twig-base-flash branch from 315874a to f2a2e58 Compare March 29, 2022 12:54

Kocal reviewed Mar 29, 2022

View reviewed changes

symfony/twig-bundle/5.4/templates/base.html.twig Outdated Show resolved Hide resolved

kbond mentioned this pull request Mar 31, 2022

[RFC][WIP] add scaffolding system (make:scaffold) symfony/maker-bundle#1085

Open

9 tasks

javiereguiluz reviewed Apr 29, 2022

View reviewed changes

symfony/twig-bundle/5.4/templates/base.html.twig Outdated Show resolved Hide resolved

kbond force-pushed the twig-base-flash branch from f2a2e58 to 975609f Compare April 29, 2022 13:34

dunglas previously approved these changes Apr 29, 2022

View reviewed changes

[symfony/twig-bundle] add flash messages to base.html.twig

ed3c383

kbond dismissed dunglas’s stale review via ed3c383 April 29, 2022 18:10

kbond force-pushed the twig-base-flash branch from 975609f to ed3c383 Compare April 29, 2022 18:10

kbond closed this May 19, 2022

auto-merge was automatically disabled May 19, 2022 13:40
Pull request was closed

fabpot reopened this May 19, 2022

github-actions bot enabled auto-merge (squash) May 19, 2022 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[symfony/twig-bundle] add flash messages to `base.html.twig` #1070

[symfony/twig-bundle] add flash messages to `base.html.twig` #1070

kbond commented Mar 29, 2022

github-actions bot commented Mar 29, 2022 •

edited

nicolas-grekas commented Mar 29, 2022

kbond commented Mar 29, 2022 •

edited

kbond commented Mar 29, 2022 •

edited

nicolas-grekas commented Apr 29, 2022

nicolas-grekas commented Apr 29, 2022 •

edited

kbond commented Apr 29, 2022

nicolas-grekas commented Apr 29, 2022

gnito-org commented Apr 29, 2022

kbond commented Apr 29, 2022

gnito-org commented Apr 29, 2022 •

edited

dunglas commented Apr 30, 2022 •

edited

kbond commented May 1, 2022

dunglas commented May 1, 2022

gnito-org commented May 1, 2022

kbond commented May 1, 2022

nicolas-grekas commented May 19, 2022

kbond commented May 19, 2022

gnito-org commented May 19, 2022

weaverryan commented May 19, 2022

fabpot commented May 19, 2022

gnito-org commented May 19, 2022

[symfony/twig-bundle] add flash messages to base.html.twig #1070

Are you sure you want to change the base?

[symfony/twig-bundle] add flash messages to base.html.twig #1070

Conversation

kbond commented Mar 29, 2022

github-actions bot commented Mar 29, 2022 • edited

How to test these changes in your application

Diff between recipe versions

symfony/twig-bundle

nicolas-grekas commented Mar 29, 2022

kbond commented Mar 29, 2022 • edited

kbond commented Mar 29, 2022 • edited

nicolas-grekas commented Apr 29, 2022

nicolas-grekas commented Apr 29, 2022 • edited

kbond commented Apr 29, 2022

nicolas-grekas commented Apr 29, 2022

gnito-org commented Apr 29, 2022

kbond commented Apr 29, 2022

gnito-org commented Apr 29, 2022 • edited

dunglas commented Apr 30, 2022 • edited

kbond commented May 1, 2022

dunglas commented May 1, 2022

gnito-org commented May 1, 2022

kbond commented May 1, 2022

nicolas-grekas commented May 19, 2022

kbond commented May 19, 2022

gnito-org commented May 19, 2022

weaverryan commented May 19, 2022

fabpot commented May 19, 2022

gnito-org commented May 19, 2022

[symfony/twig-bundle] add flash messages to `base.html.twig` #1070

[symfony/twig-bundle] add flash messages to `base.html.twig` #1070

github-actions bot commented Mar 29, 2022 •

edited

kbond commented Mar 29, 2022 •

edited

kbond commented Mar 29, 2022 •

edited

nicolas-grekas commented Apr 29, 2022 •

edited

gnito-org commented Apr 29, 2022 •

edited

dunglas commented Apr 30, 2022 •

edited