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] Added a light-weight, low-level API for basic option resolving#11716
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
stof commentedAug 20, 2014
IMO, the static API of the component should not live in the same class than the container of options being resolved. These are 2 different responsibilities which are better left to separate classes |
stof commentedAug 20, 2014
this would require finding a good name for the class holding the static API ( |
webmozart commentedAug 20, 2014
@stof I thought so too in the beginning. But then again, it doesn't really matter where we put some static methods which are independent of the surrounding class. |
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.
array_key_exists() ?
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.
this is the same than the existing code. It is just moving around.
but this indeed looks like a bug. I suggest sending a separate PR fixing it in 2.3 (while this code will go only in 2.6+)
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, good catch!
webmozart commentedAug 20, 2014
An alternative to the $options = Options::resolve($options,array('format' => Options::REQUIRED,'calendar' => \IntlDateFormatter::GREGORIAN,)); |
stof commentedAug 20, 2014
@webmozart but this would be confusing IMO, because it would give 2 different meanings to the content of the array (and would forbid one value as default) |
869ebc0 toc50f4c8Comparewebmozart commentedAug 20, 2014
I renamed |
51797e7 tof8a9be3Comparewebmozart commentedAug 21, 2014
@shoomyth The reason is that I don't think that we actuallyneed |
stof commentedAug 21, 2014
@webmozart all the documentation you added in the README belongs to symfony-docs IMO rather than the README for consistency with other components (I know this component is already inconsistent before this PR). The advantages of having it in the doc repo are:
Regarding the code changes, I think it makes sense to decouple the OptionsConfig from the class doing the resolution itself. For instance, form types are currently getting an OptionsResolver instance, but they are not expected to ever resolve options. |
webmozart commentedAug 21, 2014
@stof You are right. We can port the README over to symfony-docs when we're done here. |
webmozart commentedAug 21, 2014
I added a documentation PR now:symfony/symfony-docs#4159 (rendered documentation). Feedback welcome! |
webmozart commentedAug 29, 2014
ping @symfony/deciders |
1 similar comment
webmozart commentedSep 12, 2014
ping @symfony/deciders |
stof commentedSep 12, 2014
👍 for this, but I would like to simplify the README to avoid duplicating the documentation in it (see my previous comment for the arguments). @webmozart could you rebase your branch to fix conflicts ? |
b642511 to02c0c44Comparewebmozart commentedSep 12, 2014
@stof Thanks! I forgot that after submitting the docs PR. |
fabpot commentedSep 12, 2014
I've just read the new documentation. It's a nice addition but I'm wondering if the new |
webmozart commentedSep 12, 2014
@fabpot In many cases the creation of an |
fabpot commentedSep 14, 2014
@webmozart I'm not talking about the implementation but about the user point of view. There is no other place in Symfony with such static methods. |
fabpot commentedSep 23, 2014
Thank you@webmozart. |
…for basic option resolving (webmozart)This PR was merged into the 2.6-dev branch.Discussion----------[OptionsResolver] Added a light-weight, low-level API for basic option resolving| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#11705| License | MIT| Doc PR |symfony/symfony-docs#4159See [the updated documentation](https://github.com/webmozart/symfony-docs/blob/issue11705/components/options_resolver.rst) for details on the usage of the simple API.The most important motivation for this change is DX and speed. The basic features of the component should be easily usable in a wide variety of use cases without impacting performance.For DX reasons, I added the static methods to the `Options` class, which makes the code concise and easy to read and understand:```phpuse Symfony\Component\OptionsResolver\Options;$options = Options::validateRequired($options, 'format');$options = Options::validateTypes($options, array( 'format' => array('string', 'int'), 'calendar' => 'int',));$options = Options::validateValues($options, array( 'calendar' => array( \IntlDateFormatter::GREGORIAN, \IntlDateFormatter::TRADITIONAL, ),));$options = Options::resolve($options, array( 'format' => null, 'calendar' => \IntlDateFormatter::GREGORIAN,));```If you need to distribute the option configuration, this PR also extracts the configuration part of the `OptionsResolver` class into a new class `OptionsConfig`, which can be passed around. When the configuration is complete, pass the config object to `Options::resolve()` as second argument:```php$config = new OptionsConfig();$config->setDefaults(array( 'format' => \IntlDateFormatter::MEDIUM, 'calendar' => \IntlDateFormatter::GREGORIAN,));$options = Options::resolve($options, $config);```Consequently - since `OptionsResolver` extends `OptionsConfig` - the two following statements now become identical:```php$options = $resolver->resolve($options);$options = Options::resolve($options, $resolver);```Commits-------9066025 [OptionsResolver] Added a light-weight, low-level API for basic option resolving
…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
…umentation to describe the 2.6 API (webmozart, peterrehm)This PR was merged into the master branch.Discussion----------[WCM][OptionsResolver] Adjusted the OptionsResolver documentation to describe the 2.6 API| Q | A| ------------- | ---| Doc fix? | no| New docs? | yes (symfony/symfony#11716,symfony/symfony#12156)| Applies to | 2.6+| Fixed tickets | -Commits-------575c320 Updated OptionsResolver documentation: removed static methodsfab825d Merge pull request#1 from peterrehm/patch-9f202fb8 Removed duplicate use statementsae66893 Added missing mailer class and use statements43ae1d8 [OptionsResolver] Adjusted the OptionsResolver documentation to describe the 2.6 API
Seethe updated documentation for details on the usage of the simple API.
The most important motivation for this change is DX and speed. The basic features of the component should be easily usable in a wide variety of use cases without impacting performance.
For DX reasons, I added the static methods to the
Optionsclass, which makes the code concise and easy to read and understand:If you need to distribute the option configuration, this PR also extracts the configuration part of the
OptionsResolverclass into a new classOptionsConfig, which can be passed around. When the configuration is complete, pass the config object toOptions::resolve()as second argument:Consequently - since
OptionsResolverextendsOptionsConfig- the two following statements now become identical: