Skip to content

[HttpFoundation] Update all method to handle default value #38891

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

Korbeil
Copy link
Contributor

@Korbeil Korbeil commented Oct 30, 2020

Q A
Branch? 5.x
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR none

I added a quick getArray method for better DX after #34363 changes.
I know we can use all($key) (and #37229 did almost same addition) with the same purpose but here this method will be here for its default value mostly which is not handled by all($key) method.
I think it's better DX to do getArray($key, $default) rather than all($key) ?? $default

I updated all() method to handle a $default variable as second parameter as following: all(string $key = null, array $default = []).

@Korbeil
Copy link
Contributor Author

Korbeil commented Oct 30, 2020

Indeed, the other way to fix my DX issue here would be to add a second parameter to all() method to manage default values. I can do it aswell, tell me which way you prefer.

@jderusse jderusse added this to the 5.x milestone Oct 30, 2020
@fabpot
Copy link
Member

fabpot commented Nov 1, 2020

I'd prefer to avoid adding a new method.

@Korbeil
Copy link
Contributor Author

Korbeil commented Nov 1, 2020

@fabpot I updated all method to handle default value if provided.

@Korbeil Korbeil changed the title [HttpFoundation] Add getArray method to ParameterBag [HttpFoundation] Update all method to handle default value Nov 1, 2020
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 2, 2020

Honestly, I don't see why we should add an API for this when there is already a language construct for it...

all($key, $default)
vs
all($key) ?: $default

better learn the language than a specific API, this makes it easier for everyone, readers and writters.

@Korbeil
Copy link
Contributor Author

Korbeil commented Nov 2, 2020

Honestly, I don't see why we should add an API for this when there is already a language construct for it...

all($key, $default)
vs
all($key) ?: $default

better learn the language than a specific API, this makes it easier for everyone, readers and writters.

@nicolas-grekas It's more about consistency than learn a language, all getters in ParameterBag has a default parameter to handle this behavior that's why I think it's better to keep it the same way there.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 3, 2020

It's more about consistency than learn a language, all getters in ParameterBag has a default parameter to handle this behavior that's why I think it's better to keep it the same way there.

mmm, I'm not convinced. All the other methods return single items. all() returns a collection of items. There's no need for consistency here, these are two different beasts.

BTW, the $default arguments were added before the ?: and ?? operators existed. Not sure we would add them if we were to design this again from scratch now (I'm not suggesting deprecating them either - the cost would be too high for too little gain.)

@fabpot
Copy link
Member

fabpot commented Nov 4, 2020

I tend to agree with @nicolas-grekas arguments. Let's close. Thanks for the discussion.

@fabpot fabpot closed this Nov 4, 2020
@Korbeil Korbeil deleted the feature/get-array branch November 13, 2020 09:52
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.

6 participants