Skip to content

[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

Closed
wants to merge 1 commit into from

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Jul 29, 2014

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.

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #4833
License MIT
Doc PR todo

Thanks @sstok, for giving this nice idea :)

* @param null|array $value
* @param string $option
*/
public function nestedNormalizer(Options $options, $value, $option)
Copy link
Contributor

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

Copy link
Member Author

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?

Copy link
Contributor

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.

@Koc
Copy link
Contributor

Koc commented Jul 29, 2014

Why OptionResolver component was created? IMHO it was opportunity use Config component and not create new one, wasn't it?

// cc @webmozart

@wouterj
Copy link
Member Author

wouterj commented Jul 29, 2014

Why OptionResolver component was created? IMHO it was opportunity use Config component and not create new one, wasn't it?

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 attr option).

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)

@Koc
Copy link
Contributor

Koc commented Jul 29, 2014

Yes, I know that he extracted this component from Form. But can you agree that this two components designed to do the same thing?

@wouterj
Copy link
Member Author

wouterj commented Jul 29, 2014

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.
Config is made very complex for handling, validating, merging and loading the whole application config.

@Tobion
Copy link
Contributor

Tobion commented Jul 29, 2014

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

@Koc
Copy link
Contributor

Koc commented Jul 30, 2014

Thanks for explanation @wouterj.

@stof
Copy link
Member

stof commented Aug 14, 2014

@Koc The Config Definition component and the OptionsResolver component have different goals

  • Config Definition merges several (deep) nested arrays and validates them against a tree. Each node has a single possible default value which does not depend on other node (except by doing very crazy stuff)
  • OptionsResolver is about resolving a flat array of options, where a single array is passed as input, but defaults values can be much more complex and depend on each other.

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()));
Copy link
Contributor

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.

@webmozart
Copy link
Contributor

ref #11716

@webmozart
Copy link
Contributor

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:

  • If an option contains nested options, the corresponding entry of Options::$values contains another Options instance.
  • The properties of the nested Options instance are set to references of the corresponding array entries in the root Options instance.
  • Resolving is done in the root Options instance. Lazy options and normalizers always receive the root Options instance.

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

@webmozart webmozart closed this Oct 16, 2014
@wouterj wouterj deleted the issue_4833 branch October 16, 2014 09:52
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