Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[OptionsResolver] Allow setting same normalizer to multiple option #18254

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

Conversation

unexge
Copy link

@unexge unexge commented Mar 21, 2016

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

@HeahDude
Copy link
Contributor

I've done the same in #18134 to easily normalize prototype options, but haven't pushed it yet (there is more to come).

#18134 is just a proof of concept for now, so I guess it's good to have this one as a stand-alone too ;)

@backbone87
Copy link
Contributor

whats the problem of calling setNormalizer multiple times? calling setNormalizer('a', $f) makes it perfectly clear that $f normalizes all values of option a, but calling setNormalizer([ 'a', 'b' ], $f) isnt as clear: does $f operate on each single option a and b or does it normalize the combination of both [ 'a', 'b' ]?

@HeahDude
Copy link
Contributor

@backbone87 actually setDefined and setRequired works that way.

@backbone87
Copy link
Contributor

@HeahDude These methods have no additional arguments, so its less confusing. But the whole interface shifted more towards a "one call per option" in the recent versions, but the "array set" possibilities didnt got removed (BC). I dont think its a good idea to soften or reverse this path now.

@HeahDude
Copy link
Contributor

@backbone87 I agree. however for handling prototypes like in #18134 it can make sense, my version to come is a new method setNormalizerForAll

@unexge
Copy link
Author

unexge commented Mar 22, 2016

@backbone87 nothing wrong with calling setNormalizer multiple times, this is just syntactic sugar 😄.

i didn't know the whole interface shifted to "one call per option".

closing this PR in favor of interface reliability.

@unexge unexge closed this Mar 22, 2016
@backbone87
Copy link
Contributor

i didn't know the whole interface shifted to "one call per option".

at least that is my interpretation of recent changes, you dont need to close this PR and wait for feedback from some symfony members or @webmozart in this particular case

@webmozart
Copy link
Contributor

I agree with the decision to close this ticket. I don't think this kind of syntactic sugar is needed. :)

@unexge unexge deleted the allow-setting-normalizer-to-multiple-option branch March 30, 2016 17:03
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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.