-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1efc945
to
4e29c4e
Compare
What about access to children data or parent data in lazy options? |
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
to
07f4719
Compare
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
to
480fb66
Compare
Status: Needs Review |
d76b002
to
937a1cd
Compare
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.
937a1cd
to
d04e40b
Compare
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. |
Some research in Even fixing bugs explained by #4833 |
Hey! You can see now a demo for this feature in https://github.com/yceruto/nested-optionsresolver-demo, just clone it and play with nested options. |
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#9995 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), 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 default value. **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 app https://github.com/yceruto/nested-optionsresolver-demo Commits ------- d04e40b Added support for nested options definition
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 feature Documents symfony/symfony#27291 Commits ------- 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. #27291 Commits ------- 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 | MIT this removes unnecessary limitations that I hadn't considered when introducing nested options feature in #27291. #### 1. Allow defining default values for nested options Imagine you want to define the following nested option in a generic form type: ```php class 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. ```php class 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` instances For 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 options the same problem would occur with a lazy default for a nested definition: ```php class 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()` method this also enables the ability to set default values for prototyped options, which wasn't possible before. For example: ```php class 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
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
OptionsResolver
argument: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: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 app https://github.com/yceruto/nested-optionsresolver-demo