Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[OptionsResolver] Implemented policies for treating unknown/missing options#10616
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Either we need to check all flags for mutual exclusivity, or none. Sincechecking all flags is expensive, I'm removing this check altogether.
iamluc commentedApr 2, 2014
Should be "BC breaks" because the OptionResolverInterface has changed, no ? |
webmozart commentedApr 2, 2014
@iamluc You're right, thanks! Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure this is really needed (just as there is no IGNORE_INVALID) because people can just make all options optional. Btw, setRequired() followed by setOptional() does not seem to override an option to become optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can't really compare these two things. The code that callsresolve() is not necessarily the code that configures the resolver. That was the reason why these flags were needed in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes I know. I'm just saying an alternative approach could be something like the following where getKnownOptions just returns all available options, which is not there currently.
$resolver->setOptional($resolver->getKnownOptions());$resolver->resolve(...)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Tobion Do you think it would be better to add the variousget*Options() (known, required, optional, ..) methods and push this functionality into userland code? The PR currently adds overhead to every code that uses theresolve() method, regardless of whether they need the flags or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure. I think since the resolver defines the semantics like required/optional, it makes sense to also do the validation there. How much influence has the overhead? Or do you mean to also remove the validation that was already present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, I'm just talking about this PR. We need to measure the influence with a moderately big form. Do you have time for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No I don't.
fabpot commentedApr 2, 2014
@webmozart Can you squash your commits? |
wouterj commentedMay 21, 2014
ping@webmozart |
stof commentedMay 21, 2014
@wouterj given we reached the feature freeze 1 month and a half ago, it is too late for 2.5 anyway |
wouterj commentedMay 21, 2014
Hmm, yeah indeed :) Maybe it's better to focus on stabilization now |
fabpot commentedJun 3, 2014
@webmozart Can you finish this one? |
sstok commentedAug 16, 2014
@webmozart ping |
webmozart commentedOct 15, 2014
Replaced by#12156. |
…r (webmozart)This PR was merged into the 2.6-dev branch.Discussion----------[OptionsResolver] Merged Options class into OptionsResolver| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | yes| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#4500,#9174,#10585,#10202,#11020, makes#7979+#10616 obsolete| License | MIT| Doc PR |symfony/symfony-docs#4159#11716 was reverted as preparation of this PR (453882c).The Options class was merged into OptionsResolver. This made it possible to fix several tickets, as indicated above.**Usage**See [the updated documentation](https://github.com/webmozart/symfony-docs/blob/issue11705/components/options_resolver.rst).**Bug Fixes**Previously, the options weren't validated before the normalizers were called. As a result, the normalizers had to perform validation again:```php$resolver->setAllowedTypes(array( 'choices' => 'array',));$resolver->setNormalizers(array( 'choices' => function (Options $options, $value) { array_merge($options['choices'], ...); },));// fatal error$resolver->resolve(array('choices' => 'foobar'));```This is fixed now.**BC Breaks**The `array` type hint was removed from `setRequired()`, `setAllowedValues()`, `addAllowedValues()`, `setAllowedTypes()` and `addAllowedTypes()`. If anybody implemented `OptionsResolverInterface`, they must adapt their code.The Options class was turned into an interface extending ArrayAccess and Countable. Anybody instantiating Options directly should instantiate OptionsResolver instead. Anybody using any of the methods available in Options (`get()`, `has()`) should use the ArrayAccess interface instead.Normalizers are not called anymore for undefined options (#9174). People need to set a default value if they want a normalizer to be executed independent of the options passed to `resolve()`.**Deprecations**OptionsResolverInterface was deprecated and will be removed in 3.0. OptionsResolver instances are not supposed to be shared between classes, hence an interface does not make sense.Several other methods were deprecated. See the CHANGELOG and UPGRADE-2.6 files for information.**Todo**- [x] Fix PHPDoc- [x] Adapt CHANGELOG/UPGRADE- [x] Adapt documentation- [x] Deprecate OptionsResolver[Interface]Commits-------642c119 [OptionsResolver] Merged Options class into OptionsResolver
replacement for#10574 which was closed by accident