Skip to content

[HttpFoundation] Add InputBag #34363

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.

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
Apr 13, 2020
Merged

[HttpFoundation] Add InputBag #34363

merged 1 commit into from
Apr 13, 2020

Conversation

azjezz
Copy link
Contributor

@azjezz azjezz commented Nov 13, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? yes
License MIT

When ppl read a request attribute, they never check if an array is returned
This means many apps just fail with a 500 when adding [] in the query string.
This PR turns them to 400 basically (with a deprecation for now)

@derrabus
Copy link
Member

Doesn't the parameter bag have all those fancy get*() methods for this purpose?

@azjezz
Copy link
Contributor Author

azjezz commented Nov 13, 2019

There's no getString() or/and getArray() 😄

@derrabus
Copy link
Member

There's no getString() or getArray() 😄

If this is what you're missing, why don't we add those?

@azjezz
Copy link
Contributor Author

azjezz commented Nov 13, 2019

If this is what you're missing, why don't we add those?

because people are already using get() as getString(), you can check this by a simple search in GitHub : https://github.com/search?l=PHP&q=%24request-%3Equery-%3Eget%28&type=Code

this can ( and already does ) cause issues :

$term = $request->query->get('q');
if ($term) {
  $results = $repository->search($term);
} else {
  $results = [];
} 

...

class SomethingRepository
{
   ....
   public function search(string $term): array

app.wip/search?q=foo

works

app.wip/search?q[]=foo

500 error, should be 400 instead :)

@derrabus
Copy link
Member

I understand what you're trying to accomplish. But right now, you're deprecating the only way to access the raw value of an item inside the bag.

@azjezz
Copy link
Contributor Author

azjezz commented Nov 13, 2019

It highly unlikely that you would request raw value, not knowing its type. i suppose we can add getRaw(), but its possible to access raw value using $request->query->all()['foo'] ?? $default

@derrabus
Copy link
Member

It highly unlikely that you would request raw value, not knowing its type.

It might not be the common case, but it has to remain possible.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 13, 2019

Please don't deprecate things that work. Changing for the sake of strictness must be worth the trouble it will incur on the community.
Here, the solution is known: make a stricter child class and use it when applicable.

@azjezz azjezz force-pushed the req-get-all branch 2 times, most recently from 3f430d6 to 05e0587 Compare November 13, 2019 22:37
@azjezz azjezz changed the title [HttpFoundation] deprecate using parameterBag::get() with non-string values [HttpFoundation] Add InputBag Nov 13, 2019
@azjezz azjezz force-pushed the req-get-all branch 5 times, most recently from 8430bdc to ed6c9d8 Compare November 14, 2019 09:44
@apfelbox
Copy link
Contributor

apfelbox commented Nov 14, 2019

@nicolas-grekas @derrabus what are your thoughts about adding more strict validation in the specialized methods like ->getInt().

Because right now:

// URL: /test?a[]=5&a[]=6

assert(1 === $request->getInt("a"));

seems like a bug to me. wdyt?

PS: I am not talking about the general ->get(), only about ->get{Alpha,Digit,..}()

@derrabus
Copy link
Member

@apfelbox Those methods simply wrap ext-filter. I agree that the behavior of that extension is not ideal. A better/stricter input validation layer could be worth investigating.

@MLukman
Copy link

MLukman commented Sep 21, 2020

There's no getString() or/and getArray() 😄

And better if there are isString() or/and isArray() so we can do logic to use get($key) or all($key). This deprecation relies on the developer to know that a key would always be string or array, while in reality it can be either and it's up to the developer to do due diligence to perform the checking.

@azjezz
Copy link
Contributor Author

azjezz commented Sep 21, 2020

This deprecation relies on the developer to know that a key would always be string or array, while in reality it can be either and it's up to the developer to do due diligence to perform the checking.

You should always know what type of input you are expecting, if you are expecting a string, use ->get('name'), if an array, use ->all('users').

note: ->all('users') already throws an exception if case users is a string.

In some rare cases, you might be expecting an array or string ( you should probably move to using arrays always in this case ), but you can still do:

$input = $request->request->all();
$users = $input['users'];
if (is_string($users)) {
  $users = [$users];
}

