Set session.cookie_* parameters properly #3423

mvrhov · 2012-02-23T11:05:08Z

Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: /

Cookie parameters in $options are not prefixed with cookie_ the same is true for data returned from session_get_cookie_params.

I've marked this as BC because the options that get dumped into the container have different name. But I don't think anybody was actually changing them or accessing them in their bundles.

P.S. @Drak also desires some credits for this PR as I incorporated some lines written by him in one of the iterations.

ghost · 2012-02-23T14:24:42Z

@mvrhov - what does this fix exactly? It looks like a different way of doing the same thing but now there is no default value on cookie_httponly.

mvrhov · 2012-02-23T15:09:17Z

Like I said in description. $option contains some cookie options and none of them has cookie_ prefix.
And this prefix is needed in two cases:

to properly merge defaults and override them with what user set
in a foreach for for proper ini_set

Sorry non native speaker an a bit hard to explain, could you ping me in a couple of hours on IRC if this still doesn't make any sense.

ghost · 2012-02-23T15:29:41Z

@mvrhov - I wrote some tests for this particular code and I still don't see what this PR fixes. I'll try to catch you on IRC later on but can't guarantee it.

mvrhov · 2012-02-23T16:02:41Z

added test

ghost · 2012-02-24T07:53:24Z

CHANGELOG-2.1.md

@@ -42,6 +42,8 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
 * added support for placeholders in route defaults and requirements (replaced by the value set in the service container)
 * added Filesystem component as a dependency
 * added support for hinclude (use ``standalone: 'js'`` in render tag)
+ * [BC BREAK] following session options: 'lifetime', 'path', 'domain', 'secure', 'httponly'


This is not really a BC break since the configuration is still read using the old values, it's just updated in the dumped container. What might be nicer is to alter the code so that lifetime is rewritten to cookie_lifetime for BC, but that the configfile may also use the updated values as cookie_lifetime. This way there is still no BC break and there is no magic happening in the background which is always going to give someone a headache down the line.

I've been trying to figure out how to do the config for the past hour, but to no avail.
Can somebody who is more intimate with config builder give some hints if this is even possible.
ping @stof, @vicb

->beforeNormalization() would allow to change the keys names (if that's what you are looking for)

Would also need to update the FrameworkBundle\Resources\config\schema\symfony-1.0.xsd I believe.

@vicb: It's a bit complicated.

we should support both keys the prefixed one and unprefixed one.

if both are present in the config prefixed one should take precedence (preferably)? or exception should be thrown

Mark the unprefixed keys as deprecated in the documentation.

I'm guessing you need to add the keys to the .xsd, and then add the nodes
to
https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php#L161

ghost · 2012-02-24T08:30:51Z

Just for reference for those reading this ticket, session_set_cookie_params() alters the runtime ini settings it corresponds to see http://docs.php.net/manual/en/function.session-set-cookie-params.php so we agreed to remove the special handling that was present since it is redundant.

fabpot · 2012-02-27T09:11:57Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

            if (isset($config[$key])) {
                $options[$key] = $config[$key];
            }
        }
+        //drivers requre correct names for cookie options e.g the one with cookie_ prefix


typo: require

dlsniper · 2012-02-28T22:19:32Z

Hi, Is this patch relevant or not after all?
ping @Drak @mvrhov

Thanks :)

ghost · 2012-02-29T03:34:22Z

It is relevant. Maybe I'll do the cleanup this PR by forking it if @mvrhov doesn't have time.

Prefixed following session options: 'lifetime', 'path', 'domain', 'secure', 'httponly' because this results in better session driver code

mvrhov · 2012-02-29T05:40:47Z

Fixed the typo and changed the false to ture as reported in comments. I've also rebased. I'll see what I can do about config file change later today. Sorry for the delay, been too busy for the past week.

mvrhov · 2012-02-29T08:49:23Z

I've also done the config part.

mvrhov · 2012-02-29T11:01:14Z

Ok, this should be it.

ghost · 2012-02-29T13:14:21Z

src/Symfony/Component/HttpFoundation/Session/Storage/AbstractSessionStorage.php

