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);
```
Symfony2 is a PHP 5.3 full-stack web framework. It is written with speed and
flexibility in mind. It allows developers to build better and easy to maintain
websites with PHP.
Symfony can be used to develop all kind of websites, from your personal blog
to high traffic ones like Dailymotion or Yahoo! Answers.
Requirements
Symfony2 is only supported on PHP 5.3.3 and up.
Installation
The best way to install Symfony2 is to download the Symfony Standard Edition
available at http://symfony.com/download.
Documentation
The "Quick Tour" tutorial gives you a first feeling of the framework. If,
like us, you think that Symfony2 can help speed up your development and take
the quality of your work to the next level, read the official
Symfony2 documentation.
Contributing
Symfony2 is an open source, community-driven project. If you'd like to contribute,
please read the Contributing Code part of the documentation. If you're submitting
a pull request, please follow the guidelines in the Submitting a Patch section.
9fb567dc43
