Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Checking whether there is data in the session, creates a session #6036

Open
disposable-ksa98 opened this Issue · 58 comments
@disposable-ksa98

UPDATE: This issue has 2 parts. The first one is explained in this post. The second one is explained in this comment.

I noticed the website I am developing was sending a cookie in the response, even for anonymous requests (ie: not logged in, and deleted all cookies).

After trying everything and failing to find the problem, I created a "hello world" with a fresh install, and there was no cookie.

But as soon as I added this, the cookie came back:

{% for flashMessage in app.session.flashbag.get('success') %}
    <div class="alert alert-success">
        <button type="button" class="close" data-dismiss="alert">×</button>
        {{ flashMessage }}
    </div>
{% endfor %}

So apparently app.session.flashbag.get() creates the session. I'm sure many other parts of my website are unintentionally creating a session, and that's why I couldn't find the culprit.

Is there a reason for this? Isn't it adding overhead that could have been avoided? I imagine this won't affect sites that are login-only, as they will need the session anyway for most of the visits. But sites that can be used anonymously might suffer a bit.

@ghost

The session once had to be specifically started, but it was changed to autostart when used by request.

@morgen2009

Just add in your template something like {% if app.session.started %} ... {% endif %}

@disposable-ksa98

@morgen2009 Thanks, that helped. I still think get() should not start the session though. Why add a session for every visit to any page in the website? (I disabled lots of things in my real website but still can't find where I'm not checking if the session started, before using it) I haven't checked it yet, but if this also adds an empty session to the database (when using a db as storage for sessions) then it's definitely wrong IMO. This performance detail is similar to that of using a different domain for static assets, to avoid sending/receiving cookies when not necessary.

@drak ping

@dlsniper

Hi there,

It seems that the changes in this PR #4541 have been reverted somehow and this is why the session is not correctly reported as being active or not by the WDT for example.
I'll investigate right now to see what happened.

Best regards.

@dlsniper

Seems @fabpot didn't agreed with the change of the functionality but in this case I'm not sure how to apply the change as the functionality is currently broken for 2.1 and newer and HttpFoundation/Request is marked with @api.
While the change is simple I'm not sure how to proceed about it as imo the hasSession() should be changed to reflect the functionality of 2.0.X branch and not introduce a BC break as well.

Any thoughts?

@dlsniper

@disposable-ksa98 If you do a get() operation from the session then it should be activated, what doesn't seem right to you? There's no other way to check if the variable is in session or not unless you start it. The fact that the WDT displays the session as always active it's a bug from a previous commit, see my comment above.

I've just tested this against master: fresh copy of master + session in memcached. When you enter the default page, WDT displays that you have a active session in your request but if you apply the change from #4541 in the Request.php file, then you'll receive the right values when checking if the session is indeed started or not.

@disposable-ksa98

@dlsniper I'm not using the WDT to check the session, I'm checking the http headers and the cookies in my browser. And I originally detected the problem in production, so...

To sum up:
If I'm not storing anything in the session, why is there a session cookie? It's unnecessary overhead, the fastest framework shouldn't do that.

Checking whether there is data in the session should not create a session. I don't know much about Sf2's internals, but if I were to design a session component, I would probably do something like this:

function get($key) {
    // If the request didn't give us a cookie, and we didn't set() it ourselves,
    // why do anything else? Just return null.
    if( ! $this->session) { return null; }

    // find and return the value for that key
}

Another option could be to throw an exception: "There is no session to get values from". That would be correct but more annoying to use, because every time you want to check for a key/value you will have to first ask hasSession() or something like that (I would prefer that though, rather than the current situation). Session should be started only if the request comes with a session cookie, or if we set() a key/value.

I'm sure many bundles and extensions are doing checks on the session, without knowing they are starting it, because we don't have the proposed exception, so they didn't know they had to first check whether the session existed.

@dlsniper

@disposable-ksa98 I don't think that the approach is good, either with throwing an exception or returning null.
If you write: $this->get('session')->get($key); then I really see no point in not starting the session automatically for you.

Also, using exceptions as flow control methods is wrong! Look only how the code would look like:

try {
    $this->get('session')->get($key);
} catch (SessionNotStartedException $e) {
    // Now what? 

    //Start the session and get the value again?
    $this->get('session')->start();
    $this->get('session')->get($key);

    // Or just ignore it?

    // Other option?
}

Imagine the above code in a for fetching things from the session, like flashes. And what happens if you don't have any of that code in the controller/service/model/whatever and you have it in the templates? {% app.session.start %}

Using $this->get('session')->hasSession(); would be a sufficient check for your application in order to know if it has a session or not and the either fetch the values from the session.

And thinking the functional way of making things, do you remember who all apps have session_start() on the very first line? Or if they don't have that, what a pita is to actually have all the session detection for each read/write you are going to do?

