8000 [OptionsResolver] handle nested options by Doctrs · Pull Request #27251 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

[OptionsResolver] handle nested options #27251

wants to merge 6 commits into from

Conversation

Doctrs
Copy link
Contributor
@Doctrs Doctrs commented May 13, 2018 •
edited
Loading
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #4833, #15524
License MIT
Doc PR symfony/symfony-docs#9819

Summary

Handle nested options resolving

    $configuration = [
        'host' => '127.0.0.1',
        'mysql' => [
            'option' => [
                'first_option' => false,
            ],
        ],
    ];

    $resolver->setDefaults([
        'host' => 'localhost',
        'user' => function(Options $option){
            return $option['mysql']['host'] === 'mysql_host';
        },
        'port' => 1234,
        'mysql' => new OptionResolverNested([
            'host' => 'mysql_host',
            'port' => 5678,
            'option' => new OptionResolverNested([
                'first_option' => true,
                'second_option' => function(ResolveData $root){
                    return $root['port'] === 1234;
                },
            ]),
        ]),
    ]);

Work with all methods

    $resolver->setRequired([
        ['mysql', 'ports'], // mysql => ports
        ['mysql', 'option', 'first_option'], // mysql => option => first_option
        'host' // host in root
    ]);

    $resolver->isRequired(['mysql', 'ports']); // true
    $resolver->isRequired(['mysql', 'ports', 'any_non_required_option']); // false
    $resolver->isMissing(['mysql', 'ports']); // true

If called get%ANY%Options we get an array of values are string or arrays

    $resolver->getRequiredOptions();

    // OUTPUT

    array (size=3)
      0 => 
        array (size=3)
          0 => string 'mysql' (length=5)
          1 => string 'option' (length=6)
          2 => string 'first_option' (length=12)
      1 => 
        array (size=2)
          0 => string 'mysql' (length=5)
          1 => string 'ports' (length=5)
      2 => string 'host' (length=4)

Keys with array values are not defined, because they are not defined as value. They are defined as nested OptionResolver

    $resolver->setDefined([
        ['mysql', 'any_data']
    ]);
    var_dump($resolver->isDefined('host')); // true
    var_dump($resolver->isDefined('mysql')); // false
    var_dump($resolver->isDefined(['mysql', 'any_data'])); // true

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 and function(ResolveData $root) for access to parent data. Array $root contains all resolve data. This step is performed last

Work in progress

  • Fix lazy load. Sometimes it works unexpectedly
  • Fix allowValues and allowTypes. When variables are redefined it try to compare with old and new values.
  • Tests
  • Code style

Sorry for my English ;)

@nicolas-grekas nicolas-grekas added this to the next milestone May 14, 2018
@linaori
Copy link
Contributor
linaori commented May 15, 2018

This looks very interesting!

@yceruto yceruto mentioned this pull request May 17, 2018
Merged
@Doctrs
Copy link
Contributor Author
Doctrs commented May 19, 2018

Deprecations: no

@Doctrs Doctrs changed the title [WIP] [OptionsResolver] handle nested options [OptionsResolver] handle nested options May 19, 2018
@Doctrs
Copy link
Contributor Author
Doctrs commented May 19, 2018

My work finished
Last changes

  • To access parent options you should use function(ResolveData $root). Array $root contains all resolve data. This step is performed last
  • offsetGet no longer accepts string[][]. To get access to children options use array access ($data['var']['any_var'])
  • Tests coverage 100%

Need review ;)

@Doctrs Doctrs mentioned this pull request May 25, 2018
Closed
nicolas-grekas
nicolas-grekas reviewed Jun 19, 2018
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a 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?

src/Symfony/Component/OptionsResolver/CHANGELOG.md Outdated
@@ -1,6 +1,10 @@
CHANGELOG
=========

4.2.0
-----
* added `OptionResolverNested` and support for nesting options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing blank line before

src/Symfony/Component/OptionsResolver/OptionsResolver.php Outdated
private $nested = array();

/**
* A list of parent name. Use in exceptions.
Copy link
Member

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.

src/Symfony/Component/OptionsResolver/OptionsResolver.php Outdated
private $nestedLevelsName = array();

/**
* A list of resoilve data. Needs for root access from children.
Copy link
Member

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.

src/Symfony/Component/OptionsResolver/OptionsResolver.php Outdated
@@ -129,7 +151,8 @@ class OptionsResolver implements Options
*
* @return $this
*
* @throws AccessException If called from a lazy option or normalizer
* @throws AccessException If called from a lazy option or normalizer
* @throws \ReflectionException
Copy link
Member

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

src/Symfony/Component/OptionsResolver/OptionsResolver.php Outdated
@@ -140,6 +163,27 @@ public function setDefault($option, $value)
throw new AccessException('Default values cannot be set from a lazy option or normalizer.');
}

// If an options is array that shoul be created
// new nested OptionResolver
Copy link
Member

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?

src/Symfony/Component/OptionsResolver/OptionsResolver.php Outdated
*
* @param array $data
*
* @return array
Copy link
Member

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

src/Symfony/Component/OptionsResolver/OptionsResolver.php Outdated
*
* @throws \ReflectionException
*/
private function rezolveParentAccess(array $data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolve

src/Symfony/Component/OptionsResolver/OptionsResolver.php Outdated
*
* @return array
*
* @throws \ReflectionException
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about this

src/Symfony/Component/OptionsResolver/OptionsResolver.php Outdated
/**
* @param array $nestedLevelsName
*
* @return self
Copy link
Member

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)

src/Symfony/Component/OptionsResolver/OptionsResolver.php Outdated
/**
* @param null $parent
*
* @return self
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@Doctrs
Copy link
Contributor Author
Doctrs commented Jun 19, 2018

Thanks for working on this.
What's the plan compared to #27291?

@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

$resolver->setDefaults([
        'database' => function (OptionsResolver $extra) {
            $extra->setDefaults([
                'mysql' => function (OptionsResolver $extra1) {
                    $extra1->setDefaults([
                        'host' => 'localhost',
                    ]);
                }
            ]);
        }
    ]);

And mine

$resolver->setDefaults([
    'database' => new OptionResolverNested([
        'mysql' => new OptionResolverNested([
            'host' => 'localhost',
        ]),
    ]),
]);

We solved one problem in different ways. And you have to make a decision.

And sorry for my english 😟

@fabpot
Copy link
Member
fabpot commented Oct 10, 2018

After reviewing both PRs, I prefer to merge the other one (#27291), so closing this one. Thanks for working on this.

@fabpot fabpot closed this Oct 10, 2018
fabpot added a commit that referenced this pull request Oct 10, 2018
@fabpot
feature #27291 [OptionsResolver] Added support for nesting options de…
d3fac86 
…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
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

Assignees
No one assigned
Labels
Feature OptionsResolver Status: Needs Review
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
6 participants
@Doctrs @linaori @fabpot @nicolas-grekas @ogizanagi @carsonbot
0