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] ParameterBag::getEnum() #48820

Merged
merged 1 commit into from Jan 5, 2023

Conversation

nikophil
Copy link
Contributor

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR todo if the PR gets accepted

Adds availability to get an enum directly from a parameter bag:

$bag->getEnum('key', Foo::class)

return $default;
}

return $class::tryFrom($value) ?? $default;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering if its a good idea to return the default in case an invalid value (not a valid backend enum value) is passed? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think get() behaves the same, but in what case a default should be returned then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better keep it consistent with the rest of the API I think.

Copy link
Member

@GromNaN GromNaN Jan 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With get, there is no invalid value.

In #48525 there is the same dilemma and the trend is to throw an exception when the value is invalid. This is particularly useful when the value comes from the query string, a BadRequest response is sent when the value is unexpected.

The default is returned when the parameter is not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think to add the same parameter $failOnInvalid?
We could even add annotation psalm-assert to narrow the type

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$failOnInvalid is added for changing the behaviour while keeping backward compatibility. I think it is better to define a behaviour for all instead of adding options.

If you return the default in case of invalid value, you cannot distinguish when the parameter is not set and when the value is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the behavior of the method is fixed now:

  • we throw an \UnexpectedValueException when the value is not part of the backed enum, and not a string or an int (ie: it could be an array)
  • I've overridden the method in InputBag to throw a BadRequestException in that case

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With remaining suggestion applied

@nikophil nikophil force-pushed the feat/http-foundation/get-enum branch 3 times, most recently from 3bb289f to 6b6c7df Compare January 4, 2023 08:15
@nikophil nikophil requested review from GromNaN and OskarStark and removed request for GromNaN January 4, 2023 08:19
@nikophil nikophil requested review from GromNaN and dmaicher and removed request for OskarStark and GromNaN January 4, 2023 08:19
@nikophil nikophil force-pushed the feat/http-foundation/get-enum branch from 6b6c7df to e4ba7b7 Compare January 4, 2023 10:06
@fabpot
Copy link
Member

fabpot commented Jan 5, 2023

Thank you @nikophil.

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.

None yet

8 participants