8000 [Twig Bridge] Getting flashes should start the session · Issue #25073 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Twig Bridge] Getting flashes should start the session #25073

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
MatTheCat opened this issue Nov 21, 2017 · 4 comments
Closed

[Twig Bridge] Getting flashes should start the session #25073

MatTheCat opened this issue Nov 21, 2017 · 4 comments
Labels
TwigBridge

Comments

@MatTheCat
Copy link
Contributor
MatTheCat commented Nov 21, 2017 •
edited
Loading
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.3.0

In 5a56b23 you can see app.flashes returns an empty array if the session hasn't started. Reading http://symfony.com/doc/current/session/avoid_session_start.html this seems really weird to me; this means I have to add app.session.start() after checking forapp.request.hasPreviousSession if I want to make sure any flash is displayed.

Is this really wanted? If we must check for app.request.hasPreviousSession then reading flashes could start the session.
@nicolas-grekas
Copy link
Member

I agree, this was a workaround that prevented starting the session when "just" checking for flashed, but now that the session is more lazy (since #24523), we could be more "correct" here.
PR welcomed.

@sroze
Copy link
Contributor
sroze commented Nov 21, 2017

You can definitely PR a removal of the isStarted() method call 👍

@MatTheCat
Copy link
Contributor Author
MatTheCat commented Nov 21, 2017 •
edited
Loading

Hmm should I base it on master or 3.3?

@sroze
Copy link
Contributor
sroze commented Nov 21, 2017 •
edited
Loading

I'd go from 3.4, as sessions have been improved massively from this version (which allows us to remove this isStarted(), not before)

@MatTheCat MatTheCat mentioned this issue Nov 21, 2017
Merged
fabpot added a commit that referenced this issue Nov 21, 2017
@fabpot
bug #25077 [Bridge/Twig] Let getFlashes starts the session (MatTheCat)
72a204e 
This PR was merged into the 3.4 branch.

Discussion
----------

[Bridge/Twig] Let getFlashes starts the session

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    |no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #25073
| License       | MIT

Commits
-------

a27f959 Let getFlashes starts the session
@fabpot fabpot closed this as completed Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
TwigBridge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @nicolas-grekas @sroze @MatTheCat @xabbuh
0