-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[POC] [OptionsResolver] Support nested options #11509
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
* @param null|array $value | ||
* @param string $option | ||
*/ | ||
public function nestedNormalizer(Options $options, $value, $option) |
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.
missing return doc. also this method is only for internal purposes. and manually calling it can result in php error (when option is not set in optionResolvers).
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.
So I should tag it with @internal
?
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.
Yes, mark it with @internal
.
Why OptionResolver component was created? IMHO it was opportunity use Config component and not create new one, wasn't it? // cc @webmozart |
I don't know why you ask this question, but it was about validating option arrays in an easy and quick way. It was created especially for usage in the Form component. In the form component, there are some options which have nested arrays (e.g. the And if you are unsure if @webmozart will like this: He proposed this feature himself :) See more context in #4833 (I've added it in the table) |
Yes, I know that he extracted this component from Form. But can you agree that this two components designed to do the same thing? |
No. OptionsResolver is made to be very quick and very simple (i.e. has not many features). It should be used within a class. |
@Koc you are months late as the component already exists now. Please read the original discussion to get insights. Also this has nothing to do with this PR. |
Thanks for explanation @wouterj. |
@Koc The Config Definition component and the OptionsResolver component have different goals
Using the Config component to resolve options of the Form would not work (well, except if you accept writing 100 lines of code to define a default value for an option depending on another one) and supporting the type hierarchy would be insane (child types can redefine default values for options even if a parent type defined it previously for instance) |
'db' => $this->getNestedResolver(), | ||
)); | ||
|
||
$this->assertEquals(array(), $this->resolver->resolve(array())); |
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.
The assert is actually not needed as your expecting an exception.
ref #11716 |
Thank you for the PR @wouterj! I did a few changes in #12156 that allow for a nicer implementation of nested options in 2.7+. The rough idea is:
Rough usage: // Set nested options
// $options['nested'] returns a new Options instance if previously undefined/unset
$options['nested']['foo'] = 'bar';
$options['nested']['answer'] = 42;
$options['nested']['dynamic'] = function (Options $options) {
// $options is the root instance, not the nested one
}
// Overwrite nested options
$options['nested'] = new Options();
$options['nested']['foo'] = 'bar';
// Internally
$options['nested']->values = &$options->values['nested'] = array();
$options['nested']->defined = &$options->defined['nested'] = array();
// ... Since that differs quite a lot from this PR, I'll close it. If you want to implement this, I'll be happy to review your PR! :) |
Issue #4833 (which can't be fixed before 3.0) is causing unexpected behaviour in this PR. Maybe this PR has to be merged for 3.0.
Thanks @sstok, for giving this nice idea :)