Skip to content

[HttpFoundation] use InputBag for Request::$request only if data is coming from a form #37265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 15, 2020
Merged

[HttpFoundation] use InputBag for Request::$request only if data is coming from a form #37265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 3 additions & 14 deletions src/Symfony/Component/HttpFoundation/InputBag.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,29 +36,18 @@ public function get(string $key, $default = null)
$value = parent::get($key, $this);

if (null !== $value && $this !== $value && !is_scalar($value) && !(\is_object($value) && method_exists($value, '__toString'))) {
trigger_deprecation('symfony/http-foundation', '5.1', 'Retrieving a non-string value from "%s()" is deprecated, and will throw a "%s" exception in Symfony 6.0, use "%s::all()" instead.', __METHOD__, BadRequestException::class, __CLASS__);
trigger_deprecation('symfony/http-foundation', '5.1', 'Retrieving a non-string value from "%s()" is deprecated, and will throw a "%s" exception in Symfony 6.0, use "%s::all($key)" instead.', __METHOD__, BadRequestException::class, __CLASS__);
}

return $this === $value ? $default : $value;
}

/**
* Returns the inputs.
*
* @param string|null $key The name of the input to return or null to get them all
* {@inheritdoc}
*/
public function all(string $key = null): array
{
if (null === $key) {
return $this->parameters;
}

$value = $this->parameters[$key] ?? [];
if (!\is_array($value)) {
throw new BadRequestException(sprintf('Unexpected value for "%s" input, expecting "array", got "%s".', $key, get_debug_type($value)));
}

return $value;
return parent::all($key);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you tried making the behavior between the two bags more similar.. nice.. not sure if you were driven by my comment... but this is indeed nice..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember exactly my flow of thoughts, but it might be :)

}

/**
Expand Down
18 changes: 16 additions & 2 deletions src/Symfony/Component/HttpFoundation/ParameterBag.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

namespace Symfony\Component\HttpFoundation;

use Symfony\Component\HttpFoundation\Exception\BadRequestException;

/**
* ParameterBag is a container for key/value pairs.
*
Expand All @@ -31,11 +33,23 @@ public function __construct(array $parameters = [])
/**
* Returns the parameters.
*
* @param string|null $key The name of the parameter to return or null to get them all
*
* @return array An array of parameters
*/
public function all()
public function all(/*string $key = null*/)
{
return $this->parameters;
$key = \func_num_args() > 0 ? func_get_arg(0) : null;

if (null === $key) {
return $this->parameters;
}

if (!\is_array($value = $this->parameters[$key] ?? [])) {
throw new BadRequestException(sprintf('Unexpected value for parameter "%s": expecting "array", got "%s".', $key, get_debug_type($value)));
}

return $value;
}

/**
Expand Down
10 changes: 6 additions & 4 deletions src/Symfony/Component/HttpFoundation/Request.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class Request
/**
* Request body parameters ($_POST).
*
* @var InputBag
* @var InputBag|ParameterBag
*/
public $request;

