Commits
-------
45219ef [Process] Add default xampp path to the list of possible paths to check
28e1313 [Process] Clean-up/simplify code
56f473a [Process] Add extra dirs argument to the executable finder to allow searching more dirs
Discussion
----------
ExecutableFinder changes
It cleans up stuff a bit and adds a guess for xampp users that wouldn't have php in their path
Commits
-------
eda439f [EventDataCollector] Display a better message when no events have been recorded
6b87981 [TimeDataCollector] Do not throw an exception when no events are recorded
Discussion
----------
[Profiler] Better support for collector in a production env
relates to #3706.
With this PR it is possible to:
- enable only the profiler in a production environment - the wdt being disabled you have to switch to the development environment to inspect the collected data,
- enable both the profiler and the wdt in a production environment (the use case form #3706).
@jmikola would this solve your use case ?
Commits
-------
26a1e0b [ClassLoader] ordered ClassCollectionLoader writing to avoid redeclaration at runtime
Discussion
----------
[HttpKernel] allowed classes to compile to be prepended
I had an issue when registering JMSDIExtraBundle before the frameworkBundle, because the bundle is adding a class to compile wich extends a class to compile added by the frameworkbundle (https://github.com/schmittjoh/JMSDiExtraBundle/blob/master/HttpKernel/ControllerResolver.php#L39).
In my kernel, the bundle was registered before the frameworkbundle, if it's the case, the class is writed before the symfony core class in the cache file, so it will trigger the autoloader to load the symfony core class, then we'll have a fatal error because we're declaring 2 times the same class.
I'm suggesting to add a way to prepend the classes to compile added by an extension, this way we could force classes from the core bundle to pe prepended, and avoid this kind of error with other bundles adding classes extending core classes or implementing core interface. I've also added it to the frameworkbundle.
---------------------------------------------------------------------------
by travisbot at 2012-07-01T12:32:28Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1750090) (merged a989d35c into a1b73887).
---------------------------------------------------------------------------
by schmittjoh at 2012-07-01T12:45:46Z
This doesn't sound really failure proof, better would be to resolve the compiled classes in a way that handles the dependencies gracefully regardless of the order that they are registered in.
---------------------------------------------------------------------------
by bamarni at 2012-07-01T13:03:15Z
yes that would be much cleaner, even if it's not perfect, I don't have any other examples, but I think in most of situations this error happens because of the frameworkbundle late registration?
---------------------------------------------------------------------------
by bamarni at 2012-07-01T23:07:24Z
I've added an automatic reordering as suggested. Now I don't get the error whatever the order of FrameworkBundle and JMSDIExtraBundle is.
---------------------------------------------------------------------------
by vicb at 2012-07-02T10:02:33Z
Does the algo really works ?
Let's say I have the following hierarchy `A < B < C` and the classes added in the following order; `C, B, A`. I believe that the algo would return `B, A, C` instead of `A, B, C`.
An other question: should the dependency resolution be implemented in the `ClassCollectionLoader` rather than in the compiler pass ?
---------------------------------------------------------------------------
by vicb at 2012-07-02T10:23:11Z
@bamarni could you confirm the issue with the algo ? If it is confirmed, do you have time to work on a fix ?
---------------------------------------------------------------------------
by bamarni at 2012-07-02T11:05:53Z
the algo looks correct, there is a loop checking dependencies and appending the parent classes at the begining so it should also work with A > B > C, but I'll check and add a unit test.
You're right about the location it could belong there if we want a general fix, should I put it in the ClassLoader component?
---------------------------------------------------------------------------
by vicb at 2012-07-02T11:28:14Z
Yep please move the code to the ClassLoader.
You could also add some cache mechanism:
- ReflectionClass could be cached (they might be use mulitple times when some dependencies exist and are also used by the class loader)
- May be you could also cache the hierarchy of the classes
---------------------------------------------------------------------------
by bamarni at 2012-07-02T17:25:08Z
@vicb : indeed there were something wrong with the algo when it needed to handle more than 1 level of dependency, it's fixed, I've added a few test case and it passes.
Caching the dependencies found with reflection would be useful if people are dumping several files with some similar classes in the same process, but I don't see why we would need to cache the hierarchy?
---------------------------------------------------------------------------
by vicb at 2012-07-02T18:29:06Z
@bamarni I was referring to a local cache (ie local variable).
- On the first iteration, the code instantiate a reflection class,
- On other iteration you would instantiate again a reflection class for a subset of the classes
- ...
- In the end the former code instantiate again a reflection class for each class in order to get the file name, ...
I was also thinking of an other algo: count the number of parent classes for each classes to include and order classes according to this number. This would be a single loop only, what do you think ?
---------------------------------------------------------------------------
by bamarni at 2012-07-02T19:32:34Z
hum good idea counting the parents is cleaner, even though it looks enough, would it also make sense to treat interfaces and classes separately? I'm thinking about a file with all the interfaces at the begining (ordered by number of parents desc), then the classes would be appended (ordered by number of parents desc too).
---------------------------------------------------------------------------
by vicb at 2012-07-03T07:05:07Z
There is no real benefit in making the interfaces appear first but you can do it if it doesn't make the code more complex.
Please also add the `@throws` annotation to the methods.
---------------------------------------------------------------------------
by bamarni at 2012-07-03T09:37:28Z
@vicb : I've changed it to use the parents count, looks good to me.
---------------------------------------------------------------------------
by bamarni at 2012-07-03T12:23:46Z
changed
---------------------------------------------------------------------------
by bamarni at 2012-07-03T12:42:29Z
fixed
Commits
-------
3466896 [Routing] fix encoding of static static, so UrlGenerator produces valid URLs
Discussion
----------
[Routing] fix encoding of static text
Fixes#4166
As requested by Fabien, I split #4205 into multiple PRs.
---------------------------------------------------------------------------
by stof at 2012-06-09T12:49:32Z
github tells me this PR cannot be merged automatically. Could you rebase it ?
---------------------------------------------------------------------------
by Tobion at 2012-06-09T13:12:55Z
Done
---------------------------------------------------------------------------
by travisbot at 2012-06-09T13:18:18Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1576266) (merged 3466896a into 15b5aa4f).
Commits
-------
d9439ab made the charset overridable (closes#2072)
Discussion
----------
made the charset overridable (closes#2072)
The charset was configurable in a configuration file but it never worked:
framework:
charset: ISO-8859-1
Now, like for the cache and log dirs, you can configure the charset by
overriding the getCharset() method in the app kernel:
public function getCharset()
{
return 'ISO-8859-1';
}
---------------------------------------------------------------------------
by fabpot at 2012-07-03T07:26:04Z
See #2072 for the previous attempts to fix this issue.
The charset was configurable in a configuration file but it never worked:
framework:
charset: ISO-8859-1
Now, like for the cache and log dirs, you can configure the charset by
overriding the getCharset() method in the app kernel:
public function getCharset()
{
return 'ISO-8859-1';
}
Commits
-------
6296a24 Standalone query string normalization
Discussion
----------
[HttpFoundation] Standalone query string normalization
I wanted to leverage query string normalization in a test. I considered copying this code to my own library but I noticed that the only instance data `getQueryString` needed to know from the `Request` was `QUERY_STRING` so I broke the rest out into a standalone static function.
---------------------------------------------------------------------------
by simensen at 2012-07-02T20:05:59Z
I made the requested changes.
---------------------------------------------------------------------------
by fabpot at 2012-07-02T20:10:27Z
Can you squash your commits? Thanks.
---------------------------------------------------------------------------
by simensen at 2012-07-02T20:16:52Z
Sure, no problem. Hopefully I did it correctly.
Commits
-------
8ffaafa Make the session entry for the target url firewall dependent.
Discussion
----------
[Security] Make the session entry for the target url firewall dependent.
Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets:
License of the code: MIT
If there are two firewalls (eg. main and admin), calling an protected admin url
will direct you to the login form of the admin. If I ignore this and go to the login
form of the main firewall directly I will end up being redirected to the stored
admin target url, which will lead me to the admin login form again.
---------------------------------------------------------------------------
by travisbot at 2012-05-25T09:33:44Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1431566) (merged 8ffaafa8 into 45849ce3).
---------------------------------------------------------------------------
by uwej711 at 2012-06-09T08:05:54Z
Doesn't this make sense or did this slip through? Or is there something missing?
Commits
-------
88caf3a [HttpKernel] removed the storage of the current locale in the session
Discussion
----------
[HttpKernel] removed the storage of the current locale in the session
Before this commit, the current locale was stored in the session (if one
was already started). That way, for the next requests, even if the
request locale attribute was not set, the locale was "restored".
But this is a really bad practice as it means that the same URL can have
a different content depending on the previous requests. It would have
been better if the Vary header was set but the locale can be different
from the value coming from the Accept-Language anyway.
This is a BC break but fortunately, you can restore the 2.0 behavior by
creating a simple event listener that contains the logic removed by this
commit.
---------------------------------------------------------------------------
by travisbot at 2012-07-01T06:56:48Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1748659) (merged 009e30f0 into 2e356c1a).
---------------------------------------------------------------------------
by schmittjoh at 2012-07-01T08:15:46Z
How about using a cookie instead? It would remove the BC break, and also be possible to use a vary header?
---------------------------------------------------------------------------
by fabpot at 2012-07-01T09:13:44Z
The goal is to make Symfony as stateless as possible; introducing a cookie would defeat this goal.
---------------------------------------------------------------------------
by drak at 2012-07-01T09:19:37Z
@fabpot - thank you for bringing this to attention. I was meaning to do it a long time ago. The requested language is entirely a per request issue and must always be so. URLs must only ever return one content, and not multiple (e.g. different languages). The correct way to behave is to detect the language based on URL and failing that where a language is not requested, to look at the preferred language from the browser request and if available it can be redirected to that resource (e.g. /fr). This is what we do in Zikula. We have a further session based setting for "preferred language" which if set will override the browser default.
In summary:
1. If the language is specified in the GET request, return that language always. E.g. domain.com/fr/foo should return a French version of foo
2. If no language is specified in the GET request: first check the session for a preferred language, otherwise check the browser string for the preferred language and then if necessary, redirect to that resource. We have a setting which additionally say "always have language in URL, and don't put language code in URL for default language"
This means what in Zikula we only ever have one URL per language version of a page, but it still allows for users to set their preferred language which is taken in to account mainly when they visit the homepage (but in fact any page without a specific language in the request).
---------------------------------------------------------------------------
by drak at 2012-07-01T09:38:06Z
+1 on this PR. Basically the request locale should be in the Request object and calculated according to the applications preferences.
---------------------------------------------------------------------------
by schmittjoh at 2012-07-01T12:38:25Z
I agree that content must be detected based on the request, but I strongly disagree with relying entirely on the URL.
@fabpot, if you think about it using a cookie would still be stateless. There would be no state whatsoever, the detection would be entirely based on the request. Whether the language information is transmitted in the URL or as part of request headers is for the developer to decide eventually, at least IMO. My suggestion would just provide a default which is more BC.
---------------------------------------------------------------------------
by drak at 2012-07-01T20:08:50Z
@schmittjoh it's not entirely from the URL, there are browser preferences and also user defaults ca nalso available but the latter is slightly higher level. IMO it's not really Symfony's job here, it's application level specific. We have a pretty good working example of that in Zikula. Anyone can easily implement your own requirements with a listener.
What is absolutely clear however is it is wrong for one URL to deliver more than one version of any content.
---------------------------------------------------------------------------
by schmittjoh at 2012-07-01T21:16:52Z
I'm 100% for this change. My suggestion would just be more BC while still keeping Symfony2 stateless. Of course, it can be easily implemented in userland if we do not care about BC here.
Regarding different URLs per content, I do not think that this is our decision to make. Generally, developers should be able to make whatever content negotation they see fit. Whether they rely solely on the URL, or also take other request headers into account should not be limited by Symfony2.
---------------------------------------------------------------------------
by fabpot at 2012-07-02T10:37:26Z
I've added a paragraph in the UPGRADE file with a listener example that can be used to keep BC.
Before this commit, the current locale was stored in the session (if one
was already started). That way, for the next requests, even if the
request locale attribute was not set, the locale was "restored".
But this is a really bad practice as it means that the same URL can have
a different content depending on the previous requests. It would have
been better if the Vary header was set but the locale can be different
from the value coming from the Accept-Language anyway.
This is a BC break but fortunately, you can restore the 2.0 behavior by
creating a simple event listener that contains the logic removed by this
commit.
Commits
-------
4d09fe6 [Finder] '*' and '?' are considered are glob pattern rather than delimiters (fix#4664)
Discussion
----------
[Finder] '*' and '?' are considered are glob pattern rather than delimit...
...ers (fix#4664)
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
-------
7464dcd added phpdoc
c413e7b [Routing] remove RequestContextAwareInterface from RequestMatcherInterface
921be34 [Routing] fix phpdoc
Discussion
----------
[Routing] RequestMatcherInterface doesn't need context
Matchers that implement RequestMatcherInterface should match a Request, thus they don't need the request context.
---------------------------------------------------------------------------
by travisbot at 2012-06-14T21:39:48Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1624496) (merged f5ff1fe0 into 7c91ee57).
---------------------------------------------------------------------------
by schmittjoh at 2012-06-15T13:32:59Z
I think it makes sense to remove the RequestContext from the RequestMatcher.
---------------------------------------------------------------------------
by travisbot at 2012-06-15T15:54:28Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1628931) (merged 7464dcd2 into f881d282).
---------------------------------------------------------------------------
by Tobion at 2012-06-26T12:32:06Z
Anything missing?
Commits
-------
c1e4166 moved create of default form label to view layer
Discussion
----------
move create of default form label to view layer
A small optimization if you provide custom labels in the view layer (i.e. `{{ form_label(form.name, 'Your name') }}`
```
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: ~
Todo: ~
License of the code: MIT
Documentation PR: ~
```
---------------------------------------------------------------------------
by travisbot at 2012-06-24T14:45:17Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1694310) (merged 37f0b774 into 0d4b02e4).
---------------------------------------------------------------------------
by travisbot at 2012-06-24T15:03:44Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1694418) (merged c1e4166e into 0d4b02e4).
Commits
-------
911db69 [FrameworkBundle] Typo fix
19eeac8 [HttpFoundation] Removed erroneous reliance on session.auto_start
dcac5d7 [HttpFoundation] Corrected docblocks and properties.
1fd66f3 [FrameworkBundle] Remove 'auto_start' configuration parameter.
Discussion
----------
[HttpFoundation] Remove session start on demand
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
License of the code: MIT
This removes false reliance on ini directive `session.auto_start` to allow a session to start when session bags are accessed before the `SessionStorageInterface` is started.
Sessions must be explicitly started in all circumstances.
---------------------------------------------------------------------------
by stloyd at 2012-06-13T07:22:24Z
@drak Shouldn't you add note about this change in upgrade file ?
---------------------------------------------------------------------------
by drak at 2012-06-13T15:13:37Z
It's a development version change, so not really. But saying that, I have a bunch of documentation to amend when this gets merged and at that time I'll make sure the changelogs and upgrading are up to date as part of that.
---------------------------------------------------------------------------
by dlsniper at 2012-06-13T21:57:28Z
@drak If this change will kick in what does one user of Symfony 2 Standard must do in order to keep compat with this merge? I see that you said you'll update the docs but until that happens some might upgrade their app directly to master :)
Thanks.
---------------------------------------------------------------------------
by drak at 2012-06-14T01:36:04Z
@dlsniper - nothing. This corrects a bug and inconsistency.
---------------------------------------------------------------------------
by travisbot at 2012-06-29T17:48:42Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1739033) (merged 19eeac88 into 62100f1a).
---------------------------------------------------------------------------
by drak at 2012-06-29T17:55:13Z
@fabpot ping. The failing Travis is nothing to do with this PR (see the travis logs).
---------------------------------------------------------------------------
by travisbot at 2012-06-29T20:39:43Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1739805) (merged 911db69b into 62100f1a).
Commits
-------
df8d94e added Request::getSchemeAndHttpHost() and Request::getUserInfo() (closes#4312, refs #3416, refs #3056)
Discussion
----------
added Request::getSchemeAndHttpHost() and Request::getUserInfo()
see #4312
---------------------------------------------------------------------------
by travisbot at 2012-06-28T15:22:03Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1730172) (merged 598bd56f into 0d275701).
---------------------------------------------------------------------------
by Seldaek at 2012-06-28T15:22:35Z
Why not just `getSchemeAndHost`? That sounds long enough, and is fairly explicit given the context.
---------------------------------------------------------------------------
by fabpot at 2012-06-28T15:25:34Z
@Seldaek because (and that's probably unfortunate) we have both `getHost()` and `getHttpHost()`. The former does not include the port whereas the latter does.
---------------------------------------------------------------------------
by Seldaek at 2012-06-28T15:26:42Z
Ok makes sense.
---------------------------------------------------------------------------
by travisbot at 2012-06-28T16:11:16Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1730630) (merged df8d94e3 into 884a8264).
Commits
-------
b217897 [HttpFoundation] Complete Request::overrideGlobals
Discussion
----------
[2.2][HttpFoundation] complete Request::overrideGlobals
Bug fix: yes
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: [](http://travis-ci.org/stealth35/symfony)Fixes the following tickets: -
Todo: -
---------------------------------------------------------------------------
by stealth35 at 2011-12-15T14:20:36Z
Thank guys, should be better now
---------------------------------------------------------------------------
by stealth35 at 2011-12-15T16:14:40Z
@stloyd ✌️
---------------------------------------------------------------------------
by stloyd at 2011-12-15T16:22:48Z
@stealth35 You should update also [`RequestTest`](https://github.com/symfony/symfony/blob/master/tests/Symfony/Tests/Component/HttpFoundation/RequestTest.php#L623) which would show you typos you have few mins ago ;-)
---------------------------------------------------------------------------
by stealth35 at 2011-12-15T16:57:16Z
@stloyd done, thanks for your review
---------------------------------------------------------------------------
by canni at 2011-12-15T20:27:28Z
As this is bugfix, this shouldn't be re-based against 2.0?
---------------------------------------------------------------------------
by stealth35 at 2011-12-15T20:50:05Z
@canni It's more a forget feature, I tagged it to bug fix because of the `FIXME`, and it's add a method, IMO there is no rush
---------------------------------------------------------------------------
by canni at 2011-12-15T20:55:28Z
@stealth35 no rush at all, I was just curious :)
---------------------------------------------------------------------------
by vicb at 2012-01-06T16:24:31Z
I would say "Backwards compatibility break: yes" i.e.tests have been modified.
---------------------------------------------------------------------------
by stealth35 at 2012-01-06T16:36:15Z
@vicb the tests are not modified, just some addition
---------------------------------------------------------------------------
by vicb at 2012-01-06T16:40:30Z
@stealth35 https://github.com/symfony/symfony/pull/2892/files#L2R46
---------------------------------------------------------------------------
by stealth35 at 2012-01-06T17:13:07Z
@vicb it's not a compatibility break ...
---------------------------------------------------------------------------
by vicb at 2012-01-06T17:19:35Z
Well, same inputs, different outputs, this is a compatibility break to me.
But however it is named we should not change the behavior of this class; Client values are values as passed by the client you should no try to guess them.
---------------------------------------------------------------------------
by stealth35 at 2012-01-06T17:32:41Z
@vicb the behavior ? when you change the GET or POST values with `HttpFoundation\*Bag` (replace/set) it's the same thing
---------------------------------------------------------------------------
by vicb at 2012-01-06T17:35:39Z
I am referring to the difference in behavior between the current implementation and the version in this PR.
They do not behave the same and that's why the tests have been modified.
---------------------------------------------------------------------------
by fabpot at 2012-02-14T23:33:42Z
any progress on this PR?
---------------------------------------------------------------------------
by vicb at 2012-02-15T07:48:34Z
To make it clear I strongly disagree with the modifs in this PR. Available to help if needed.
---------------------------------------------------------------------------
by stealth35 at 2012-02-15T09:24:50Z
@fabpot Well, `move_uploaded_file` will not work so I have some doubt about this, @vicb just don't like the fact to add the mime type type and the size, it's not an important thing, I can remove it we can discuss later about that,
@vicb the last thing to do, it's to recreate the weird php $_FILES array
---------------------------------------------------------------------------
by vicb at 2012-02-23T17:11:29Z
@stealth35 I don't think we can bypass the `move_uploaded_file` security check - which is good. Is there any interest in this PR w/o this ?
If no we should just update phpDoc comment and remove the FIXME (meaning we can not override the `$_FILES`).
---------------------------------------------------------------------------
by stealth35 at 2012-03-10T16:13:14Z
@vicb updated
---------------------------------------------------------------------------
by vicb at 2012-03-11T09:38:20Z
@stealth35 what about adding some unit tests ?
---------------------------------------------------------------------------
by stealth35 at 2012-03-11T11:06:44Z
> what about adding some unit tests ?
@vicb `request_order` is PHP_INI_PERDIR, so I don't really how to handle this
---------------------------------------------------------------------------
by vicb at 2012-03-11T11:15:55Z
by creating a `protected getRequestOrder()` method or something like this ?
---------------------------------------------------------------------------
by stealth35 at 2012-03-11T11:36:11Z
it's too bad to create a method just for this, I can make cond in the test
``` php
<?php
$request->initialize(array('get' => 'foo'), array('post' => 'bar'));
$request->overrideGlobals();
$request_order = ini_get('request_order');
if ('gp' === $request_order) {
$this->assertEquals(array('get' => 'foo', 'post' => 'bar'), $_REQUEST);
} else if ('pg' === $request_order) {
$this->assertEquals(array('post' => 'bar', 'get' => 'foo'), $_REQUEST);
}
// ...
```
---------------------------------------------------------------------------
by vicb at 2012-03-11T12:02:17Z
This would only test one case.
Some thoughts about your snippet:
* The init should probably be `$request->initialize(array('foo' => 'get'), array('foo' => 'post'));`,
* `$request_order` does not take into account `variables_order.ini`,
* missing `strtolower`
---------------------------------------------------------------------------
by fabpot at 2012-03-23T21:21:59Z
What's the status of this PR? What needs to be done before merging?
---------------------------------------------------------------------------
by stealth35 at 2012-03-24T18:33:42Z
@fabpot missing some tests, it's not essay to tests an `ini`directive, @vicb recommand a `getRequestOrder` method, it's not a bad idea
---------------------------------------------------------------------------
by vicb at 2012-03-24T20:06:53Z
and change `$request_order` to `$requestOrder` as suggested by @henrikbjorn I can't find where
---------------------------------------------------------------------------
by stealth35 at 2012-06-14T12:42:25Z
I need help for testing
``` php
<?php
$request = $this->getMock('Request', array('overrideGlobals', 'initialize'));
$request->expects($this->any())
->method('getRequestOrder')
->will($this->returnValue('gp'));
$request->initialize(array('foo' => 'fooget'), array('foo' => 'foopost'));
$request->overrideGlobals();
$this->assertEquals(array_merge($_GET, $_POST), $_REQUEST);
```
Commits
-------
a609d55 [Locale] fixed StubIntlDateFormatter to behave like the ext/intl implementation
Discussion
----------
[2.0][WIP][Locale] StubIntlDateFormatter should use the TZ environment variable instead of the PHP's date.timezone setting
Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #3841
Todo: Check ext/intl changes for the next PHP 5.4 release
License of the code: MIT
There were changes that need to be investigated for the next PHP 5.4 release:
- [php-src @ eb346ef](eb346ef0f4)
- [php-src @ 888e77f](888e77ff73)
A strong evidence of bug in ext/intl was found while testing `StubIntlDateFormatter`. See the comment available at the docblock of `StubIntlDateFormatterTest`'s `testFormatWithDefaultTimezoneIntlShouldUseTheTzEnvironmentVariableWhenAvailable()` method and the following Gist for test scripts: https://gist.github.com/2946342
Maybe the upcoming PHP 5.4 release fix this bug since it will use the PHP's `date.timezone` when no time zone is provided. If confirmed the bug, it will need to be reported to the ext/intl maintainers.
---------------------------------------------------------------------------
by travisbot at 2012-06-18T05:02:05Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1644431) (merged a609d55c into cd0aa378).
---------------------------------------------------------------------------
by fabpot at 2012-06-28T14:09:08Z
@eriksencosta Now that PHP 5.4.4 is out, our tests for the Locale components are broken. Is this PR ready to be merged?
---------------------------------------------------------------------------
by eriksencosta at 2012-06-28T14:53:14Z
@fabpot the failed test case seems unrelated to this issue. I will debug it.
Failed test: `Locale\Tests\Stub\StubNumberFormatterTest::testParseTypeInt64IntlWith32BitIntegerInPhp32Bit`
Recent build job: http://travis-ci.org/#!/symfony/symfony/jobs/1729618
I just need to confirm mine todo note. If you want, merge it, I'll track this and make a new PR if needed (possibly only to remove the TODO note.)
Commits
-------
9d730be Method rename and phpdoc fixes
e01a95e Add a set of unit tests for the ExprBuilder class
Discussion
----------
Add a set of unit tests for the Config/ExprBuilder class
---------------------------------------------------------------------------
by travisbot at 2012-06-13T14:55:31Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1611400) (merged e01a95e1 into c07e9163).
---------------------------------------------------------------------------
by jeanmonod at 2012-06-13T22:04:52Z
Hi there,
I write all these tests because I'll come latter with an other PR that extend the ExprBuilder functionality. But I'm not sure I use the best way for testing this class. It's working, but some refactoring suggestions will be welcome...
@stof and @schmittjoh what do you think about that?
