8000 [OptionsResolver] Choose policy on unknown options by iamluc · Pull Request #9754 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[OptionsResolver] Choose policy on unknown options #9754

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

Closed
wants to merge 1 commit into from
Closed

[OptionsResolver] Choose policy on unknown options #9754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions src/Symfony/Component/OptionsResolver/OptionsResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,18 @@ public function isRequired($option)
/**
* {@inheritdoc}
*/
public function resolve(array $options = array())
public function resolve(array $options = array(), $flags = 0)
{
$this->validateOptionsExistence($options);
if (($flags & OptionsResolverInterface::REMOVE_UNKNOWN) && ($flags & OptionsResolverInterface::IGNORE_UNKNOWN)) {
throw new \InvalidArgumentException('OptionsResolverInterface::REMOVE_UNKNOWN and OptionsResolverInterface::IGNORE_UNKNOWN are mutually exclusive');
}

if ($flags & OptionsResolverInterface::REMOVE_UNKNOWN) {
$options = $this->removeUnknownOptions($options);
} elseif (!($flags & OptionsResolverInterface::IGNORE_UNKNOWN)) {
$this->validateOptionsExistence($options);
}

$this->validateOptionsCompleteness($options);

// Make sure this method can be called multiple times
Expand All @@ -236,11 +245,23 @@ public function resolve(array $options = array())
return $resolvedOptions;
}

/**
* Remove unknown options
*
* @param array $options A list of option names as keys.
*
* @return array Options with all unknown options removed
*/
private function removeUnknownOptions(array $options)
{
return array_intersect_key($options, $this->knownOptions);
}

/**
* Validates that the given option names exist and throws an exception
* otherwise.
*
* @param array $options An list of option names as keys.
* @param array $options A list of option names as keys.
*
* @throws InvalidOptionsException If any of the options has not been defined.
*/
Expand All @@ -264,7 +285,7 @@ private function validateOptionsExistence(array $options)
* Validates that all required options are given and throws an exception
* otherwise.
*
* @param array $options An list of option names as keys.
* @param array $options A list of option names as keys.
*
* @throws MissingOptionsException If a required option is missing.
*/
Expand Down
15 changes: 14 additions & 1 deletion src/Symfony/Component/OptionsResolver/OptionsResolverInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,18 @@
*/
interface OptionsResolverInterface
{
/**
* Remove unknown options passed to the resolve() method
* Is mutually exclusive with IGNORE_UNKNOWN
*/
const REMOVE_UNKNOWN = 1;

/**
* Ignore unknown options passed to the resolve() method
* Is mutually exclusive with REMOVE_UNKNOWN
*/
const IGNORE_UNKNOWN = 2;

/**
* Sets default option values.
*
Expand Down Expand Up @@ -196,6 +208,7 @@ public function isRequired($option);
* Returns the combination of the default and the passed options.
*
* @param array $options The custom option values.
* @param int $flags Can be REMOVE_UNKNOWN or IGNORE_UNKNOWN
*
* @return array A list of options and their values.
*
Expand All @@ -206,5 +219,5 @@ public function isRequired($option);
* @throws Exception\OptionDefinitionException If a cyclic dependency is detected
* between two lazy options.
*/
public function resolve(array $options = array());
public function resolve(array $options = array(), $flags = 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO the default should also be represented by a const like FORBID_UNKNOWN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we add this, one of the 3 flags will be required (but mutually exclusive) and I think flags should be optionals. No ?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think he means public function resolve(array $options = array(), $flags = self::FORBID_UNKNOWN);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but you can't call $resolver->resolve($myOptions, 0);
So one flag is required (but only one).

In that case, IMO, it should be $mode or $policy, but not $flags

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree that the default should be named. If we add more flags than just handling of unknown options, the default won't make sense anymore.

}
49 changes: 49 additions & 0 deletions src/Symfony/Component/OptionsResolver/Tests/OptionsResolverTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\OptionsResolver\Tests;

use Symfony\Component\OptionsResolver\OptionsResolverInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\OptionsResolver\Options;

Expand Down Expand Up @@ -678,4 +679,52 @@ public function testClone()
'three' => '3',
), $clone->resolve());
}

/**
* @expectedException \InvalidArgumentException
*/
public function testRemoveUnknownAndIgnoreUnknownAreMutuallyExclusive()
{
$this->resolver->resolve(array(
'one' => 'one',
'two' => 'two'
), OptionsResolverInterface::REMOVE_UNKNOWN | OptionsResolverInterface::IGNORE_UNKNOWN);
}

public function testRemoveUnknownOption()
{
$this->resolver->setDefaults(array(
'one' => 'default',
'two' => 'default'
));

$options = $this->resolver->resolve(array(
'one' => 'keep',
'three' => 'remove'
), OptionsResolverInterface::REMOVE_UNKNOWN);

$this->assertEquals($options, array(
'one' => 'keep',
'two' => 'default'
));
}

public function testIgnoreUnknownOption()
{
$this->resolver->setDefaults(array(
'one' => 'default',
'two' => 'default'
));

$options = $this->resolver->resolve(array(
'one' => 'keep',
'three' => 'ignored'
), OptionsResolverInterface::IGNORE_UNKNOWN);

$this->assertEquals($options, array(
'one' => 'keep',
'two' => 'default',
'three' => 'ignored'
));
}
}
0