Do not start session on get, if no prevoius session #6152
Conversation
|
@fabpot - If I am not mistaken this, and the related ticket is an example of why there should be an explicit session start as we had during development but we removed. While this PR might solve the problem now, it's just solving the problem that was already solved. IMO the better way is to put back the session autostart option (not to be confused with the PHP global ini). |
|
@Drak I am not sure that I follow you logic here. |
| * | ||
| * @var bool | ||
| */ | ||
| private $emptySession=false; |
|
A unit test should be added |
|
I guess I will have to read up on unit tests sooner or later. Where is a good place to start reading? |
|
@dlsniper I am afraid I am not so into the version system used by symfony. Could you please explain to me why this should be opened against the 2.1 branch? I thought the master branch was where all the development happened. |
|
@dlsniper - the problem with the PR is it's mixing up application level stuff with the session, it's also a BC break. The way things were before, with controllable autostart was much cleaner with a single session start point and explicit logic. |
|
@Drak How was things before? What do you mean by a controllable autostart, single session start point and explicit logic? Do you mean that the programmer must explicitly declare that he wants to start a session on every web-page that uses the session? |
|
In this PR, the concept "session empty" is not part of the session code, but part of the request and you are then flagging a property in the session to reflect this. You could equally just flag the session with a session variable. The information about whether the session is new or not is already stored as part of the session meta data. The autostart mechanism is now being controlled arbitrarily by whether you use the request object method or not according to an assume configuration which may or may not be. You are also assuming that the presence of data in the session bag makes the session empty - it does not. The session is a collection of bags. You could for example add some flashes but no session-bag variables so the logic is flawed. As pointed out in the ticket you can test if the session has started also in your application logic. This is a BC break (changing the interface is a BC break since anything implementing that interface must now implement the new method). The session code is also doing exactly what it's supposed to within the context of PHP sessions. That there maybe situations where you may not wish to start a session is not really part of the session logic, again it's a detail with your application. The best way forward would be for me to reintroduce logic that I was asked to remove that allows the control of the session autostart. Beyond that, the session logic should not be altered imo. |
No, here is where you are wrong. It could definitively not be a flag in a session variable, because in order to store something in the session, the session must be started. The whole point of this issue is that the session should not be started if it is only read from and it is no data in the session when there was no previous session. If there was a previous session, the session must be started as normal so that the data in the session can be retrieved.
Ok, may be it is better to check for the presence of a PHP session cookie directly in the session code then. I thought that belonged to the request object, because there was already code there that checked the PHP session cookie.
I am making no such assumption. Where do you get that from?
Not so, if flashes are added, my code would start the session as normal. You obviously haven't looked very closely at what my patch actually does.
That is true. But the point of this issue is that we should try to relieve the application programmer of that burden.
Here you assume that applications use the SessionInterface to implement their own session classes. I think that is very rare, but I guess you are right that it can be cons AA2A idered a BC break.
You assume that not starting the session is the exception. For some applications the normal thing is not to start the session, and they try to avoid it if it is possible. |
|
This pull request is now outdated. |
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #6036
License of the code: MIT
The session->get function should only start the session in the current request if the session had already been started in a previous request.