You could disagree with me, and I'm sorry to say it, but it seems that your application is actually poorly thought / written rather that a problem in the framework.

And I'm fairly sure that with bundles using the session then they might actually need it, that's what hasSession() function is for.

@disposable-ksa98

@dlsniper I got emotional when you said my application is poorly thought, so I'll start over and just ignore that part. All what I'm gonna say is that the only part in my own code that was doing this, was that flash snippet. Code that does not check for hasSession() is all over the place apparently, probably in things that aren't enabled by default.

Nowhere in the framework's documentation it says that getting the session will start the session. And calling hasSession() is not being enforced by the framework's code either (no exceptions thrown).

Don't tell me what I'm saying is ridiculous, I don't know the internals, I can't give you a fix. I don't even know the exact moment when the session is started. Is it when it's created by the DIC? Is it on $session->get() ?

The code you posted could be changed to this:

if there is a session
    do something with the session
endif

If someone tries to do it your way but without the try/catch, will get an exception soon enough and learn that he must either check hasSession() or catch the exception. I didn't want to force people to use try/catch everywhere, only force them to read "you must check hasSession() first!". That way, if I install a third party bundle that uses the session component, I will know I won't be sending an unwanted cookie by mistake, without reading thousands of third party LoC.
Or, the more friendly approach: get() returns null. I don't see how the creation of the session object or calling get() must start the session no matter what. What's so wrong with this?:

$this->get('session')->get('bla');
/**
 * Does not start the session, and a cookie is not sent,
 * because this is a smart object that can make a simple
 * check before doing that.
 */

For a moment imagine you are trying to "sell" the framework and you are asked: "If I didn't store anything in the session, why are you sending a session cookie? Is that good for performance?".
What would you answer?

Another interesting scenario to think about: In some european websites I've been welcome with a modal box telling me something like "By the law 1234 we are forced to ask for your approval to receive cookies. Cookies are X, that are used for Y". What would they do if they used Symfony2? They would be in trouble.

@dlsniper

First of all, I apologize I was rude to you without having a reason to be so and even then I shouldn't have been so. Reading over and over again helped me realize what you wanted to say :)

Second, I've now finally been able to see that the session driver itself is broken, it tries to store the serialized object like this: _sf2_attributes|a:0:{}_sf2_flashes|a:0:{}_sf2_meta|a:3:{s:1:"u";i:1353365612;s:1:"c";i:1353365612;s:1:"l";s:1:"0";}.

Third, I'll come up with a fix for the driver ASAP but like I've said, the other request, to not autostart the session doesn't seem reasonable to me, if you want, open a PR for this or a RFC and see the feedback, I won't interfere with it.

Fourth, having the session started even if not used at all is a bad idea for performance indeed hence I'll try and fix it asap. As for the EU law, you can do whatever the rest of the websites do: We are going to use these cookies to make your experience better. And IIRC, the EU law doesn't say anything about local storage so it would be nice to see a new way to bypass the thingy by having the local storage store the 'former cookie' info then pass it via AJAX :D But I don't suggest you to break the law!

@dlsniper

Oh and one more detail, what version of PHP are you using? I'm on 5.4.6 / Ubuntu 12.10. Thanks!

@cs278

@dlsniper FYI the EU directive applies to all forms of client side storage

@dlsniper

It seems that I had:


                {% for flashMessage in app.session.flashbag.get('notice') %}
                    <div class="flash-message">
                        <em>Notice</em>: {{ flashMessage }}
                    </div>
                {% endfor %}

in my template, using AcmeDemo, it would indeed start the session. Once I've wrapped it with {% if app.session.started %} ... {% endif %} and removed the existing cookie it would start behaving correctly and not send the cookie.

So to sum it up. There's indeed a 'issue' when you don't store anything in the session but you use it to retrieve flashes/data that could be in it without checking if the session is started or not first.

Then there's a section in the manual that explains the sessions and it documents the behavior pretty well: http://symfony.com/doc/master/components/http_foundation/sessions.html saying
While it is recommended to explicitly start a session, a sessions will actually start on demand,
that is, if any session request is made to read/write session data.

Also, any of the suggested methods for starting a session / returning null / throwing an exception won't guarantee you that a bundle won't do the same thing to start a session, think about it ;)

@disposable-ksa98

@dlsniper I'm using PHP 5.3.10-1ubuntu3.4 with Suhosin-Patch, and Ubuntu 12.04

To me, the way it works right now, is anti-intuitive and unwanted behavior. Just like when you pass an invalid argument, and the framework throws InvalidArgumentException, trying to get something from a non-existent session should either issue a warning/throw an exception, or just return null.

