[OptionsResolver] Added a light-weight, low-level API for basic option resolving #11716

webmozart · 2014-08-20T10:42:07Z

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#4159

See the 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 Options class, which makes the code concise and easy to read and understand:

use 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:

$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:

$options = $resolver->resolve($options);
$options = Options::resolve($options, $resolver);

stof · 2014-08-20T11:14:45Z

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 · 2014-08-20T11:30:36Z

this would require finding a good name for the class holding the static API (Options is indeed a nice one. It is too sad we already use it for the container)

webmozart · 2014-08-20T11:37:43Z

@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.

shoomyth · 2014-08-20T11:47:32Z

src/Symfony/Component/OptionsResolver/Options.php

+    public static function validateValues(array $options, array $allowedValues)
+    {
+        foreach ($allowedValues as $option => $optionValues) {
+            if (isset($options[$option])) {


array_key_exists() ?

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+)

Yes, good catch!

webmozart · 2014-08-20T11:54:13Z

An alternative to the required() method could be to add a constant Options::REQUIRED holding a UUID which can be passed to resolve(), sparing one additional method call:

$options = Options::resolve($options, array(
    'format' => Options::REQUIRED,
    'calendar' => \IntlDateFormatter::GREGORIAN,
));

stof · 2014-08-20T12:25:15Z

@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)

webmozart · 2014-08-20T15:27:48Z

I renamed required() to validateRequired() now. It's not the most beautiful name, but at least it's in line with the other validate*() methods.

webmozart · 2014-08-21T11:06:47Z

@shoomyth The reason is that I don't think that we actually need OptionsResolver and OptionsResolverInterface (this could be removed in Symfony 3.0). Every additional class loaded/created for this low-level utility functionality is a waste of resources. In the updated README you'll see that that class isn't mentioned anymore.

stof · 2014-08-21T12:42:19Z

@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:

the doc team works on improving these explanations (while they don't come looking at readme files of components as most of them just link to the online doc) and does a great job at making things easy to understand
people are used to looking at the symfony.com website to get the doc (all other components have their doc there)
given that the OptionsResolver already has a doc in symfony-docs, it duplicates the maintenance work if you have it in the README as well
(minor) the explanations get translated into other languages for maintained translations (not a killer advantage given tht maintaining translations is very hard though, and Fabien even thought about removing them from the website)

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 · 2014-08-21T12:44:57Z

@stof You are right. We can port the README over to symfony-docs when we're done here.

webmozart · 2014-08-21T17:46:38Z

I added a documentation PR now: symfony/symfony-docs#4159 (rendered documentation). Feedback welcome!

webmozart · 2014-08-29T09:24:58Z

ping @symfony/deciders

webmozart · 2014-09-12T10:08:26Z

ping @symfony/deciders

stof · 2014-09-12T10:16:41Z

👍 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 ?

webmozart · 2014-09-12T10:24:51Z

@stof Thanks! I forgot that after submitting the docs PR.

…n resolving

fabpot · 2014-09-12T12:49:32Z

I've just read the new documentation. It's a nice addition but I'm wondering if the new OptionsConfig is not enough. Using the static methods makes the code a bit shorter, but not that much.

webmozart · 2014-09-12T19:37:40Z

@fabpot In many cases the creation of an OptionsConfig object is not necessary. Using just the static methods performs much better. That means that we can use the component in places now where OptionsResolver would have been too slow.

fabpot · 2014-09-14T18:05:59Z

@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 · 2014-09-23T10:46:14Z

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#4159 See [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: ```php use 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

…vel API for basic option resolving (webmozart)" This reverts commit aa59445, reversing changes made to cc63edb.

…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 methods fab825d Merge pull request #1 from peterrehm/patch-9 f202fb8 Removed duplicate use statements ae66893 Added missing mailer class and use statements 43ae1d8 [OptionsResolver] Adjusted the OptionsResolver documentation to describe the 2.6 API

webmozart mentioned this pull request Aug 20, 2014

[OptionsResolver] Support simple option validation #11705

Closed

webmozart added Feature labels Aug 20, 2014

shoomyth reviewed Aug 20, 2014
View reviewed changes

webmozart force-pushed the issue11705 branch 3 times, most recently from 869ebc0 to c50f4c8 Compare August 20, 2014 15:26

webmozart force-pushed the issue11705 branch 17 times, most recently from 51797e7 to f8a9be3 Compare August 20, 2014 17:37

webmozart force-pushed the issue11705 branch from f8a9be3 to cb9a33c Compare August 21, 2014 17:03

webmozart mentioned this pull request Aug 21, 2014

[WCM][OptionsResolver] Adjusted the OptionsResolver documentation to describe the 2.6 API symfony/symfony-docs#4159

Merged

webmozart force-pushed the issue11705 branch 2 times, most recently from b642511 to 02c0c44 Compare September 12, 2014 10:24

[OptionsResolver] Added a light-weight, low-level API for basic optio…

9066025

…n resolving

webmozart force-pushed the issue11705 branch from 02c0c44 to 9066025 Compare September 12, 2014 10:25

This was referenced Sep 15, 2014

[OptionsResolver] performing validation before filters #4500

Closed

[POC] [OptionsResolver] Support nested options #11509

Closed

Added mailer class and use statements webmozart/symfony-docs#1

Merged

fabpot merged commit 9066025 into symfony:master Sep 23, 2014

stof mentioned this pull request Sep 25, 2014

[OptionsResolver] Exception doesn't report the actual type used #11021

Closed

webmozart mentioned this pull request Oct 20, 2014

[OptionsResolver] Merged Options class into OptionsResolver #12156

Merged

4 tasks

webmozart added a commit that referenced this pull request Oct 21, 2014

Revert "feature #11716 [OptionsResolver] Added a light-weight, low-le…

453882c

…vel API for basic option resolving (webmozart)" This reverts commit aa59445, reversing changes made to cc63edb.

webmozart deleted the issue11705 branch October 22, 2014 17:20

weaverryan mentioned this pull request Dec 4, 2014

[OptionResolver] (Default) null values are not allowed since 2.6 #12809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[OptionsResolver] Added a light-weight, low-level API for basic option resolving #11716

[OptionsResolver] Added a light-weight, low-level API for basic option resolving #11716

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[OptionsResolver] Added a light-weight, low-level API for basic option resolving #11716

[OptionsResolver] Added a light-weight, low-level API for basic option resolving #11716

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!