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] handle nested options#27251
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
linaori commentedMay 15, 2018
This looks very interesting! |
Doctrs commentedMay 19, 2018
Deprecations: no |
Doctrs commentedMay 19, 2018
My work finished
Need review ;) |
nicolas-grekas left a comment
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.
Thanks for working on this.
What's the plan compared to#27291?
| 4.2.0 | ||
| ----- | ||
| * added`OptionResolverNested` and support for nesting options |
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.
missing blank line before
| private$nested =array(); | ||
| /** | ||
| * A list of parent name. Use in exceptions. |
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.
A list of parent names. Used in exceptions.
| private$nestedLevelsName =array(); | ||
| /** | ||
| * A list of resoilve data. Needs for root access from children. |
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.
A list of resolved data. Needed for root access from children.
| * | ||
| * @throws AccessException If called from a lazy option or normalizer | ||
| * @throws AccessException If called from a lazy option or normalizer | ||
| * @throws \ReflectionException |
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.
should be removed (not for this PR at least, unrelated to what it does)
same below
| } | ||
| // If an options is array that shoul be created | ||
| // new nested OptionResolver |
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 understand what this means. Shouldn't we just remove the comment?
| * | ||
| * @param array $data | ||
| * | ||
| * @return array |
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.
should be replaced by real return type
| * | ||
| * @throws \ReflectionException | ||
| */ | ||
| privatefunctionrezolveParentAccess(array$data) |
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.
resolve
| * | ||
| * @return array | ||
| * | ||
| * @throws \ReflectionException |
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 about this
| /** | ||
| * @param array $nestedLevelsName | ||
| * | ||
| * @return self |
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.
docblock to be removed (we don't as them when they don't add anything over the signature)
| /** | ||
| * @param null $parent | ||
| * | ||
| * @return self |
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.
same
Doctrs commentedJun 19, 2018
@nicolas-grekas I don't know 😃 @yceruto solved that problem in simple way and doesn't modify core of that component. My code is more complicated. I modified core. But I think it's easier to understand for developers using Symfony. Compare@yceruto code And mine We solved one problem in different ways. And you have to make a decision. And sorry for my english 😟 |
fabpot commentedOct 10, 2018
After reviewing both PRs, I prefer to merge the other one (#27291), so closing this one. Thanks for working on this. |
…finition (yceruto)This PR was merged into the 4.2-dev branch.Discussion----------[OptionsResolver] Added support for nesting options definition| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#4833| License | MIT| Doc PR |symfony/symfony-docs#9995I'd like to propose an alternative to#27251 and#18134 with a different approach.It would allow you to create a nested options system with required options, validation (type, value),normalization and more.<details> <summary><strong>Short documentation</strong></summary>**To define a nested option, you can pass a closure as the default value of the option with an `OptionsResolver` argument:**```php$resolver ->defaults([ 'connection' => 'default', 'database' => function (OptionsResolver $resolver) { $resolver ->setRequired(['dbname', 'host', ...]) ->setDefaults([ 'driver' => 'pdo_sqlite', 'port' => function (Options $options) { return 'pdo_mysql' === $options['driver'] ? 3306 : null, }, 'logging' => true, ]) ->setAllowedValues('driver', ['pdo_sqlite', 'pdo_mysql']) ->setAllowedTypes('port', 'int') ->setAllowedTypes('logging', 'bool') // ... }, ]);$resolver->resolve(array( 'database' => array( 'dbname' => 'demo', 'host' => 'localhost', 'driver' => 'pdo_mysql', ),));// returns: array(// 'connection' => 'default',// 'database' => array(// 'dbname' => 'demo',// 'host' => 'localhost',// 'driver' => 'pdo_mysql',// 'port' => 3306,// 'logging' => true,// ),//)```Based on this instance, you can define the options under ``database`` and its desired defaultvalue.**If the default value of a child option depend on another option defined in parent level,adds a second ``Options`` argument to the closure:**```php$resolver ->defaults([ 'profiling' => false, 'database' => function (OptionsResolver $resolver, Options $parent) { $resolver ->setDefault('logging', $parent['profiling']) ->setAllowedTypes('logging', 'bool'); }, ]);```**Access to nested options from lazy or normalize functions in parent level:**```php$resolver ->defaults([ 'version' => function (Options $options) { return $options['database']['server_version']; }, 'database' => function (OptionsResolver $resolver) { $resolver ->setDefault('server_version', 3.15) ->setAllowedTypes('server_version', 'numeric') // ... }, ]);```As condition, for nested options you must to pass an array of values to resolve it on runtime, otherwise an exception will be thrown:```php$resolver->resolve(); // OK$resolver->resolve(['database' => []]); // OK$resolver->resolve(['database' => null); // KO (Exception!)```</details>---Demo apphttps://github.com/yceruto/nested-optionsresolver-demoCommits-------d04e40b Added support for nested options definition
Uh oh!
There was an error while loading.Please reload this page.
Summary
Handle nested options resolving
Work with all methods
If called
get%ANY%Optionswe get an array of values are string or arraysKeys with array values are not defined, because they are not defined as value. They are defined as nested OptionResolver
All other methods also work with options as an array.
when we need any value in lazy load, we can use
$option['mysql']['host'][....]for access to children variables andfunction(ResolveData $root)for access to parent data. Array $root contains all resolve data. This step is performed lastWork in progress
Sorry for my English ;)