Yes I've already seen that checking for app.session.started prevents the cookie issue, I said it here. But we shouldn't rely on people remembering to do that. Also, code that comes with the framework is already making this mistake (let alone third party bundles or extensions)

Please compare these two and tell me if it's not doable and good.

// Note: I haven't read the source code, this is only to show the concept

// Current situation
class Session {
    public function __construct($request) {
        // Someone asked for the session, I'll just start it even if this might
        // add unnecessary overhead and unwanted or even illegal cookies
        $this->startSession();
    }
    public function get($key) {
        // return value
    }
    public function set($key, $value) {
        // set the value
    }
}

// Desired
class Session {
    public function __construct($request) {
        // Maybe the object has been created only to call get(), no need to rush.
        // So I'll only start the session if there already is one.
        if($this->hasSessionCookie($request)) { $this->startSession(); }
    }
    public function get($key) {
        if( ! $this->isSessionStarted()) { return null; }

        // return value
    }
    public function set($key, $value) {
        if( ! $this->isSessionStarted()) { $this->startSession(); }

        // set the value
    }
}

Same usage, better perfomance, no unexpected behavior, and no legal issues.

@morgen2009

@disposable-ksa98, your sketch is wrong (look into source code at Symfony/Component/HttpFoundation/Session). IMHO, session.get must be pure function (same output for the same input). Different results depending on internal switches on/off is bad idea. The solution would be either the third parameter for get method (auto_start or not) or twig extension with filter function ({{ app.session | myget('flash') }}).

@TerjeBr

Different results depending on if the session is started or not is a good idea, since it was the whole point of this bug report.
And it makes good sense to not start the session if you do a {{ app.session.flashbag.get('notice')}} and there is no session cookie from before.
I agree with the original poster. Please reconsider.

@dlsniper

Just make a PR and see if Fabien will merge it, the best way to solve it.

The long way to solve it:

  • different return types based on session states checked internally in the framework is a very BAD idea as it won't provide consistency in output and it will surprise the user. And there's a thing that says: don't surprise your users. I deal with a lot of bad code lately, and I mean bad code, and one of the major issues of it is that you call a function and based on some random arcane stuff it does some things or other things, would you really want that in a framework???

  • how would you store null then?

  • the code in the demo framework isn't flawed, it's just a demo to show you how you can do some things, a demo should be regarded as a demo and if you consider it a bug, by all means, open a PR/issue;

  • read the manual, like I've said/pointed out, it's written there for a reason. Understand the manual. If you find something that is not properly documented help the guys who wrote the manual by letting them know about the problem or having a PR opened with the missing information;

  • how can you say that by having the check there is a good thing? If I need to start the session in the code the I'll need to start it at some point and since we are using a modern framework which has a DI container and events and some other bells and whistles, how do you expect for a bundle author to write a bundle that uses the session to provide some functionality if he'll do the very same thing as you? And how do you plan to control that, starting point of the session in the code that is not yours? Or even if it's yours, a code that's using events for example, or the DI? I think you just view all the implications this change would have: imagine that if everyone would be doing session start calls and then the framework component that handles the session does more checks internally. You'll surely not have a performance increase but as at the same time you just successfully added a new layer of complexity and useless code by just having everything in 'can I do? this mode';

  • TWIG has some flow control structures, you should use them as I'm really not sure how you handle the case where no flash is in your session;

  • what isn't natural about having the session stared for you when YOU requested something from it explicitly in YOUR code? In non-oop, out of framework code, how would you write that code???? I don't get it, really, how would your code look like in your code? Like this?

echo $_SESSON['myVar'];

It would give a hell of a big PHP Notice and if you tell me that your sever doesn't run with error_reporting on or with notices turned off then it means that you are just promoting bad code and that's for sure what Symfony tries to force you not to do, maybe you've seen the 'annoying' notice error screen from the framework.

If your code would on the other hand look like:

// Detect if the session was started
$isSessionStarted = // MAGIC??

if ($isSessionStarted) {
    echo $_SESSION['myVar'];
}

Then I'd ask you, why did you just did that? Put a check if the session is started or not? Was it because you knew it would give you a warning if you tried to access it directly? If so, what prevents you from having the same thing in the current situation we are talking about? And if you do this, how would other bundle authors be able to provide you with features. Could it be... the same way you do it? Or just restrict non-user bundles accessing the session? (besides sounding like an Apple programmer would rather trap the users soul forever that to give him choices) Do you really really really want that option?!?!

  • as a last point. If you think that empty sessions should be closed/destroyed from the server then there's the following issue, besides the performance thing which might not be that big but... it would mean that before writing off the session data you should check if the session is empty or not and if not then what? Either remove the existing session cookie from the cookies that are about to be sent, BAAAD idea because you might just want to have session cleaned but NOT removed and there wouldn't be any way to prevent it unless you add more logic to the framework to handle this, or leave the cookie alone but then you'll end up with the cookie which you didn't wanted.

If you still can't see how many problems would have to be fixed by this change, also consider this. All, and I mean all, programmers want faster, lighter, smarter framework. Having one that meets all those three requirements is very hard. If you didn't noticed how many problems that the framework would have then to fix for you and not to mention the increase in unnecessary code you just added then there's your issue.

And RTFM :)

Finally, since my job is exhausting as it is, I won't bother you with more messages on this topic as I think I've did tried to explain the best way possible how a documented feature works and why and I might not be the right guy for this. If you have a different opinion on this topic, fine by me, respect mine since I'm trying to respect yours, even if I'm trying to help you understand why you are wrong, at least from my point of view. just /mention someone from GH like Drak or Fabien or Stof and maybe they will be able to help you out.

Have a great day.

@disposable-ksa98

@dlsniper I wasn't talking about demo code, I was talking about core and/or third party code. I don't know what I have enabled in my app that is generating the cookie (I will track it down when I have time), but I'm only using core code and well-known third party bundles (so it's definitely not my code).

RTFM is not the answer, the current behavior is bad and you should feel bad*. We use names for functions as a hint of what they do. If I only call get(), and it sets a cookie, it's wrong.

If you** decide to stick to your position, at least fix the core code and do what you said we are supposed to do, because apparently you or well known third party developers didn't RTFM. But I don't think this will solve anything, someone will forget to do the check and the cookie will be back.

just kidding
*
I'm talking to everyone who develops this, not just @dlsniper

@dlsniper

Download the Symfony Standard distribution, run check the layout.twig.html file for the flashes thing to be enveloped by the above check and you'll see that the core doesn't produce any cookie by its own.

Can you paste a list of third party bundles you are using from composer.json? I'll add them in a new/fresh copy of Symfony 2.1.3 and check it against it. And if it's a third party bundle, just open a issue to the bundle author when you find it.

@disposable-ksa98

@dlsniper As I said two times, I did the test and it worked fine, that's not the point. I'm saying core things that are not enabled by default are doing this. I'll be very busy until friday, please don't close this, I'll be back with a real example. But again, even if it's third party, I don't think it's their fault: get shouldn't set.

@disposable-ksa98
  • Install 2.1.3
  • Create a new bundle and an empty page for '/'
  • Add a new firewall to security.yml:
security:
    firewalls:
        main:
            pattern: ^/
            form_login: ~
            anonymous: ~
  • Optionally disable csrf, doesn't matter
  • '/' will create a session cookie

The reason is probably this:

Anonymous users are technically authenticated, meaning that the isAuthenticated()
method of an anonymous user object will return true.

The issue just became bigger.

To sum up: With Symfony2, you can't avoid sending cookies to anonymous users, if your pages also allow users that are logged in. You can also be sending cookies even if you don't have any firewalls, if you make the mistake of calling get() without first checking isSessionStarted(), because get will set the session (unexpected and undesired behavior).

@TerjeBr

I guess the main problem is; that if there is a session, you have to start it to get the data in the session. That is why get must start a session.

@TerjeBr

I think I have found the source of the big misunderstanding and disagreement in this issue. It is that we can mean different things when we say "the session has started" or "start the session". It is a big difference if we mean "the session was started in this request" or "the session was started in a previous request". By being more explicit about this we can avoid some misunderstanding.

If the session was not started in a previous request it makes sense not to start the session on a get. Even if you start the session, you will still get null as the value. So the get function will not return different values depending on if the request was started in this request or not.

What this issue is about then is that the session->get function should only start the session in the current request if it had already been started in a previous request.

@webda2l

"If you use a form login, Symfony2 will create a cookie even if you set stateless to true."
http://symfony.com/doc/current/book/security.html#stateless-authentication

Remains to understand why.

@dlsniper

Maybe @schmittjoh could help us out on this one?

@disposable-ksa98

I think I have found the source of the big misunderstanding and disagreement in this issue. It is that we can mean different things when we say "the session has started" or "start the session". It is a big difference if we mean "the session was started in this request" or "the session was started in a previous request". By being more explicit about this we can avoid some misunderstanding.

If the session was not started in a previous request it makes sense not to start the session on a get. Even if you start the session, you will still get null as the value. So the get function will not return different values depending on if the request was started in this request or not.

What this issue is about then is that the session->get function should only start the session in the current request if it had already been started in a previous request.

@TerjeBr That's right.

@TerjeBr

Please see if not PR #6152 does what you want.

@ghost

Second, I've now finally been able to see that the session driver itself is broken, it tries to store the serialized object like this: _sf2_attributes|a:0:{}_sf2_flashes|a:0:{}_sf2_meta|a:3:{s:1:"u";i:1353365612;s:1:"c";i:1353365612;s:1:"l";s:1:"0";}.

@dlsniper - what is broken here in your opinion? That looks exactly as it should be to me unless I am missing something obvious.

@dlsniper

@drak I should have removed that comment. That part of the sessions is ok, the problem was that I was too tired when I've did the debugging. So mind that comment please.

Also as the other guys found out, the session component is ok, the security one was doing all the cookie session issues.

@ghost
@dlsniper

@drak I'm not sure that we should have this closed yet, as @schmittjoh hasn't yet replied with the reasons behind having the Security Component doing the cookie all the time.

@TerjeBr

@drak could you please state why you disagree? I am not sure if I understand your stand or your reasons for not liking it.

@TerjeBr

I was thinking about extending my PR so the code also stop flash requests from starting the session if there was no previous session. After all that was the use example @disposable-ksa98 gave at the start of this issue.
(Currently my PR only handles ordinary $session->get() and $session->has())

Currently the session is automatically started whenever you get a session bag. So my code relies on not getting any session bag from the sesson store if the session was not started in a previous request.
The problem is that all the methods on the session class to get data from the flashBag have been deprecated, and new code must first get the flashBag from the session object, and then directly manipulate the flash bag. (Like in {% app.session.flashbag.get('success') %})

So, assuming we have the situation that no session has been started in a previous request, how do we trigger a session start only if the session bag is written to?

As far as I can see there are 4 possible solutions to this:

  1. Delay the start of the session until a write happens in the bag, by rewriting all session bags.
  2. Delay the start of the session until a write happens by creating a wrapper session bag that handles the delay.
  3. Leave the bags as they are, and delay the start of the session until just before the response is about to be sendt. Then a new response event listner has to be created that checks if anything has been written to any session bag, and start the session if that is the case.
  4. Keep the delayed session start logic in the session object and undeprecate the deprecated methods of the session object, and encourage devolpers to use them instead of getting the flashBag if they do not want the session to autostart.

What should we go for?

@TerjeBr

Ok, I got no response to my previous comment, so I just went ahead with option 2. May be this will be a good solution to this issue.

@ghost

I will reply soon.

@TerjeBr

@drak, I am still waiting.

@TerjeBr

@fabpot Should this issue be fixed at all? @drak seems to be against it.

@TerjeBr

@drak, still waiting for your input.

@ghost

In 2.0 the session workflow required explicit start. The framework listener started the session if autostart was set in the config see reference.

As far as I am concerned this is entirely an application development decision. If you have areas of your application which are just for anonymous users and you don't need sessions, then you should add a simple isStarted check in your template or some kind of is logged in block. Hacking detection into the session code based on whether the session has had data written too is bogus because it messes with the ability to know how long the session has been active and last accessed.

Returning null or whatever is also not so great because it messes with APIs which you'd expect to return array() for empty.

When I wrote the session code I was quite aware of so called anonymous sessions but as it is an application level decision, I intentionally left it out of the component as it make things very convoluted for something better done in application level.

Knowing the session is autostarted when accessed, one can code accordingly. In most areas you need a session, but where you don't, simply add a check.

FYI: Originally in the 2.1 development stage, sessions had to be explicitly started, and threw an exception if you tried to access it when not started. The reason for that was so that if you found yourself using a session in a place you didn't expect, you'd know about it, and could code against it - but then it meant you'd need logic to decide when to start a session or not, I'm not averse to that, but I am averse to the PR presented because it's the wrong approach and it makes assumptions which are false as I express above.

I would have preferred to have the session explicitly start and throw exceptions. You could return null, it's not ideal. But returning the bags anyway when not started would be similar to PHP's existing session behaviour: $_SESSION which is null before session_start(). PHP's session way is to address this superglobal. If you write to it before a session is started, it will be overwritten when the session is started.

BTW - I had a very annoying exception in the mock session code and was pressured a lot to remove it - I didnt, and it lead to a bugs being fixed which would not have been noticed or easy to find otherwise. The exception was there to catch things that shouldnt have happened in the first place.

So my POV is - you must write your application taking into consideration how the session code behaves. I'll can submit a PR which allows the session to return bags in a "PHP emulation mode" - this means you can have it not autostart, but still return the bags without starting a session (so your code would return array() without starting the session, but in that mode you'd need to manually start the session).

BUT I still believe the right was is 100% to have a check like this:

{% if app.session.isstarted() %}
    {% for flashMessage in app.session.flashbag.get('notice') %}
        <div class="flash-notice">
            {{ flashMessage }}
        </div>
    {% endfor %}
{% endif %}
@TerjeBr

In 2.0 the session workflow required explicit start. The framework listener started the session if autostart was set in the config see reference.

Yes, I see how that worked. I guess some of the problem is that it is not done like that any more.

