Skip to content

[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

Merged
merged 1 commit into from
Oct 10, 2018

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented May 17, 2018

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

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

$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 app https://github.com/yceruto/nested-optionsresolver-demo

@yceruto yceruto force-pushed the nested_options_resolver branch 5 times, most recently from 1efc945 to 4e29c4e Compare May 18, 2018 12:14
@nicolas-grekas nicolas-grekas added this to the next milestone May 18, 2018
@Doctrs
Copy link
Contributor

Doctrs commented May 18, 2018

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
Member Author

yceruto commented May 19, 2018

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

@yceruto yceruto force-pushed the nested_options_resolver branch 4 times, most recently from 97bc7e4 to 07f4719 Compare May 19, 2018 04:04
@yceruto
Copy link
Member Author

yceruto commented May 19, 2018

The strategy has changed a bit, now instead of passing an instance of OptionsResolver as default value, you can pass a closure with OptionsResolver 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
Member Author

yceruto commented May 20, 2018

Status: Needs Review

@yceruto yceruto force-pushed the nested_options_resolver branch from d76b002 to 937a1cd Compare July 12, 2018 16:16
Copy link

@beoboo beoboo 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
Member Author

yceruto commented Sep 10, 2018

@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 yceruto force-pushed the nested_options_resolver branch from 937a1cd to d04e40b Compare September 12, 2018 21:53
@yceruto
Copy link
Member Author

yceruto commented Sep 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
Copy link
Member Author

yceruto commented Sep 13, 2018

Some research in SonataAdminBundle where this feature can be applied:

Even fixing bugs explained by #4833

@yceruto
Copy link
Member Author

yceruto commented Sep 17, 2018

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.

@fabpot
Copy link
Member

fabpot commented Oct 10, 2018

Thank you @yceruto.

@fabpot fabpot merged commit d04e40b into symfony:master Oct 10, 2018
fabpot added a commit that referenced this pull request Oct 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#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
@yceruto yceruto deleted the nested_options_resolver branch October 10, 2018 13:58
@yceruto
Copy link
Member Author

yceruto commented Oct 10, 2018

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

fabpot added a commit that referenced this pull request Oct 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-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
This was referenced Nov 3, 2018
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Dec 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 feature

Documents symfony/symfony#27291

Commits
-------

701e914 [OptionsResolver] Documenting nested options feature
fabpot added a commit that referenced this pull request Jan 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. #27291

Commits
-------

f06e6b4 Replace slave and master by replica and primary
fabpot added a commit that referenced this pull request Feb 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       | 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants