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

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

Closed
Doctrs wants to merge6 commits intosymfony:masterfromDoctrs:master

Conversation

@Doctrs
Copy link
Contributor

@DoctrsDoctrs commentedMay 13, 2018
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#4833,#15524
LicenseMIT
Doc PRsymfony/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 calledget%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 andfunction(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 ;)

linaori, Doctrs, and FaustVIII reacted with thumbs up emoji
@linaori
Copy link
Contributor

This looks very interesting!

@Doctrs
Copy link
ContributorAuthor

Deprecations: no

@DoctrsDoctrs changed the title[WIP] [OptionsResolver] handle nested options[OptionsResolver] handle nested optionsMay 19, 2018
@Doctrs
Copy link
ContributorAuthor

My work finished
Last changes

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

Need review ;)

Copy link
Member

@nicolas-grekasnicolas-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?


4.2.0
-----
* added`OptionResolverNested` and support for nesting options

Choose a reason for hiding this comment

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

missing blank line before

private$nested =array();

/**
* A list of parent name. Use in exceptions.

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.

private$nestedLevelsName =array();

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

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.

*
* @throws AccessException If called from a lazy option or normalizer
* @throws AccessException If called from a lazy option or normalizer
* @throws \ReflectionException

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

}

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

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?

*
* @param array $data
*
* @return array

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

*
* @throws \ReflectionException
*/
privatefunctionrezolveParentAccess(array$data)

Choose a reason for hiding this comment

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

resolve

*
* @return array
*
* @throws \ReflectionException

Choose a reason for hiding this comment

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

not sure about this

/**
* @param array $nestedLevelsName
*
* @return self

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)

/**
* @param null $parent
*
* @return self

Choose a reason for hiding this comment

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

same

@Doctrs
Copy link
ContributorAuthor

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

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

@fabpotfabpot closed thisOct 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
@nicolas-grekasnicolas-grekas modified the milestones:next,4.2Nov 1, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

Assignees

No one assigned

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

[8]ページ先頭

©2009-2025 Movatter.jp