As far as I am concerned this is entirely an application development decision. If you have areas of your application which are just for anonymous users and you don't need sessions, then you should add a simple isStarted check in your template or some kind of is logged in block.

Here I think you have misunderstood the problem a bit. Adding a simple isStarted check in your template may not work. Let us say we have these scenarios in a web application:

Scenario 1

  1. Start the session in a request and store something in the session.
  2. Request a simple page that do not use the session. (The session should not be started because it is not used)
  3. Request another page that reads something from the session. (The session must be started, so it can read the data)

Scenario 2

  1. The session has never been started, and nothing is stored in the session
  2. Request a simple page that do not use the session. (The session should not be started because it is not used)
  3. Request another page that reads something from the session. (The session should not be started, but just return null)

Hacking detection into the session code based on whether the session has had data written too is bogus because it messes with the ability to know how long the session has been active and last accessed.

I am sorry, but I fail to understand what you mean here, or why it should be relevant. May be you need to elaborate a bit on this point.

Returning null or whatever is also not so great because it messes with APIs which you'd expect to return array() for empty.

The code should return the same as if nothing has been written to the session. I fail to see which API's you are refering to here and how it messes with them.

When I wrote the session code I was quite aware of so called anonymous sessions but as it is an application level decision, I intentionally left it out of the component as it make things very convoluted for something better done in application level.

I do not know what an anonymous session might be (may be you mean anonymous login), but here we talk about more than that, some applications may not have users or logins at all. Such web-applications wants to avoid starting a session if they can.

Knowing the session is autostarted when accessed, one can code accordingly. In most areas you need a session, but where you don't, simply add a check.

Ok, so here you simply push the problem out to the application programmer. You simply say that we will not fix this issue, the applications programmers has to deal with it by adding checks in their code.

FYI: Originally in the 2.1 development stage, sessions had to be explicitly started, and threw an exception if you tried to access it when not started. The reason for that was so that if you found yourself using a session in a place you didn't expect, you'd know about it, and could code against it - but then it meant you'd need logic to decide when to start a session or not,

Yes, the original poster of this issue mentioned that this could be a solution. Then you at least get a warning as a programmer that you need to put in some checks. But many thinks this is an inferior solution.

I'm not averse to that, but I am averse to the PR presented because it's the wrong approach and it makes assumptions which are false as I express above.

I am not convinced that it is the wrong approach, and I also fail to see which assumptions you are referring to.

I would have preferred to have the session explicitly start and throw exceptions. You could return null, it's not ideal. But returning the bags anyway when not started would be similar to PHP's existing session behaviour: $_SESSION which is null before session_start(). PHP's session way is to address this superglobal. If you write to it before a session is started, it will be overwritten when the session is started.

Yes, but this will not solve the issue stated here. It just creates another problem.

BTW - I had a very annoying exception in the mock session code and was pressured a lot to remove it - I didnt, and it lead to a bugs being fixed which would not have been noticed or easy to find otherwise. The exception was there to catch things that shouldnt have happened in the first place.

Yes, I do not dispute that throwing exceptions when situations that is not supposed to happen happens can be a good thing.

So my POV is - you must write your application taking into consideration how the session code behaves. I'll can submit a PR which allows the session to return bags in a "PHP emulation mode" - this means you can have it not autostart, but still return the bags without starting a session (so your code would return array() without starting the session, but in that mode you'd need to manually start the session).

As I said, this will not be very helpful for the problem at hand.

BUT I still believe the right was is 100% to have a check like this:

{% if app.session.isstarted() %}
    {% for flashMessage in app.session.flashbag.get('notice') %}
        <div class="flash-notice">
            {{ flashMessage }}
        </div>
    {% endfor %}
{% endif %}

And here again you show that you have not understood the problem properly. This code seems to correctly not start the session in step 3 of scenario 2, but fails to start the session in step 3 of scenario 1.

@TerjeBr

@fabpot, @drak ping.

We should have a resolution on this issue. It has been 2 months now.

@fabpot
Owner

@drak submitted a PR. Does everyone agree with it?

@TerjeBr

I have now looked at @drak's PR. I guess it makes the situation a bit better for those that do not want the session started unless it is necessary. But it does not completely solve this issue.

The issue here is that some programmers want to be able to check for flashes or other things in the session (do a read from a session bag) whithout the session being started if no session has been started before. This can be done now by first setting the autostart option to "OFF_LAX".

But what I tried to do in my PR was to just autostart the session only when it was written to or had been started before in a previous request. That would be a much better experience for the application programmer, no need to think about session start at all. With this solution from @drak the application programmer has to first find out that there is such a autostart setting to the session and how to set it in his symfony bundle. And it is not very obvious from the documentation that this is the setting that will solve this problem.

@TerjeBr

