Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:masterfromyceruto:nested_options_resolver
Oct 10, 2018

Conversation

@yceruto
Copy link
Member

@ycerutoyceruto commentedMay 17, 2018
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#4833
LicenseMIT
Doc PRsymfony/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) and
normalization.

Short documentation

To define a nested option, you can pass a closure as the default value of the option with anOptionsResolver argument:

$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 underdatabase and its desired default
value.

If the default value of a child option depend on another option defined in parent level,
adds a secondOptions argument to the closure:

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

$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 pass an array of values to resolve it at runtime, otherwise an exception will be thrown:

$resolver->resolve();// OK$resolver->resolve(['database' => []]);// OK$resolver->resolve(['database' =>null);// KO (Exception!)

Demo apphttps://github.com/yceruto/nested-optionsresolver-demo

Koc, yceruto, evelync23, Gummibeer, stephanvierkant, beoboo, gmponos, martiis, NicoBoos, vudaltsov, and 3 more reacted with heart emoji
@ycerutoycerutoforce-pushed thenested_options_resolver branch 5 times, most recently from1efc945 to4e29c4eCompareMay 18, 2018 12:14
@nicolas-grekasnicolas-grekas added this to thenext milestoneMay 18, 2018
@Doctrs
Copy link
Contributor

What about access to children data or parent data in lazy options?
And as I see we cant add normalize or set required for nested options after defined main data

@yceruto
Copy link
MemberAuthor

yceruto commentedMay 19, 2018
edited
Loading

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

@ycerutoycerutoforce-pushed thenested_options_resolver branch 4 times, most recently from97bc7e4 to07f4719CompareMay 19, 2018 04:04
@yceruto
Copy link
MemberAuthor

yceruto commentedMay 19, 2018
edited
Loading

The strategy has changed a bit, now instead of passing an instance ofOptionsResolver as default value, you can pass a closure withOptionsResolver argument, thus subdefinitions can alter the previous definition and you can access to parent resolved options.

What about access to ... parent data in lazy options?

Done! 🎉 don't directly in lazy option but explicit in nested configuration.

And as I see we cant add normalize or set required for nested options after defined main data

Done as well 🎉

Description updated.

@yceruto
Copy link
MemberAuthor

Status: Needs Review

@ycerutoycerutoforce-pushed thenested_options_resolver branch fromd76b002 to937a1cdCompareJuly 12, 2018 16:16
Copy link

@beoboobeoboo left a 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
Copy link
MemberAuthor

@beoboo thanks for trying it. I hope this takes enough attention to merge into 4.2. Also, there is another PR proposing the same feature (#27251), so the members/deciders should pick one of the approaches.

Cheers!

@yceruto
Copy link
MemberAuthor

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
Copy link
MemberAuthor

Some research inSonataAdminBundle where this feature can be applied:

Even fixing bugs explained by#4833

@yceruto
Copy link
MemberAuthor

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.

@fabpot
Copy link
Member

Thank you@yceruto.

yceruto reacted with hooray emojiyceruto reacted with heart emoji

@fabpotfabpot merged commitd04e40b intosymfony:masterOct 10, 2018
fabpot added a commit that referenced this pull requestOct 10, 2018
…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
@ycerutoyceruto deleted the nested_options_resolver branchOctober 10, 2018 13:58
@yceruto
Copy link
MemberAuthor

I'm very happy to see this merged for 4.2! 🎉

beoboo, majermi4, and phansys reacted with hooray emoji

fabpot added a commit that referenced this pull requestOct 12, 2018
…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
@nicolas-grekasnicolas-grekas modified the milestones:next,4.2Nov 1, 2018
This was referencedNov 3, 2018
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestDec 14, 2018
…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
fabpot added a commit that referenced this pull requestJan 2, 2019
…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
fabpot added a commit that referenced this pull requestFeb 26, 2025
…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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

+1 more reviewer

@beoboobeoboobeoboo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

6 participants

@yceruto@Doctrs@nicolas-grekas@fabpot@beoboo@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp