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

fix: Add array to return of InputBag::get #41766

Closed
wants to merge 1 commit into from
Closed

fix: Add array to return of InputBag::get #41766

wants to merge 1 commit into from

Conversation

shyim
Copy link
Contributor

@shyim shyim commented Jun 21, 2021

Q A
Branch? 5.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

The method can return array. The set method has already array typehint

@nicolas-grekas
Copy link
Member

This is on purpose. If you want to get an array, you should use all('foo').
See the deprecation in the body of the method.

@TjorvenB
Copy link

TjorvenB commented Jun 22, 2021

This is quite the BC break. Is there a place where we can find the reasoning behind this? There is no explanation in the upgrade notes or the commits.
It would be a nice read while I look for all controllers where I handle array query parameters and add a custom implementation for default values, as such are no longer supported.

@MaximePinot
Copy link
Contributor

This is quite the BC break. Is there a place where we can find the reasoning behind this? There is no explanation in the upgrade notes or the commits.
It would be a nice read while I look for all controllers where I handle array query parameters and add a custom implementation for default values, as such are no longer supported.

It was introduced in #34363. You will find some explanations here! ;)

@dvdknaap
Copy link

dvdknaap commented Jul 8, 2021

Only adding the array type will not fix it. the if statement check in the get should be the same as the set function so you should add !\is_array($value)

@stof
Copy link
Member

stof commented Jul 8, 2021

@nicolas-grekas even if it is deprecated, it can still return an array in 5.3

@nicolas-grekas
Copy link
Member

Sure, but we don't phpdocument deprecated types.

@stof
Copy link
Member

stof commented Jul 8, 2021

@nicolas-grekas we will need to change that policy anyway. With native return types, we cannot remove the deprecated types from the signature, otherwise we get a TypeError

@dvdknaap
Copy link

dvdknaap commented Dec 3, 2021

Only adding the array type in the docblock doesn't fix anything.

This is still a bug and throws a error because it doesn't accept array while it accepts it in the set function.
vendor/symfony/http-foundation/InputBag.php:36

        if (null !== $value && $this !== $value && !is_scalar($value)) {
            throw new BadRequestException(sprintf('Input value "%s" contains a non-scalar value.', $key));
        }

It should also accept array again because a lot of websites and app are broke now like mine.

It should accept the same as in the set function

vendor/symfony/http-foundation/InputBag.php:77

        if (null !== $value && !is_scalar($value) && !\is_array($value) && !$value instanceof \Stringable) {
            throw new \InvalidArgumentException(sprintf('Excepted a scalar, or an array as a 2nd argument to "%s()", "%s" given.', __METHOD__, get_debug_type($value)));
        }

@shyim
Copy link
Contributor Author

shyim commented Dec 3, 2021

@dvdknaap you need to call now all instead of get when you want a array

@szepczynski
Copy link
Contributor

szepczynski commented Mar 31, 2022

But how to test is given key is array or scalar value.
Let's go with this example:

GET https://localhost/?q=a
GET https://localhost/?q[]=a&q[]=b

First case you can get only through $request->query->get('q');
Second case only through $request->query->all('q');

I can't see a way how to test is q parameter is scalar or array (except catching exception of course)

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Apr 29, 2022

I also think this leads into some strange cases now specially when working with the FOSRestBundle which is setting the JSON Content to $request->request via the BodyListener. I would had understand if it go strict to string now as from the http specification but instead now float, int are allowed but no array feels strange.

@ddr5292
Copy link

ddr5292 commented Mar 23, 2023

This breaks my code guys. Bad form.

@ro0NL
Copy link
Contributor

ro0NL commented Mar 23, 2023

a deprecation breaks your code?

@ro0NL
Copy link
Contributor

ro0NL commented Mar 23, 2023

But how to test is given key is array or scalar value.

you mostly know what your application expects

getting the raw value is done using all()['key']. Please open a doc PR if it's unclear.

experteam-mx pushed a commit to experteam-mx/api-crud-bundle that referenced this pull request Apr 18, 2023
@christiaangoossens
Copy link

@ddr5292 And another one, also broke my code on a minor dependency update. For anyone searching for this on the internet, if you get Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value $key contains a non-scalar value. in /vendor/symfony/http-foundation/InputBag.php:37 you have a controller doing $request->json()-get($key) somewhere in your code on an array. Replace it with $request->input($key) (for Laravel).

Filoz added a commit to Filoz/form-filter-bundle that referenced this pull request Aug 17, 2023
Change from
$request->query->get
to
$request->query->all

because of Symfony 6 ( symfony/symfony#41766 (comment) ).
This change prevent this error: "Input value "horse_filter" contains a non-scalar value."
@Nerogee
Copy link

Nerogee commented Dec 3, 2023

But how to test is given key is array or scalar value. Let's go with this example:

GET https://localhost/?q=a
GET https://localhost/?q[]=a&q[]=b

First case you can get only through $request->query->get('q'); Second case only through $request->query->all('q');

I can't see a way how to test is q parameter is scalar or array (except catching exception of course)

I cannot agree more with you. I use get() for all type of data. but now it all mess up. The worst thing is that I cannot see the reason why it worth even introducing a breaking change. User submitted value could be very unpredictable. Sometimes it's on purpose and allowed to input either a scalar value or an array. What we need is the flexibility in the back-end to handle it.

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

None yet