Another problem with the mode "OFF_LAX", is that you have to track down every session write in your code and any third party code or bundles that you use, and make sure the session is manually started before the session write. Otherwise the session writes will just be silently ignored, as no session is auto-started.

This will be a major reason NOT to run your code with the "OFF_LAX" mode, and you are back to the same problem we started with.

@fabpot fabpot referenced this issue from a commit
@fabpot fabpot merged branch drak/start_on_demand (PR #7576)
This PR was merged into the master branch.

Discussion
----------

[2.3][Session] Give greater control over how and when session starts

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | na
| License       | MIT
| Doc PR        | symfony/symfony-docs#2475

Refs #6036

Gives control over how start on demand works: allowing to turn it on or off and to allow bag access when session is off.

Commits
-------

f431cb0 Fix tests
1f521d8 Coding standards
2583c26 [HttpFoundation][FrameworkBundle] Keep save auto_start behaviour as in 2.2 and make component values consistent with FrameworkBundle's configuration options.
ceaf69b [FrameworkBundle] Use more sophisticated validation and configuration.
af0a140 [FrameworkBundle] Add configuration to allow control over session start on demand.
8fc2397 [HttpFoundation] Give control over how session start on demand.
7aa0681
@TerjeBr

What is the way forward now on this issue? @fabpot do you want me to work more on my PR #6388?
Or does anyone else have something to say about how this should be done?

I guess one advantage of PR #7576 is that if you use mode "OFF" you will get exceptions instead of auto-started sessions. It can be useful for debugging to find out what code is starting the session when you really want to avoid starting unnecessary sessions.

@TerjeBr

@drak please answer my above comment #6036 (comment)

May be we can resolve this with some constructive discussion and giving each other new ideas?

@ewgRa

Read all related issues and PRs for this issue.

What is the state of this issue? Please use "isStarted"?

I agree with @disposable-ksa98. Session::get must not start session.

This is impossible to test all functionality that it not cause session start on pages, that "read-only" for session and forget about checking "isStarted" is easy.

Maybe create something like "Session::isDirty" and "Session::canBeStoredEmpty" and check it on write step?
isDirty will be changed to true at write methods (set, remove, start and so on). canBeStoredEmpty default return true.

In case of (isDirty || canBeStoredEmpty) - write session. In this case we haven't BC problems.
SessionHandlers that don't know anything about this methods - will be ignore it and work as before - no BC

Handlers that know about this methods will be check it and if isDirty and canBeStoredEmpty is false - just don't write session.

For avoid start session at "get" method - just set canBeStoredEmpty to false. Or even I think canBeStoredEmpty can be called something like "startOnRealWriteOnly" - or something like this

I think it can work. What do you think?

@ewgRa

Or even better idea.

Session::isDirty and SessionHandler::storeOnlyDirtySessions - default is false

Changes at Session haven't BC. Session handlers will be check storeOnlyDirtySessions and Session::isDirty

@ewgRa

Try think about my proposal. Seems main problem how to get access to Session object from SessionStorage::save to check that Session::isDirty.

Any ideas?

My idea is create new interface, check at session that storage implement this interface and mark storage as dirty, because session already save "started, closed" info at storage. In this case there is no BC at all. And on major release merge this interface to SessionStorageInterface and drop checks. Also at major release storeOnlyDirtySessions set default to true.

ping @drak @disposable-ksa98 @dlsniper @TerjeBr

@ewgRa ewgRa referenced this issue from a commit in ewgRa/symfony
@ewgRa ewgRa lazy session start for #6036 2ee567a
@ewgRa ewgRa referenced this issue from a commit in ewgRa/symfony
@ewgRa ewgRa lazy session start for #6036 6a4047a
@ewgRa ewgRa referenced this issue from a commit in ewgRa/symfony
@ewgRa ewgRa lazy session start for #6036 e3ccc79
@ewgRa ewgRa referenced this issue from a commit in ewgRa/symfony
@ewgRa ewgRa lazy session start for #6036 b959884
@ewgRa ewgRa referenced this issue from a commit in ewgRa/symfony
@ewgRa ewgRa lazy session start for #6036 22ba8e0
@ewgRa ewgRa referenced this issue from a commit in ewgRa/symfony
@ewgRa ewgRa lazy session start for #6036 a88bdc6
@ewgRa ewgRa referenced this issue from a commit in ewgRa/symfony
@ewgRa ewgRa lazy session start for #6036 dead1c8
@ewgRa ewgRa referenced this issue from a commit in ewgRa/symfony
@ewgRa ewgRa lazy session start for #6036 7aae116
@ewgRa ewgRa referenced this issue from a commit in ewgRa/symfony
@ewgRa ewgRa lazy session start for #6036 b8360d5
@Tobion
Collaborator

I think how sessions should behave is clear to me:

  • Reading from the session when there is no session cookie, i.e. there is no previous session: It SHOULD NOT start the session. There is no reason to create a session with a cookie when only reading data. It just means the data is not available as we already know since there is no session. This is also what @TerjeBr suggested. @drak argument that people need to check themselves whether the session is active is also possible. But the example he gave {% if app.session.isstarted() %}{% for flashMessage in app.session.flashbag.get('notice') %} is wrong. If the session is closed/saved this won't even work and not show the messages. So actually one would need to check Request::hasPreviousSession each time one reads session data at the moment. But the session part does not even have a method for that. So when injecting the session alone (without request) you don't have access to this important method. So in reality, forcing users to check the session state everywhere where they read data is not viable.

  • Reading from the session when there is IS a session cookie . It should auto-start the session which reads the information stored in the session and makes them available. Otherwise reading data would not find the data unless it has been started manually which is also not developer friendly.

  • Reading from the session when the session has already been started in the current request but is currently closed/saved: In this case we don't necessarily need to re-start the session because the data is already available. This is a performance enhancement because it prevents to read the data again that we already have. The only disavantage is that it does not synchronize the data written in another request between your save() and the get(). But this is highly unlikely and rather strange case anway. And devs could just call start() themselves if they want that sync.

  • Writing to the session should always start the session so that the data is actually persisted. Otherwise there would be no point in writing to the session when it's not available in the next request. Zend Session raises an immutable exception when writing to a session that is already closed/saved. But I don't think that's a good idea because then you cannot add session data later in the lifecycle anymore.

@mpdude

+1 for what @Tobion said, actually that is what I thought things were like.

@TerjeBr

I am still waiting for any feedback on if I should continue working on #6388

@ureimers

Thanks @Tobion for the summary. This issue got a little bit out of hand. As @lsmith77 commented on #6388 this is an important feature on high traffic sites:

So by having a restricted form-login area and a shared-directory between our load-balancers for the sessions, the first thing that happened to our site was that the session directory was flooded with sessions after some hours (even if the users didn't access the restricted area that frequently) and on top of that we had to realize that our varnish didn't cache anything because of those sessions being set in the cookie on every response.

This could have been avoided if we configured varnish correctly in the first place but we didn't expect Symfony to store sessions/set cookies in every response.

So by referring to one of Symfonys mantras: "Use sane defaults (including default behavior)", I'm absolutely in favor of Tobion's suggestion/view on how Sessions should work in Symfony.

@hackzilla hackzilla referenced this issue from a commit
@fabpot fabpot merged branch drak/start_on_demand (PR #7576)
This PR was merged into the master branch.

Discussion
----------

[2.3][Session] Give greater control over how and when session starts

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | na
| License       | MIT
| Doc PR        | symfony/symfony-docs#2475

Refs #6036

Gives control over how start on demand works: allowing to turn it on or off and to allow bag access when session is off.

Commits
-------

f431cb0 Fix tests
1f521d8 Coding standards
2583c26 [HttpFoundation][FrameworkBundle] Keep save auto_start behaviour as in 2.2 and make component values consistent with FrameworkBundle's configuration options.
ceaf69b [FrameworkBundle] Use more sophisticated validation and configuration.
af0a140 [FrameworkBundle] Add configuration to allow control over session start on demand.
8fc2397 [HttpFoundation] Give control over how session start on demand.
c21c53a
@netmikey

We're starting to think about varnish caching as our site is growing. During our analysis, we stumbled upon the cacheability issue that @ureimers explained. Also, we're storing sessions into mysql and all this implicit session creation generates an awful lot of unnecessary load.

+1 for @Tobion's excellent behavior description and @ureimers quote about sane defaults btw. ;)

Also, github got me a bit confused with the merging and reverting... did the pull request eventually make it into Symfony @fabpot ? If so, what Symfony version would we need to migrate to, to be able to tame session creation?

@TerjeBr

It is not in any symfony version yet.

I am still waiting for any response from @fabpot if we should go ahead with #6388 or not.

@weaverryan
Collaborator

@Tobion I agree fully with your description as well - there's been some confusion between "is the session started" and "is there a session cookie - i.e. was the session started on a previous request". You explain that well. I think this is the right approach, so do you have time and knowledge of this area to look at #6388 to see if it's the right approach? We really either need to say "yes, this is the right approach let's finish this PR" or "no, it's not the right approach, let's work on a different PR".

Thanks :)

@weaverryan
Collaborator

@TerjeBr And thanks for not letting this go - I know it's been a huge, long confusing road :)

@TerjeBr

Thank you @weaverryan for giving your support for this issue. I absolutely agree with what you said.

As I have said before, I am happy to continue work on PR #6388 but I hesitate to do so before any one with some authority in this symfony2 community are willing to give me some positive feedback and say this is the way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.