This PR was squashed before being merged into the master branch (closes#5888).
Commits
-------
2379d86 CS Fixes - Replaced "array of type" by "Type[]" in PHPDoc block
Discussion
----------
CS Fixes - Replaced "array of type" by "Type[]" in PHPDoc block
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: no (but tests doesn't pass on master too). See Travis.
License of the code: MIT
Documentation PR: Not Applicable
Status: Finished
To improve support of the eclipse PDT pluggin (for autocompletion), I propose to change the array notation in PHPDoc blocks to match the phpDocumentor notation for "array of type".
Modifications are made for the following components:
- BrowserKit
- ClassLoader
- Config
- Console
- CssSelector
- DependencyInjection
- DomCrawler
- EventDispatcher (no changes)
- Filesystem (no changes)
- Finder
- Form
- HttpFoundation
- HttpKernel
- Locale
- OptionResolver (no changes)
- Process (no changes)
- Routing (no changes)
- Serializer (no changes)
- Templating
- Translation
- Validator
- Yaml (no changes)
- Security
- Stopwatch (no changes)
See Proposal https://github.com/symfony/symfony/pull/5852
---------------------------------------------------------------------------
by pborreli at 2012-11-01T15:19:27Z
will you make a PR for each component ? why not only one PR with one commit for each component instead ?
---------------------------------------------------------------------------
by raziel057 at 2012-11-01T15:32:39Z
Ok, I'm going try to do it.
---------------------------------------------------------------------------
by raziel057 at 2012-11-01T16:12:56Z
I would like to rename my branch from COMPONENT_Form to changes-phpdoc (as all modifications would be commited in only one branch), so I tried to execute the following command but I have an error.
git remote rename COMPONENT_Form changes-phpdoc
error: Could not rename config section 'remote.COMPONENT_Form' to 'remote.changes-phpdoc'
Do you know how to do it?
---------------------------------------------------------------------------
by pborreli at 2012-11-01T16:14:26Z
don't rename it, you will have to close and make another PR which is useless here, just edit the title.
---------------------------------------------------------------------------
by stof at 2012-11-01T16:16:17Z
and ``git remote rename`` is about renaming a remote repo, not a branch
---------------------------------------------------------------------------
by raziel057 at 2012-11-03T11:36:02Z
Is it normal that all my commit are duplicated? I would like just update my master and merge with my branch.
---------------------------------------------------------------------------
by fabpot at 2012-11-06T10:22:55Z
@raziel057 Can you rebase on master? That should fix your problem.
---------------------------------------------------------------------------
by fabpot at 2012-11-09T13:28:53Z
@raziel057 Can you finish this PR?
---------------------------------------------------------------------------
by Tobion at 2012-11-09T13:34:45Z
I'll do it for the routing component this evening because I know it by heart. ^^
---------------------------------------------------------------------------
by raziel057 at 2012-11-09T15:06:26Z
@Tobion ok Thanks!
@fabpot Yes, I will try to finish it this week end.
---------------------------------------------------------------------------
by raziel057 at 2012-11-11T13:04:07Z
@Tobion Did you already change PHPDoc in the Routing component?
---------------------------------------------------------------------------
by Tobion at 2012-11-11T15:21:18Z
@raziel057 Yes I'm working on it.
---------------------------------------------------------------------------
by Tobion at 2012-11-12T15:16:31Z
@raziel057 Done. See #5994
* 2.1: (24 commits)
forced Travis to use source to workaround their not-up-to-date Composer on PHP 5.3.3
[Routing] removed irrelevant string cast in Route
Fixed typo
Make YamlFileLoader and XmlFileLoader file loading extensible
[HttpKernel] fix typo
Fixed singularization of "prices"
[Form] Removed an exception that prevented valid formats from being passed, e.g. "h" for the hour, "L" for the month etc.
[HttpKernel] fixed Client when using StreamedResponses (closes#5370)
fixed PDO session handler for Oracle (closes#5829)
[HttpFoundation] fixed PDO session handler for Oracle (closes#5829)
[Locale] removed a check that is done too early (and it is done twice anyways)
Update src/Symfony/Component/Validator/Resources/translations/validators.fa.xlf
Adding new localized strings for farsi validation.
[HttpFoundation] moved the HTTP protocol check from StreamedResponse to Response (closes#5937)
[Form] Fixed forms not to be marked invalid if their children are already marked invalid
[Form] Excluded some tests in NumberToLocalizedStringTransformerTest which fail on ICU 4.4, but work on ICU 4.8
added missing tests from previous merge
[Form] Fixed NumberToLocalizedStringTransformer to accept both comma and dot as decimal separator, if possible
Fix export-ignore on Windows
Show correct class name InputArgument in error message
...
Conflicts:
.travis.yml
src/Symfony/Component/Form/Extension/Core/DataTransformer/NumberToLocalizedStringTransformer.php
* Default to _id for storing session ID
* Use MongoDate instead of MongoTimestamp (BC break)
* Rename default field names ("sess_" is redundant)
* "justOne" is redundant for session removal
* Assert true return values in method tests
* Add note about TTL collections for gc()
* Don't set identifier in upsert (invalid behavior)
From mongo driver version 1.2.11 E_DEPRECATED is emitted when the 2nd argument (type) is not used for MongoBinData constructor. At the moment the default value is 0x02 ("byte array").
See: http://php.net/manual/en/mongobindata.construct.php
Commits
-------
22be499 [HttpFoundation][Session] Cleaned up constructor invokation
Discussion
----------
[HttpFoundation][Session] Cleaned up constructor invokation
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: ~
Todo: ~
License of the code: MIT
Documentation PR: ~
This simply cleans up a mismatch between the class instantiation and the actual class constructor. Evidently something left over from the various iterations.
---------------------------------------------------------------------------
by travisbot at 2012-08-07T19:16:14Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/2060028) (merged 22be4993 into 7dbadbf2).
---------------------------------------------------------------------------
by drak at 2012-08-09T08:25:43Z
ping @fabpot
Commits
-------
f72ba0a Fixed detection of an active session
Discussion
----------
[WIP][HttpFoundation][Session] Fixed detection of an active session
Bug fix: yes
Feature addition: no
Backwards compatibility break: not sure
Symfony2 tests pass: no
Fixes the following tickets: #4529
Todo: Fix failing tests
License of the code: MIT
Documentation PR: ~
This fixes the problem when the session variable inside $request now has always data in it as it's now more powerful but this introduces the problem that the old way of detecting if a session is started or not doesn't work anymore.
---------------------------------------------------------------------------
by travisbot at 2012-06-09T21:53:17Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1578839) (merged 9ae13e12 into 6266b72d).
---------------------------------------------------------------------------
by drak at 2012-06-10T01:57:59Z
Sessions should be started implicitly. The SF auto_start config parameter controls the session listener to start the session.
---------------------------------------------------------------------------
by dlsniper at 2012-06-11T06:46:02Z
So this patch is correct then and I should continue the work on it?
---------------------------------------------------------------------------
by drak at 2012-06-11T07:51:39Z
@dlsniper - no it's not correct. The session should not be auto-started like this, @fabpot and I recently discussed it.
---------------------------------------------------------------------------
by dlsniper at 2012-06-11T07:52:55Z
@Drak, ok I'll remove the patch for auto_start then but the fix for start would still stand, right?
---------------------------------------------------------------------------
by drak at 2012-06-12T18:40:35Z
@dlsniper - I have no objection to the rest of the PR except for the autostart stuff. I've annotated for clarity :)
---------------------------------------------------------------------------
by travisbot at 2012-06-12T19:51:12Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1604158) (merged 3499980e into 37550d23).
---------------------------------------------------------------------------
by travisbot at 2012-06-12T19:52:00Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1604166) (merged dcc73071 into 37550d23).
---------------------------------------------------------------------------
by dlsniper at 2012-06-12T19:56:51Z
Seems Travis doesn't like the squashing of commits that I've did but the PR does pass the normal tests.
@drak is this good for merging now?
Thanks :)
---------------------------------------------------------------------------
by dlsniper at 2012-06-13T09:05:09Z
@fabpot this can be merged safely, I've just applied the patch on my production application and the patch is ok, it's just travis failing.
Thanks
---------------------------------------------------------------------------
by travisbot at 2012-06-13T09:23:46Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1608735) (merged 1a6eabd2 into 37550d23).
---------------------------------------------------------------------------
by travisbot at 2012-06-13T09:28:26Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1608758) (merged 4e3a93c8 into 37550d23).
---------------------------------------------------------------------------
by dlsniper at 2012-06-13T09:29:28Z
I've noticed that this is failing, I'll fix it later on today.
---------------------------------------------------------------------------
by travisbot at 2012-06-13T15:14:01Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1611541) (merged 5504c4b7 into 37550d23).
---------------------------------------------------------------------------
by drak at 2012-06-13T15:23:47Z
It's possible that other tests are failing not related to this PR. Run the tests on the current master, and try rebasing your branch to the current master also.
---------------------------------------------------------------------------
by dlsniper at 2012-06-13T15:44:22Z
I've just reminded why this is failing on builds, I can't do them locally because of this:
```
Installing dev dependencies
Your requirements could not be solved to an installable set of packages.
Problems:
- Problem caused by:
- Installation request for doctrine/orm [>= 2.2.0.0, < 2.4.0.0-dev]: Satisfiable by [doctrine/orm-2.2.2, doctrine/orm-2.2.1, doctrine/orm-2.2.0, doctrine/orm-2.2.x-dev, doctrine/orm-2.3.x-dev].
```
I'll try and install this somehow and see what's wrong with it.
---------------------------------------------------------------------------
by mvrhov at 2012-06-13T18:08:58Z
@dlsniper: as @stof said to me this should be resolved in latest versions of composer, but it seems that is not. The problem is that composer cannot figure out that you are on dev-master if you try to instal dev. dependencies on feature branch. Take a look at the .travis.yml file on how to do a proper dev vendors install.
cc @Seldaek
---------------------------------------------------------------------------
by dlsniper at 2012-06-13T23:08:53Z
@mvrhov Thanks for pointing this out.
@drak I still got two tests not passing but I'm not sure how to fix them as adding $session->start() will either fail with the message that the session has already been started, the headers_sent() call which returns true. Any help with them will be greatly appreciated. Thanks!
Here is what the HttpKernel tests are returning:
```
There were 2 failures:
1) Symfony\Component\HttpKernel\Tests\EventListener\LocaleListenerTest::testDefaultLocaleWithSession
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'es'
+'fr'
/var/www/symfony-orig/src/Symfony/Component/HttpKernel/Tests/EventListener/LocaleListenerTest.php:51
2) Symfony\Component\HttpKernel\Tests\EventListener\LocaleListenerTest::testLocaleFromRequestAttribute
Expectation failed for method name is equal to <string:set> when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.
FAILURES!
Tests: 263, Assertions: 1025, Failures: 2, Skipped: 10.
```
---------------------------------------------------------------------------
by travisbot at 2012-06-13T23:42:59Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1614883) (merged 1004b7c0 into c07e9163).
---------------------------------------------------------------------------
by travisbot at 2012-06-13T23:53:06Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1614897) (merged f72ba0a2 into c07e9163).
---------------------------------------------------------------------------
by dlsniper at 2012-06-16T20:14:41Z
@stof / @vicb Hi, do either of you think that you can either point me out to the right direction for fixing this either ping someone else for home help as @drak doesn't seem available for this and at the moment I'm pretty much clueless in what direction I should take this fix.
Thanks!
---------------------------------------------------------------------------
by dlsniper at 2012-06-19T14:16:29Z
ping @fabpot Can you please provide some input on this one as I'm a bit stuck and seems noone else is available.
---------------------------------------------------------------------------
by drak at 2012-06-20T10:24:43Z
fyi - I'll be able to look again in a few days
---------------------------------------------------------------------------
by fabpot at 2012-07-01T07:53:28Z
I'm +1 to add the `isStarted()` method, but -1 for the change of `Request::hasSession`.
---------------------------------------------------------------------------
by drak at 2012-07-01T09:06:15Z
@fabpot, I agree. `hasSession()` should not be changed, it's semantically incorrect to make it return effectively "hasActiveSession".
Commits
-------
9a74b85 [HttpFoundation] CS and phpdoc fixes
Discussion
----------
[HttpFoundation] CS and phpdoc fixes
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: -
Hey
---------------------------------------------------------------------------
by travisbot at 2012-06-02T00:30:49Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1504379) (merged 2f0117f2 into 1541fe26).
---------------------------------------------------------------------------
by fabpot at 2012-06-25T14:53:18Z
@adrienbrault Can you have a look at my comments?
---------------------------------------------------------------------------
by adrienbrault at 2012-06-25T16:24:49Z
Done! Sorry for the delay
---------------------------------------------------------------------------
by travisbot at 2012-06-25T17:50:24Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1702850) (merged 9a74b851 into 58436de1).
Commits
-------
b865b09 [Session] Fix the PDO handler for mysql concurrent write
Discussion
----------
[RFC][Session] Make the PDO handler looks less hacky
Related discussion: ebc2f01e5b (commitcomment-1304221)
The current code works but looks hacky (`$dbTimeCol = CASE WHEN $dbTimeCol = :time THEN (VALUES($dbTimeCol) + 1) ELSE VALUES($dbTimeCol) END`).
Todo: wrap the mysql specific code in a `try...catch` if we choose this PR way (to be consistent with all other PDO invocations).
---------------------------------------------------------------------------
by travisbot at 2012-05-10T07:50:39Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1293131) (merged b865b096 into 48099a85).
Commits
-------
6756f28 [Session] Fixed Backward Compatibility issue with getFlashes()
Discussion
----------
[Session] Fixed Backward Compatibility issue with getFlashes()
---------------------------------------------------------------------------
by fabpot at 2012-04-25T22:35:42Z
ping @drak
---------------------------------------------------------------------------
by willdurand at 2012-04-25T22:37:01Z
By the way, I had this issue on a real application I upgraded from Symfony2 2.0.x to 2.1 (and written by @Seldaek)
The code looks like:
``` php
<?php
// in a controller
$this->session->setFlash('foo', array(
'code' => 'success',
'message' => 'lalala',
'params' => array())
);
```
---------------------------------------------------------------------------
by Seldaek at 2012-04-26T07:25:03Z
Yup, to be fair in retrospective maybe that should have been translated in the controller directly (that's why it had message + params as an array), but this is code that predates 2.0 by at least six months, so it was obviously not clear what best practices were. Anyway it seems it can be fixed without much harm, so for the sake of safety and because I may not be the only crazy person having done this, it'd be good to fix IMO.
This is a very important option which allows the cookie lifetime to be changed on migrate.
For example when a user converts from an anonymous session to a logged in session one might
wish to change from a persistent cookie to browser session (e.g. a banking application).
This commit allows applications to know certain meta-data about the session
Session storage is designed to only store some data against a session ID
so this method is necessary to be compatible with any session handler, including
native handlers.
Commits
-------
5ae76f1 [HttpFoundation] Update documentation.
910b5c7 [HttpFoudation] CS, more tests and some optimization.
b0466e8 [HttpFoundation] Refactored BC Session class methods.
84c2e3c [HttpFoundation] Allow flash messages to have multiple messages per type.
Discussion
----------
[2.1][HttpFoundation] Multiple session flash messages
Bug fix: no
Feature addition: yes
Backwards compatibility break: yes, but this already happened in #2583. BC `Session` methods remain unbroken.
Symfony2 tests pass: yes
Fixes the following tickets: #1863
References the following tickets: #2714, #2753, #2510, #2543, #2853
Todo: -
This PR alters flash messages so that it is possible to store more than one message per flash type using the `add()` method or by passing an array of messages to `set()`.
__NOTES ABOUT BC__
This PR maintains BC behaviour with the `Session` class in that the old Symfony 2.0 methods will continue to work as before.
---------------------------------------------------------------------------
by drak at 2012-02-13T06:28:33Z
I think this is ready for review @fabpot @lsmith77
---------------------------------------------------------------------------
by lsmith77 at 2012-02-14T19:30:39Z
the FlashBag vs. AutoExpireFlashBag behavior and setup difference should probably also be explained in the upgrading log
---------------------------------------------------------------------------
by drak at 2012-02-15T04:43:14Z
@lsmith77 Those differences are explained already in the changelog
* Added `FlashBag`. Flashes expire when retrieved by `get()` or `all()`.
This makes the implementation ESI compatible.
* Added `AutoExpireFlashBag` (default) to replicate Symfony 2.0.x auto expire behaviour of messages auto expiring
after one page page load. Messages must be retrived by `get()` or `all()`.
---------------------------------------------------------------------------
by Crell at 2012-02-19T17:35:34Z
Drak asked me to weigh in here with use cases. Drupal currently has a similar session-stored-messaging system in place that I'd like to be able to replace with Flash messages. We frequently have multiple messages within a single request, however, so this change is critical to our being able to do so.
For instance, when saving an article in Drupal there is, by default, a "yay, you saved an article!" type message that gets displayed. If you also have the site configured to send email when a post is updated, you may see a "email notifications sent" message (depending on your access level). If you have a Solr server setup for search, and you're in debug mode, there will also be a "record ID X added to Solr, it should update in 2 minutes" message. And if there's a bug somewhere, you'll also get, as an error message rather than notice message, a "Oops, E_NOTICE on line 54" message.
Form validation is another case. If you have multiple errors in a single form, we prefer to list all of them. So if you screw up 4 times on a form, you may get 4 different error messages showing what you screwed up so you can fix it in one go instead of several.
Now sure, one could emulate that by building a multi-message layer on top of single-layer messages, but, really, why? "One is a special case of many", and there are many many cases where you'll want to post multiple messages. Like, most of Drupal. :-)
---------------------------------------------------------------------------
by lsmith77 at 2012-03-06T20:55:51Z
@fabpot is there any information you still need before merging this? do you want more discussion in which case you might want to take this to the mailing list ..
---------------------------------------------------------------------------
by drak at 2012-03-08T18:54:13Z
Another plus for this PR is that it requires no extra lines of code in templates etc to display the flashes, see https://github.com/symfony/symfony/pull/3267/files#diff-1
---------------------------------------------------------------------------
by drak at 2012-03-15T06:38:21Z
Rebased against current `master`, should be mergeable again..
---------------------------------------------------------------------------
by evillemez at 2012-03-17T03:08:41Z
+1 to this, I have an extended version of HttpFoundation just for this... would love to get rid of it.
Commits
-------
bd02554 [HttpFoundation] SPL IteratorAggregate+Countable on *Bags
665fdeb [HttpFoundation] SPL on ParameterBag
Discussion
----------
[HttpFoundation] SPL on ParameterBag
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Added a couple SPL interfaces to ParameterBag, added shortcuts to working with the parameters. For example:
```php
<?php
$post = Request::createFromGlobal()->request;
echo "There are {count($post)} POST variables\n";
foreach ($post as $key => $val) {
echo "{$key}: {$val}\n";
}
```
Thoughts?
---------------------------------------------------------------------------
by stealth35 at 2012-03-07T13:09:11Z
You already have the `all` method
``` php
<?php
$post = Request::createFromGlobals()->request->all();
echo "There are ", count($post), " POST variables\n";
foreach ($post as $key => $val) {
echo "{$key}: {$val}\n";
}
```
---------------------------------------------------------------------------
by cboden at 2012-03-07T13:50:22Z
Yes, but when in the context of working with the Request object (or POST ParamegerBag), it's 1 more call and loose variable to set.
ParameterBag is a container, these common SPL interfaces give standard PHP container methods to it.
---------------------------------------------------------------------------
by lsmith77 at 2012-03-07T18:42:41Z
makes sense to me ..
---------------------------------------------------------------------------
by vicb at 2012-03-09T15:45:40Z
Probably makes sense. Could you check if any other `*Bag.php` needs to be updated so that it could ba an atomic merge.
---------------------------------------------------------------------------
by cboden at 2012-03-09T15:48:40Z
Whoops, good catch @vicb. I made a poor assumption all the *Bags extended ParameterBag, while only some do. I will post an update shortly.
Commits
-------
c4ee947 Native Redis Session Storage update
665f593 NativeRedisSessionStorage added
Discussion
----------
[HttpFoundation] Native Redis Session Storage
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
---------------------------------------------------------------------------
by lstrojny at 2012-03-04T23:15:43Z
Does Symfony (or any of its dependencies) has Redis support in any form whatsoever? If not this might be a good point to decide which clients to support
---------------------------------------------------------------------------
by lsmith77 at 2012-03-04T23:36:11Z
well ideally we just get this cache interface stuff done .. for this use case it would be perfect.
---------------------------------------------------------------------------
by pulzarraider at 2012-03-05T00:35:59Z
There is RedisProfilerStorage available (based on phpredis). I prefer and write code for [phpredis](https://github.com/nicolasff/phpredis).
It's recommended by [official Redis homepage](http://redis.io/clients#PHP). [In this benchmark](http://dev.af83.com/2011/01/01/which-php-library-to-use-with-redis-the-benchmark.html
) is fastest and less memory consumpting.
But if somebody prefer predis (with phpiredis), rediska or something other widely used, there are no limitations to add support of it to Symfony.
My opinion is, that the C extension should be supported at first, because of good performance and native session storage support. Redis is quite young and the process of creating PHP clients is comparable to Memcache.
There were created pure PHP Memcache clients in the past (Google found for example [this](http://www.phpclasses.org/browse/file/20284.html) and [this](http://code.blitzaffe.com/pages/phpclasses/files/memcached_client_52-12)), but they are not being used now. Everyone, who is seriously thinking about performance, is using only the C Redis/Memcache(d)/... extensions.
---------------------------------------------------------------------------
by drak at 2012-03-05T07:40:06Z
+1 on this PR. Needs a test written though.
I don't think there is any need to wait for #3493 imo. I'll deal with it if this is merged before #3493.
Are there any PHP ini settings for this for this driver or is everything via the `session.save_path` directive? (A quick look at the C code seems to indicate there are no explicit ini directives).
---------------------------------------------------------------------------
by lstrojny at 2012-03-05T12:14:34Z
@pulzarraider I don’t necessarily disagree with the usage of phpredis, I just wanted to bring up the issues of various clients and people having different preferences about them.
---------------------------------------------------------------------------
by fabpot at 2012-03-05T14:46:22Z
@pulzarraider Can you add some unit tests before I merge?
---------------------------------------------------------------------------
by pulzarraider at 2012-03-11T20:19:57Z
@drak No there are no php.ini settings. Only RedisArray has some, but it's another feature.
@fabpot I've added simple test based on other session storage tests.
I planned to create a RedisSessionStorage, too, but I have no time for it now. This can be added later in another PR as it's independent from NativeRedisSessionStorage.
---------------------------------------------------------------------------
by drak at 2012-03-12T02:21:25Z
The code looks OK to me.
---------------------------------------------------------------------------
by fabpot at 2012-03-15T06:05:27Z
#3493 has been merged now.
---------------------------------------------------------------------------
by pulzarraider at 2012-03-16T23:21:27Z
Code updated.
It does not make sense to try and store session ini directives since they can be changes outside
of the class as they are part of the global state.
Coding stan
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.
Added blocks, updated links and references and fixed typos.
Note it is not possible to throw exceptions in the write or close methods of a session save handler.
Commits
-------
fb2bb65 [HttpFoundation] Fix session.cache_limiter is not set correctly
Discussion
----------
[HttpFoundation] Fix session.cache_limiter is not set correctly
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Fixes a regression after the session refactoring where extra cache control http headers are sent.
This was previously handled by [calling session_cache_limiter(false) in NativeSessionStorage](https://github.com/symfony/symfony/blob/2.0/src/Symfony/Component/HttpFoundation/SessionStorage/NativeSessionStorage.php#L81)
---------------------------------------------------------------------------
by drak at 2012-02-21T12:23:48Z
@fabpot - this code can be merged imo.