// $users is known to be an array.

If the client passed you the wrong type in production, you will have a deprecation message that you should just ignore, as Symfony 6.0 will throw an exception to the client ( bad request ).

see symfony/symfony-docs#13543

@VincentLanglet
Copy link
Contributor

In the following code

/**
     * Sets an input by name.
     *
     * @param string|array $value
     */
    public function set(string $key, $value)
    {
        if (!is_scalar($value) && !method_exists($value, '__toString') && !\is_array($value)) {
            trigger_deprecation('symfony/http-foundation', '5.1', 'Passing "%s" as a 2nd Argument to "%s()" is deprecated, pass a string or an array instead.', get_debug_type($value), __METHOD__);
        }

        $this->parameters[$key] = $value;
    }
  • The phpdoc talk about string|array
  • The deprecation talk about string|array
  • The code is checking for scalar or array.

What is the truth ? Is it deprecated for any non-string value or any non-scalar value ?
If someone use ->set('foo', false), does the code should be updated ?

@azjezz
Copy link
Contributor Author

azjezz commented Mar 28, 2021

@VincentLanglet scalar values can be cast to string when strict_types=1, that's why it was allowed as far as i remember.

@VincentLanglet
Copy link
Contributor

Just found a @nicolas-grekas comment

all is_string() checks in the PR are too strict, we want to accept any stringable:

is_scalar($v) || is_callable([$v, '__toString'])

So I assume the phpdoc should be updated

@azjezz
Copy link
Contributor Author

azjezz commented Mar 28, 2021

no, it should still support array as of symfony 5.x, in symfony 6.0 the signature can change to set(string $key, string|Stringable $value)

@VincentLanglet
Copy link
Contributor

set(string $key, string|Stringable $value)

If so, the check should be is_string, because scalar won't work anymore
http://sandbox.onlinephpfunctions.com/code/d23a9fa709a138bc57012a7b9e358705e4c441b7

@azjezz
Copy link
Contributor Author

azjezz commented Mar 28, 2021

it will if strict_types is set to 0, set used to accept mixed, since we want to keep the deprecation "soft", we wanna to still allow scalar values at runtime ( https://3v4l.org/7W8Y9 ), but statically, it suggest string, which is more correct.

@Nerogee
Copy link

Nerogee commented Dec 3, 2023

If this is what you're missing, why don't we add those?

because people are already using get() as getString(), you can check this by a simple search in GitHub : https://github.com/search?l=PHP&q=%24request-%3Equery-%3Eget%28&type=Code

this can ( and already does ) cause issues :

$term = $request->query->get('q');
if ($term) {
  $results = $repository->search($term);
} else {
  $results = [];
} 

...

class SomethingRepository
{
   ....
   public function search(string $term): array

app.wip/search?q=foo

works

app.wip/search?q[]=foo

500 error, should be 400 instead :)

I used get() for all type of value. I don't know the purpose of this change. It's now breaking all my codes. Please enlighten me or show me a case that why it worth introducing breaking changes.

As for the above given example, I cannot figure out your purpose of showing that. But to me, the demo code itself is wrong. Why? because the programmer should always validate the input value and make sure it's the desired data type before calling the search($term)

Additionally, sometimes programmer would like to allow front-end submitted value to be either a string or an array, and then handle it differently in the back-end. But after this breaking change, it becomes impossible to do that.

@wouterj
Copy link
Member

wouterj commented Dec 3, 2023

This ship sailed long ago. As you can see, this has been discussed numerous times 2 years ago, so it's unlikely that the outcome changes now. Furthermore, both deprecation and new behavior has been part of Symfony for nearly 4 years. Changing or reverting now is impossible and will impact more applications than changing this in existing applications that are upgrading to 5.x or 6.x now.

Please also do not comment more or less the same things across multiple issues/PRs within the same topic. This divides the same discussion across multiple places, making it harder to track.

@derrabus
Copy link
Member

derrabus commented Dec 3, 2023

I'm locking all of those issues now. If there's something NEW to discuss, please open a new issue.

@symfony symfony locked and limited conversation to collaborators Dec 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.