[Bridge/Twig] Let getFlashes starts the session #25077
Conversation
| Q | A |
|---|---|
| Branch? | 3.4 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #25073 |
| License | MIT |
| @@ -258,7 +253,7 @@ protected function setTokenStorage($user) | |||
| $token->method('getUser')->willReturn($user); | |||
| } | |||
|
|
|||
| private function setFlashMessages() | |||
| private function setFlashMessages($startSession = true) | |||
There was a problem hiding this comment.
I think you should rename to sessionHasStarted
|
@MatTheCat can you change the base of the pull-request to be the 3.4 branch on the UI? |
|
@sroze done |
|
@MatTheCat I'm shocked about this proposal 😱 We created |
|
@javiereguiluz I thought the response was to check for |
|
@MatTheCat I don't know the internal details about this (@nicolas-grekas and @sroze will help us here) but if I ask Symfony whether there are flash messages or not ... I don't want the session to be started if it wasn't already started ... because that hurts app performance. I'm just asking if there are flash messages; I'm not getting them. That's why I think the session should not start. Thanks! |
|
@javiereguiluz but by returning an empty array how can the presence of flashes be checked? |
|
If flash messages are stored in the session ... and the session has not even been started ... the answer to "are there flash messages?" should be no ... and that's why we return an empty array, to say that there are no flash messages. |
|
@javiereguiluz AFAIK, the session is "started" when the backend has been read. I don't think we can know if any flash are set if we don't read the backend. |
|
@MatTheCat following your analogy, I'm saying: if I see there's no box, then I can say there's nothing inside the box. And you are saying (that's at least what I understand): if I see there's no box, then I'm going to create a new box an open it to see if there's something inside it. But @nicolas-grekas said this is OK, so it's clear that I'm not understanding this. So ignore my comments :) Thanks! |
|
Yeah okay I didn't understand the relation with #24523 |
|
@javiereguiluz I think the misunderstanding comes from what "session is started" means. When there is no session cookie, no "box" is created anymore. But when there is a session cookie, we must ask the backend for a corresponding "box" there. "Starting the session" means actually this: has the potentially existing box been opened? If not, telling there are no flashes there is just wrong. |
|
@javiereguiluz that's it! I don't know about performance but I would prefer to create an empty box than always saying it's empty. |
|
(My comment was "But you can't say a box is empty just because you don't want to open it?!") |
|
Thanks Nico. That was what I wasn't understanding: "starting a session" and "creating a session" were the same to me ... but they are not! Thanks! |
|
@MatTheCat would you mind rebasing on latest 3.4? (you can even squash) |
|
@nicolas-grekas done |
|
Thank you @MatTheCat. |
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
@nicolas-grekas If i have no session cookie and call app.flashes, new session and cookie is created. |
|
3.4 embeds 3.3 so that the article applies to it also. I can't tell from afar if it's legit for the session to be started or not. If you think it's a bug, please open a dedicated issue with a reproducer, i.e. a repository we could clone to run the issue locally. |
|
Sorry for delayed answer.
But blog post i mentioned previously, about improved flash messages states
|