This repository has been archived on 2023-08-20. You can view files and clone it, but cannot push or open issues or pull requests.
symfony/tests/Symfony
Fabien Potencier 42923f3044 merged branch mvrhov/session_cookie_merge (PR #3423)
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.
2012-03-01 11:39:59 +01:00
..
Tests merged branch mvrhov/session_cookie_merge (PR #3423) 2012-03-01 11:39:59 +01:00