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

Add type aliases for allowed types in OptionsResolver #12602

Closed
wants to merge 2 commits into from
Closed

Add type aliases for allowed types in OptionsResolver #12602

wants to merge 2 commits into from

Conversation

henrikbjorn
Copy link
Contributor

Q A
Bug fix? [yes]
New feature? [no]
BC breaks? [no]
Deprecations? [no]
Tests pass? [yes]
Fixed tickets [#12586]
License MIT

Improves DX by allowing type names that does not correspond 1 to 1
with php is_ functions.

integer => int
boolean => bool
double => float

(have one for 2.6 branch aswell, but cant never remember what branch i am supposed to base stuff off)

Improves DX by allowing type names that does not correspond 1 to 1
with php is_ functions.

integer => int
boolean => bool
double => float
@fabpot
Copy link
Member

fabpot commented Nov 29, 2014

As this is a bug fix, 2.3 is the correct branch. Instead of creating a test file just for this bug, I would include the test in the current ones.

@henrikbjorn
Copy link
Contributor Author

Moved the test into the other test case and changed the name a bit.

@fabpot should be ready now.

@henrikbjorn
Copy link
Contributor Author

ping @fabpot

@timglabisch
Copy link

a testcase for "double" is missing?

@henrikbjorn
Copy link
Contributor Author

@timglabisch well it tests that it uses the alias map, not sure we need one for every entry in the alias map.

@timglabisch
Copy link

@henrikbjorn adding such a testcase would prevent me from createing a pull request that removes the double entry. your choice :)

@nicolas-grekas
Copy link
Member

Can't the pb be fixed at the error message level? I fear adding this might just add confusion.

@timglabisch
Copy link

👍 for error message level

@xabbuh
Copy link
Member

xabbuh commented Nov 29, 2014

As far as I understand the issue it's not about a badly worded error message, but it's a feature that doesn't work at the moment.

You define your option to be of type boolean. But when you then turn in the type check, the component fails to validate the value since there is no is_boolean() method, but only the is_book() method does exist.

@Tobion
Copy link
Contributor

Tobion commented Nov 29, 2014

This is not a bug fix but a new feature.

@eXtreme
Copy link
Contributor

eXtreme commented Nov 30, 2014

In the original issue #12586 I proposed 3 solutions, that's why it wasn't me making a PR. :) But yea, making aliases sounds like a new feature

@stof
Copy link
Member

stof commented Jan 3, 2015

👍 for this. PHP has these aliases when casting, so it makes sense to support them when validating the type.

@fabpot
Copy link
Member

fabpot commented Jan 3, 2015

Thank you @henrikbjorn.

@fabpot fabpot closed this in a0e3757 Jan 3, 2015
@fabpot
Copy link
Member

fabpot commented Jan 3, 2015

I've adapted the patch for 2.7 as the component is much different.

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.

8 participants