Skip to content
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

[HttpFoundation] Add InputBag #34363

Open
wants to merge 1 commit into
base: master
from
Open

[HttpFoundation] Add InputBag #34363

wants to merge 1 commit into from

Conversation

@azjezz
Copy link
Contributor

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)

@azjezz azjezz force-pushed the azjezz:req-get-all branch from 4f410a3 to ae1e05b Nov 13, 2019
@nicolas-grekas nicolas-grekas added this to the next milestone Nov 13, 2019
@azjezz azjezz force-pushed the azjezz:req-get-all branch 2 times, most recently from b4bce26 to b6cd87b Nov 13, 2019
@derrabus

This comment has been minimized.

Copy link
Contributor

derrabus commented Nov 13, 2019

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

@azjezz

This comment has been minimized.

Copy link
Contributor Author

azjezz commented Nov 13, 2019

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

@derrabus

This comment has been minimized.

Copy link
Contributor

derrabus commented Nov 13, 2019

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

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

@azjezz

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

derrabus commented Nov 13, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

derrabus commented Nov 13, 2019

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

This comment has been minimized.

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 azjezz:req-get-all branch 2 times, most recently from 3f430d6 to 05e0587 Nov 13, 2019
@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 azjezz:req-get-all branch 5 times, most recently from 8430bdc to ed6c9d8 Nov 13, 2019
@apfelbox

This comment has been minimized.

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,..}()

@azjezz azjezz force-pushed the azjezz:req-get-all branch from ed6c9d8 to a088a7b Nov 14, 2019
@azjezz azjezz requested a review from xabbuh as a code owner Nov 14, 2019
@azjezz azjezz force-pushed the azjezz:req-get-all branch from a088a7b to 5a445f7 Nov 14, 2019
@derrabus

This comment has been minimized.

Copy link
Contributor

derrabus commented Nov 17, 2019

@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.

@azjezz azjezz force-pushed the azjezz:req-get-all branch 2 times, most recently from 55421e2 to 2f1946f Feb 5, 2020
@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Feb 6, 2020

i agree that other getters ( e.g getInt ) should be fixed to throw a bad request exception too.

correct me if im wrong, but this PR makes that happen as of 6.0 because Request uses the new InputBag (keeping ParameterBag as-is works for me).

what about adding a deprecation before

throw new \RuntimeException(sprintf('The %s HTTP header is not parseable (%s).', $key, $value));
to notify this will be a different exception in 6.0 ... that should solve all @Tobion's concerns :)

@azjezz

This comment has been minimized.

Copy link
Contributor Author

azjezz commented Feb 6, 2020

by "getters" i mean the getters in the InputBag inherited from ParameterBag ( getInt, getFloat, getAlnum ... etc ), the PR scope only covers input values that are usually the result of a form input.

I believe there's many things to fix in the HttpFoundation component when it comes to types, but let's go step by step :)

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented Feb 6, 2020

yes but all getters rely on calling get(), which is overridden. So those will trigger deprecations too ...

@azjezz

This comment has been minimized.

Copy link
Contributor Author

azjezz commented Feb 6, 2020

getBoolean, getInt, getDigits, getAlnum, getAlpha all pass either a string, int, or a boolean to get as a default, meaning it won't trigger deprecation for the default value, return value of get should be a string ( boolean, and numbers in $_REQUEST and other super globals are strings ), so it shouldn't trigger any deprecation if you are getting a number, boolean, .. etc.

however, the deprecation trigger in this case :

// url : /foo?alpha=[]

$input->getAlpha('alpha');

still stands since it will result in exception in 6.0 :)

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 7, 2020

We forgot about the FILTER_REQUIRE_ARRAY flag of the filter() method. It should not throw a deprecation when in use.

@azjezz azjezz force-pushed the azjezz:req-get-all branch 4 times, most recently from 4b6fbaf to 5492949 Feb 7, 2020
@azjezz azjezz force-pushed the azjezz:req-get-all branch 2 times, most recently from 6877b65 to 3cdc418 Feb 7, 2020
@azjezz azjezz force-pushed the azjezz:req-get-all branch from 3cdc418 to 4961609 Feb 7, 2020
@azjezz azjezz force-pushed the azjezz:req-get-all branch 2 times, most recently from c1658e1 to 3572be5 Feb 7, 2020
@nicolas-grekas nicolas-grekas force-pushed the azjezz:req-get-all branch 2 times, most recently from 8a272f8 to dac9845 Feb 17, 2020
Copy link
Member

nicolas-grekas left a comment

I just rebased the PR to fix a simple merge conflict and to leverage the new deprecation-contracts.
Good to go on my side.
This is a really nice incremental improvement that makes the behavior of the app more predictable in a case that is mostly unhandled right now, while it should. Now the component will. Big win for everyone.

@nicolas-grekas nicolas-grekas force-pushed the azjezz:req-get-all branch from dac9845 to 96ae38a Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.