Expand Down Expand Up @@ -268,7 +268,7 @@ public function __construct(array $query = [], array $request = [], array $attri
*/
public function initialize(array $query = [], array $request = [], array $attributes = [], array $cookies = [], array $files = [], array $server = [], $content = null)
{
$this->request = new InputBag($request);
$this->request = new ParameterBag($request);
$this->query = new InputBag($query);
$this->attributes = new ParameterBag($attributes);
$this->cookies = new InputBag($cookies);
Expand Down Expand Up @@ -298,7 +298,9 @@ public static function createFromGlobals()
{
$request = self::createRequestFromFactory($_GET, $_POST, [], $_COOKIE, $_FILES, $_SERVER);

if (0 === strpos($request->headers->get('CONTENT_TYPE'), 'application/x-www-form-urlencoded')
if ($_POST) {
$request->request = new InputBag($_POST);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the developers using symfony that will create a form using form component, let's say a login form, will get the data back as

`login` => [
    `username` => `user`,
    `password` =>  `password`,
];

So I don't see this InputBag here serving the majority..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The form component is already strict about strings vs arrays in values and knows how to deal with InputBag.

Copy link
Contributor

@gmponos gmponos Jun 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the form component, as a component knows how to deal with the InputBag.. my point is that users that will build a form with the component... they will never use this:

$request->request->get('login');

What use cases/bugs are you trying to cover by using for the request attribute the InputBag?

This means many apps just fail with a 500 when adding [] in the query string.

All this started with the scenario that someone access a query parameter and thinking that it is string etc etc.. ok.. although I disagree with the notion of it.. it is a UseCase. I don't see how developers will be protected by using InputBag on the request since most of the cases are submitting associative arrays...

All I see is that I have to do this:

        $login = $request->request->get('login');
        return \is_array($login) && isset($login['username']) && isset($login['password']) && isset($login['_token']);

to:

        $request = $request->request->all();
        if (!isset($request['login'])) {
            return false;
        }

        $login = $request['login'];
        return \is_array($login) && isset($login['username']) && isset($login['password']) && isset($login['_token']);

it's not the code that I have to write.. it's that ParameterBag::get is totally useless..

Hope I am explaining in the best way.. if not.. maybe we should move on.. there might be more important things to care..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your "to" should be:

$request = $request->request->all('login');
return isset($login['username']) && isset($login['password']) && isset($login['_token']);

Then you'll let a BadRequestException bubble down and become a 400 as it should if you expect login to be an array. This "after" is much better than the "before" actually, with a richer behavior and more semantic code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then you'll let a BadRequestException bubble down and become a 400 as it should if you expect login to be an array.

I can't.. various technical reasons that are out of the topic of this discussion...

Still this remains:

it's not the code that I have to write.. it's that ParameterBag::get is totally useless..

} elseif (0 === strpos($request->headers->get('CONTENT_TYPE'), 'application/x-www-form-urlencoded')
&& \in_array(strtoupper($request->server->get('REQUEST_METHOD', 'GET')), ['PUT', 'DELETE', 'PATCH'])
) {
parse_str($request->getContent(), $data);
Expand Down Expand Up @@ -447,7 +449,7 @@ public function duplicate(array $query = null, array $request = null, array $att
$dup->query = new InputBag($query);
}
if (null !== $request) {
$dup->request = new InputBag($request);
$dup->request = new ParameterBag($request);
}
if (null !== $attributes) {
$dup->attributes = new ParameterBag($attributes);
Expand Down
18 changes: 1 addition & 17 deletions src/Symfony/Component/HttpFoundation/Tests/InputBagTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

use PHPUnit\Framework\TestCase;
use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
use Symfony\Component\HttpFoundation\Exception\BadRequestException;
use Symfony\Component\HttpFoundation\InputBag;

class InputBagTest extends TestCase
Expand All @@ -36,21 +35,6 @@ public function testGetDoesNotUseDeepByDefault()
$this->assertNull($bag->get('foo[bar]'));
}

public function testAllWithInputKey()
{
$bag = new InputBag(['foo' => ['bar', 'baz'], 'null' => null]);

$this->assertEquals(['bar', 'baz'], $bag->all('foo'), '->all() gets the value of a parameter');
$this->assertEquals([], $bag->all('unknown'), '->all() returns an empty array if a parameter is not defined');
}

public function testAllThrowsForNonArrayValues()
{
$this->expectException(BadRequestException::class);
$bag = new InputBag(['foo' => 'bar', 'null' => null]);
$bag->all('foo');
}

public function testFilterArray()
{
$bag = new InputBag([
Expand All @@ -77,7 +61,7 @@ public function testSetWithNonStringishOrArrayIsDeprecated()
public function testGettingANonStringValueIsDeprecated()
{
$bag = new InputBag(['foo' => ['a', 'b']]);
$this->expectDeprecation('Since symfony/http-foundation 5.1: Retrieving a non-string value from "Symfony\Component\HttpFoundation\InputBag::get()" is deprecated, and will throw a "Symfony\Component\HttpFoundation\Exception\BadRequestException" exception in Symfony 6.0, use "Symfony\Component\HttpFoundation\InputBag::all()" instead.');
$this->expectDeprecation('Since symfony/http-foundation 5.1: Retrieving a non-string value from "Symfony\Component\HttpFoundation\InputBag::get()" is deprecated, and will throw a "Symfony\Component\HttpFoundation\Exception\BadRequestException" exception in Symfony 6.0, use "Symfony\Component\HttpFoundation\InputBag::all($key)" instead.');
$bag->get('foo');
}

Expand Down
16 changes: 16 additions & 0 deletions src/Symfony/Component/HttpFoundation/Tests/ParameterBagTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\HttpFoundation\Tests;

use PHPUnit\Framework\TestCase;
use Symfony\Component\HttpFoundation\Exception\BadRequestException;
use Symfony\Component\HttpFoundation\ParameterBag;

class ParameterBagTest extends TestCase
Expand All @@ -27,6 +28,21 @@ public function testAll()
$this->assertEquals(['foo' => 'bar'], $bag->all(), '->all() gets all the input');
}

public function testAllWithInputKey()
{
$bag = new ParameterBag(['foo' => ['bar', 'baz'], 'null' => null]);

$this->assertEquals(['bar', 'baz'], $bag->all('foo'), '->all() gets the value of a parameter');
$this->assertEquals([], $bag->all('unknown'), '->all() returns an empty array if a parameter is not defined');
}

public function testAllThrowsForNonArrayValues()
{
$this->expectException(BadRequestException::class);
$bag = new ParameterBag(['foo' => 'bar', 'null' => null]);
$bag->all('foo');
}

public function testKeys()
{
$bag = new ParameterBag(['foo' => 'bar']);
Expand Down
11 changes: 11 additions & 0 deletions src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

use PHPUnit\Framework\TestCase;
use Symfony\Component\HttpFoundation\Exception\SuspiciousOperationException;
use Symfony\Component\HttpFoundation\InputBag;
use Symfony\Component\HttpFoundation\ParameterBag;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Session\Session;
use Symfony\Component\HttpFoundation\Session\Storage\MockArraySessionStorage;
Expand Down Expand Up @@ -1255,6 +1257,11 @@ public function testCreateFromGlobals($method)
{
$normalizedMethod = strtoupper($method);

$_POST = [];
$request = Request::createFromGlobals();
$this->assertNotInstanceOf(InputBag::class, $request->request);
$this->assertInstanceOf(ParameterBag::class, $request->request);

$_GET['foo1'] = 'bar1';
$_POST['foo2'] = 'bar2';
$_COOKIE['foo3'] = 'bar3';
Expand All @@ -1267,6 +1274,8 @@ public function testCreateFromGlobals($method)
$this->assertEquals('bar3', $request->cookies->get('foo3'), '::fromGlobals() uses values from $_COOKIE');
$this->assertEquals(['bar4'], $request->files->get('foo4'), '::fromGlobals() uses values from $_FILES');
$this->assertEquals('bar5', $request->server->get('foo5'), '::fromGlobals() uses values from $_SERVER');
$this->assertInstanceOf(InputBag::class, $request->request);
$this->assertInstanceOf(ParameterBag::class, $request->request);

unset($_GET['foo1'], $_POST['foo2'], $_COOKIE['foo3'], $_FILES['foo4'], $_SERVER['foo5']);

Expand All @@ -1275,6 +1284,8 @@ public function testCreateFromGlobals($method)
$request = RequestContentProxy::createFromGlobals();
$this->assertEquals($normalizedMethod, $request->getMethod());
$this->assertEquals('mycontent', $request->request->get('content'));
$this->assertInstanceOf(InputBag::class, $request->request);
$this->assertInstanceOf(ParameterBag::class, $request->request);

unset($_SERVER['REQUEST_METHOD'], $_SERVER['CONTENT_TYPE']);

Expand Down