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

[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.

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') }}).