+        // set defaults for certain values
+        $defaults = array(
+            'cache_limiter' => '', // disable by default because it's managed by HeaderBag (if used)
+            'auto_start' => true,


This is incorrect, auto_start should be false.

ghost · 2012-03-01T00:59:16Z

@fabpot - looks good from my side.

@Drak

Commits ------- 471b564 auto_start should be false 6e2a7da Support session cookie options with cookie_ prefix e0fba80 Properly merge session cookie_* parameters Discussion ---------- Set session.cookie_* parameters properly Bug fix: yes Feature addition: no Backwards compatibility break: yes Symfony2 tests pass: yes Fixes the following tickets: / Cookie parameters in $options are not prefixed with cookie_ the same is true for data returned from session_get_cookie_params. I've marked this as BC because the options that get dumped into the container have different name. But I don't think anybody was actually changing them or accessing them in their bundles. P.S. @Drak also desires some credits for this PR as I incorporated some lines written by him in one of the iterations. --------------------------------------------------------------------------- by drak at 2012-02-23T14:24:42Z @mvrhov - what does this fix exactly? It looks like a different way of doing the same thing but now there is no default value on `cookie_httponly`. --------------------------------------------------------------------------- by mvrhov at 2012-02-23T15:09:17Z Like I said in description. $option contains some cookie options and none of them has cookie_ prefix. And this prefix is needed in two cases: - to properly merge defaults and override them with what user set - in a foreach for for proper ini_set Sorry non native speaker an a bit hard to explain, could you ping me in a couple of hours on IRC if this still doesn't make any sense. --------------------------------------------------------------------------- by drak at 2012-02-23T15:29:41Z @mvrhov - I wrote some tests for this particular code and I still don't see what this PR fixes. I'll try to catch you on IRC later on but can't guarantee it. --------------------------------------------------------------------------- by mvrhov at 2012-02-23T16:02:41Z added test --------------------------------------------------------------------------- by drak at 2012-02-24T08:30:51Z Just for reference for those reading this ticket, `session_set_cookie_params()` alters the runtime ini settings it corresponds to see http://docs.php.net/manual/en/function.session-set-cookie-params.php so we agreed to remove the special handling that was present since it is redundant. --------------------------------------------------------------------------- by dlsniper at 2012-02-28T22:19:32Z Hi, Is this patch relevant or not after all? ping @Drak @mvrhov Thanks :) --------------------------------------------------------------------------- by drak at 2012-02-29T03:34:22Z It is relevant. Maybe I'll do the cleanup this PR by forking it if @mvrhov doesn't have time. --------------------------------------------------------------------------- by mvrhov at 2012-02-29T05:40:47Z Fixed the typo and changed the false to ture as reported in comments. I've also rebased. I'll see what I can do about config file change later today. Sorry for the delay, been too busy for the past week. --------------------------------------------------------------------------- by mvrhov at 2012-02-29T08:49:23Z I've also done the config part. --------------------------------------------------------------------------- by mvrhov at 2012-02-29T11:01:14Z Ok, this should be it. --------------------------------------------------------------------------- by drak at 2012-03-01T00:59:16Z @fabpot - looks good from my side.

ghost reviewed Feb 24, 2012
View reviewed changes

fabpot reviewed Feb 27, 2012
View reviewed changes

Properly merge session cookie_* parameters

e0fba80

Prefixed following session options: 'lifetime', 'path', 'domain', 'secure', 'httponly' because this results in better session driver code

Support session cookie options with cookie_ prefix

6e2a7da

mvrhov mentioned this pull request Feb 29, 2012

Session Cookie is not set with framework configs #3471

Closed

ghost reviewed Feb 29, 2012
View reviewed changes

auto_start should be false

471b564

fabpot merged commit 471b564 into symfony:master Mar 1, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set session.cookie_* parameters properly #3423

Set session.cookie_* parameters properly #3423

mvrhov commented Feb 23, 2012

ghost commented Feb 23, 2012

mvrhov commented Feb 23, 2012

ghost commented Feb 23, 2012

mvrhov commented Feb 23, 2012

ghost Feb 24, 2012

mvrhov Feb 29, 2012

vicb Feb 29, 2012

ghost Feb 29, 2012

mvrhov Feb 29, 2012

ghost Feb 29, 2012

ghost commented Feb 24, 2012

fabpot Feb 27, 2012

dlsniper commented Feb 28, 2012

ghost commented Feb 29, 2012

mvrhov commented Feb 29, 2012

mvrhov commented Feb 29, 2012

mvrhov commented Feb 29, 2012

ghost Feb 29, 2012

mvrhov Feb 29, 2012

ghost commented Mar 1, 2012

Set session.cookie_* parameters properly #3423

Set session.cookie_* parameters properly #3423

Conversation

mvrhov commented Feb 23, 2012

ghost commented Feb 23, 2012

mvrhov commented Feb 23, 2012

ghost commented Feb 23, 2012

mvrhov commented Feb 23, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 24, 2012

Choose a reason for hiding this comment

dlsniper commented Feb 28, 2012

ghost commented Feb 29, 2012

mvrhov commented Feb 29, 2012

mvrhov commented Feb 29, 2012

mvrhov commented Feb 29, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 1, 2012