Skip to content

httpfoundation: Add getArray & getString helpers #37229

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

Closed
wants to merge 1 commit into from

Conversation

flack
Copy link
Contributor

@flack flack commented Jun 11, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets related to #34363
License MIT
Doc PR tbd.

#34363 deprecated working with arrays. For example, if you have form handling code like this:

foreach ($request->request->get('tags', []) as $tag) {
    /* */
}

You'll get a deprecation notice starting in 5.1. The suggested workaround from the discussion in #34363 would make it look like this:

foreach (($request->request->all()['tags'] ?? []) as $tag) {
    /* */
}

which is not only way uglier, but also incomplete, since 'tags' could be a string at this point. So it would really have to look like this:

foreach (((array) $request->request->all()['tags'] ?? []) as $tag) {
    /* */
}

(if that even works? I haven't tested it).

So why not add a simple getArray() helper function? That also makes it easier for static analysis to verify that the code is actually correct.

Also, I added a getString() method for symmetry. Because I really think according to the logic in #34363 get should be deprecated or at least redirected to a getString function so that it's obvious what is going on, but I can also remove this part.

I know there are no tests and documentation, but I wanted to check if there is even interest in merging something like this before I go through all that (so consider this RFC)

@ro0NL
Copy link
Contributor

ro0NL commented Jun 11, 2020

you missed $request->request->all('tags') :)

@flack
Copy link
Contributor Author

flack commented Jun 11, 2020

@ro0NL oh, is that a thing? I was just looking here:

and it didn't seem like it would take a parameter

@flack
Copy link
Contributor Author

flack commented Jun 11, 2020

@ro0NL nevermind, found it. Right below the functions I added actually :-)

So yeah, that takes care of my problem then. I still think getArray and getString would be more explicit & discoverable, but well, no strong opinion.

@ro0NL
Copy link
Contributor

ro0NL commented Jun 11, 2020

i think the ship has sailed. InputBag is already part of a stable release.

adding getString/getArray now is just introducing more ways to do the same.

@flack
Copy link
Contributor Author

flack commented Jun 11, 2020

alright, closing then

@flack flack closed this Jun 11, 2020
@nicolas-grekas
Copy link
Member

getString($k) -> use get($)
getArray($k) -> use all($k) (not the variant mentioned in the description)

@flack
Copy link
Contributor Author

flack commented Jun 11, 2020

@nicolas-grekas yes, I know (now). I still find it confusing because it doesn't behave at all like you would expect from a child class of ParameterBag. all takes a parameter in the child class that will (contrary to what the function's name suggests) make it not return all the bag's contents, but only the raw value of one key, if that value is an array.

At the same time get suddenly can't be used to get an arbitrary value anymore, you have to know that the value is a string. And in a third party lib, you'll need some compat wrapper if you want to support 4.4 and 5.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants