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 support for nesting options definition#27291
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
1efc945 to4e29c4eCompareDoctrs commentedMay 18, 2018
What about access to children data or parent data in lazy options? |
yceruto commentedMay 19, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Access to children data in lazy options is already covered. For the rest you're right 👍 I'm thinking in another solution to allow that, so Status: Needs Work |
97bc7e4 to07f4719Compareyceruto commentedMay 19, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The strategy has changed a bit, now instead of passing an instance of
Done! 🎉 don't directly in lazy option but explicit in nested configuration.
Done as well 🎉 Description updated. |
bef49ae to480fb66Compareyceruto commentedMay 20, 2018
Status: Needs Review |
d76b002 to937a1cdCompare
beoboo 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.
Hey, I just reviewed your code, and it looks good to me. I also tried it in a testing project, and I find the interface clear enough to be used.
yceruto commentedSep 10, 2018
937a1cd tod04e40bCompareyceruto commentedSep 12, 2018
Just rebased, no changes ;) See LDAP Connection revamping with this feature:https://github.com/yceruto/symfony/compare/nested_options_resolver...yceruto:ldap-revamping?diff=unified I'll expose some project examples soon. |
yceruto commentedSep 13, 2018
Some research in Even fixing bugs explained by#4833 |
yceruto commentedSep 17, 2018
Hey! You can see now a demo for this feature inhttps://github.com/yceruto/nested-optionsresolver-demo, just clone it and play with nested options. |
Uh oh!
There was an error while loading.Please reload this page.
fabpot commentedOct 10, 2018
Thank you@yceruto. |
…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
yceruto commentedOct 10, 2018
I'm very happy to see this merged for 4.2! 🎉 |
…n (yceruto)This PR was merged into the 4.2-dev branch.Discussion----------[LDAP] Revamp LDAP options with nested options definition| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Next move after#27291 :)This will work exactly that before, nothing change regarding behavior, BUT now we've *less code* and *intuitive definition of nested options*.Commits-------a26c284 Revamp LDAP options with nested definition
…ruto)This PR was submitted for the master branch but it was squashed and merged into the 4.2 branch instead (closes#9995).Discussion----------[OptionsResolver] Documenting nested options featureDocumentssymfony/symfony#27291Commits-------701e914 [OptionsResolver] Documenting nested options feature
…ter)This PR was submitted for the master branch but it was merged into the 4.2 branch instead (closes#29744).Discussion----------Replace slave and master by replica and primary| Q | A| ------------- | ---| Branch? | 4.2| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#29735| License | MIT| Doc PR |I targeted 4.2 branch because the words were introduced in it cf.#27291Commits-------f06e6b4 Replace slave and master by replica and primary
…a `setDefault()` use `setOptions()` instead (yceruto)This PR was merged into the 7.3 branch.Discussion----------[OptionsResolver] Deprecate defining nested options via `setDefault()` use `setOptions()` instead| Q | A| ------------- | ---| Branch? | 7.3| Bug fix? | no| New feature? | yes| Deprecations? | yes| Issues | -| License | MITthis removes unnecessary limitations that I hadn't considered when introducing nested options feature in#27291.#### 1. Allow defining default values for nested optionsImagine you want to define the following nested option in a generic form type:```phpclass GenericType extends AbstractType{ public function configureOptions(OptionsResolver $resolver): void { $resolver->setDefault('foo', function (OptionsResolver $foo) { $foo->define('bar')->allowedTypes('int'); }); }}```then, I'd like to define a concrete type with a default value for it.```phpclass ConcreteType extends AbstractType{ public function configureOptions(OptionsResolver $resolver): void { $resolver->setDefault('foo', ['bar' => 23]); } public function getParent(): string { return GenericType::class; }}```this might seem unexpected, but the fact is that the nested definition for `foo` option in `ConcreteType` is gone. As a result, when resolved, the `foo` option will have a default value (`['bar' => 23]`) but without any constraints, allowing end users any value/type to be passed through this option for `ConcreteType` instancesFor example, passing `['foo' => false]` as options for `ConcreteType` would be allowed, instead of requiring an array where `bar` expects an integer value.#### 2. Allow defining lazy default for nested optionsthe same problem would occur with a lazy default for a nested definition:```phpclass ConcreteType extends AbstractType{ public function configureOptions(OptionsResolver $resolver): void { $resolver->setRequired(['baz']) $resolver->setDefault('foo', function (Options $options) { return ['bar' => $options['baz']]; }); } public function getParent(): string { return GenericType::class; }}```the issue here is the same as in the previous example, meaning this new default essentially overrides/removes the original nested definition---the two features mentioned earlier are now supported by introducing a new method `setOptions()`, which separates the nested definition from its default value (whether direct or lazy). Additionally this PR deprecates the practice of defining nested options using `setDefault()` methodthis also enables the ability to set default values for prototyped options, which wasn't possible before. For example:```phpclass NavigatorType extends AbstractType{ public function configureOptions(OptionsResolver $resolver): void { $resolver->define('buttons') ->options(function (OptionsResolver $buttons) { $buttons->setPrototype(true); $buttons->define('name')->required()->allowedTypes('string'); $buttons->define('type')->default(SubmitType::class)->allowedTypes('string'); $buttons->define('options')->default([])->allowedTypes('array'); }) ->default([ 'back' => ['name' => 'back', 'options' => ['validate' => false, 'validation_groups' => false]], 'next' => ['name' => 'next'], 'submit' => ['name' => 'submit'], ]); }}```cheers!Commits-------b0bb9a1 Add setOptions method
Uh oh!
There was an error while loading.Please reload this page.
I'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) and
normalization.
Short documentation
To define a nested option, you can pass a closure as the default value of the option with an
OptionsResolverargument:Based on this instance, you can define the options under
databaseand its desired defaultvalue.
If the default value of a child option depend on another option defined in parent level,
adds a second
Optionsargument to the closure:Access to nested options from lazy or normalize functions in parent level:
As condition, for nested options you must pass an array of values to resolve it at runtime, otherwise an exception will be thrown:
Demo apphttps://github.com/yceruto/nested-optionsresolver-demo