Skip to content

Add getAny in InputBag #36725

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
Closed

Add getAny in InputBag #36725

wants to merge 1 commit into from

Conversation

BafS
Copy link
Contributor

@BafS BafS commented May 6, 2020

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

The new InputBag was introduced in #34363 but there is an "edge case" to get a value of any type, I find it's better to be explicit and to have a new ->getAny('foo', 'default') instead of ->all()['foo'] ?? 'default (or array_key_exists('foo', $...->all()) ? ...->all()['foo'] : 'default' to be exact).

Alternatively having a ->getString() is more explicit than a generic ->get().

@BafS BafS requested a review from xabbuh as a code owner May 6, 2020 10:58
@nicolas-grekas nicolas-grekas added this to the next milestone May 6, 2020
@Nyholm
Copy link
Member

Nyholm commented May 6, 2020

Thank you for this PR.

Could you elaborate a bit on the use case or the scenario where you need getAny()?

@BafS
Copy link
Contributor Author

BafS commented May 6, 2020

Sure

  1. I currently use ParameterBag for json input (cf. example), having a get to force strings is a good idea so I would be keen to use InputBag but for that I also need to be able to "get any" (for example I have some fields where it can be an array or a value)
  2. I think it would be easier for devs to migrate, ->get() can be replaced with ->getAny()
  3. Having ->get() which returns any type in ParameterBag and ->get() returning string only in InputBag is a bit strange because it creates a discrepancy and Symfony devs are used to have ->get() for anything and ->getInt/Alpha/etc.() for "filters"
    1. Which makes me think, wouldn't it be better to have ->getString() to get string and ->getAny() for anything in ParameterBag ? It would be more explicit

Example in my request:

        if ($this->isJson() && $content = $this->getContent()) {
            $this->request->replace((array) json_decode($content, true));
        }

@nicolas-grekas
Copy link
Member

$this->request->replace((array) json_decode($content, true));

Thanks for the explanation, that helps a lot to get your pov.
Honestly, I'm not convinced this is a legit design for JSON query arguments.
I would recommend putting the decoded JSON into a request attribute instead.

getAny() is reopening the trap we wanted to close when adding InputBag: contrary to what you describe, we started with the pov that 99% of the use case for get() is retrieving strings since that's what a query string contains at the origin.

I'm 👎 on my side with this reasoning.

@Nyholm
Copy link
Member

Nyholm commented May 6, 2020

Thank you for the context.

I like the implementation and the fact that you added a test. 👍 However, it is a good thing that you know the type of the return parameters.

I also thing we should close this PR.

@Nyholm Nyholm closed this May 6, 2020
@BafS
Copy link
Contributor Author

BafS commented May 6, 2020

Okay got it, thanks for your feedbacks !

And what do you think about introducing a getString() in ParameterBag ? I think it would make sense to match getInt/Alpha/etc., to have something more explicit and usable in ParameterBag + InputBag without discrepancies

@nicolas-grekas
Copy link
Member

You might be interested in having a look at #37100 (comment)

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.

4 participants