From 37a3a29c5991fd980d5d1e47d155426b7c9fab48 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Thu, 24 May 2012 02:48:57 +0200 Subject: [PATCH 1/5] [OptionsResolver] optimized validation --- .../OptionsResolver/OptionsResolver.php | 44 ++++++++----------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/src/Symfony/Component/OptionsResolver/OptionsResolver.php b/src/Symfony/Component/OptionsResolver/OptionsResolver.php index 1ab58ec718..cebaf0eef7 100644 --- a/src/Symfony/Component/OptionsResolver/OptionsResolver.php +++ b/src/Symfony/Component/OptionsResolver/OptionsResolver.php @@ -168,7 +168,7 @@ class OptionsResolver */ public function setAllowedValues(array $allowedValues) { - $this->validateOptionNames(array_keys($allowedValues)); + $this->validateOptionNames($allowedValues); $this->allowedValues = array_replace($this->allowedValues, $allowedValues); @@ -191,7 +191,7 @@ class OptionsResolver */ public function addAllowedValues(array $allowedValues) { - $this->validateOptionNames(array_keys($allowedValues)); + $this->validateOptionNames($allowedValues); $this->allowedValues = array_merge_recursive($this->allowedValues, $allowedValues); @@ -243,7 +243,7 @@ class OptionsResolver */ public function resolve(array $options) { - $this->validateOptionNames(array_keys($options)); + $this->validateOptionNames($options); // Make sure this method can be called multiple times $combinedOptions = clone $this->defaultOptions; @@ -266,7 +266,7 @@ class OptionsResolver * Validates that the given option names exist and throws an exception * otherwise. * - * @param array $optionNames A list of option names. + * @param array $optionNames An list of option names as keys. * * @throws InvalidOptionsException If any of the options has not been * defined. @@ -274,36 +274,28 @@ class OptionsResolver */ private function validateOptionNames(array $optionNames) { - ksort($this->knownOptions); - - $knownOptions = array_keys($this->knownOptions); - $diff = array_diff($optionNames, $knownOptions); - - sort($diff); + $diff = array_diff_key($optionNames, $this->knownOptions); if (count($diff) > 0) { - if (count($diff) > 1) { - throw new InvalidOptionsException(sprintf('The options "%s" do not exist. Known options are: "%s"', implode('", "', $diff), implode('", "', $knownOptions))); - } + ksort($this->knownOptions); + ksort($diff); - throw new InvalidOptionsException(sprintf('The option "%s" does not exist. Known options are: "%s"', current($diff), implode('", "', $knownOptions))); + throw new InvalidOptionsException(sprintf( + (count($diff) > 1 ? 'The options "%s" do not exist.' : 'The option "%s" does not exist.') . ' Known options are: "%s"', + implode('", "', array_keys($diff)), + implode('", "', array_keys($this->knownOptions)) + )); } - ksort($this->requiredOptions); - - $requiredOptions = array_keys($this->requiredOptions); - $diff = array_diff($requiredOptions, $optionNames); - - sort($diff); + $diff = array_diff_key($this->requiredOptions, $optionNames); if (count($diff) > 0) { - if (count($diff) > 1) { - throw new MissingOptionsException(sprintf('The required options "%s" are missing.', - implode('", - "', $diff))); - } + ksort($diff); - throw new MissingOptionsException(sprintf('The required option "%s" is missing.', current($diff))); + throw new MissingOptionsException(sprintf( + count($diff) > 1 ? 'The required options "%s" are missing.' : 'The required option "%s" is missing.', + implode('", "', array_keys($diff)) + )); } } From 1bfcff4fab44dab9698105630ad6b6695dd7d072 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Thu, 24 May 2012 05:29:35 +0200 Subject: [PATCH 2/5] [OptionsResolver] added failing test cases to demonstrate two bugs --- .../Tests/OptionResolverTest.php | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/Symfony/Component/OptionsResolver/Tests/OptionResolverTest.php b/src/Symfony/Component/OptionsResolver/Tests/OptionResolverTest.php index 5ecb38fd15..ef68c7f1e1 100644 --- a/src/Symfony/Component/OptionsResolver/Tests/OptionResolverTest.php +++ b/src/Symfony/Component/OptionsResolver/Tests/OptionResolverTest.php @@ -372,4 +372,49 @@ class OptionsResolverTest extends \PHPUnit_Framework_TestCase $this->assertFalse($this->resolver->isRequired('foo')); } + + public function testResolveWithoutOptionSucceedsIfRequiredAndDefaultValue() + { + $this->resolver->setRequired(array( + 'foo', + )); + $this->resolver->setDefaults(array( + 'foo' => 'bar', + )); + + $this->assertEquals(array( + 'foo' => 'bar' + ), $this->resolver->resolve(array())); + } + + public function testResolveWithoutOptionSucceedsIfDefaultValueAndRequired() + { + $this->resolver->setDefaults(array( + 'foo' => 'bar', + )); + $this->resolver->setRequired(array( + 'foo', + )); + + $this->assertEquals(array( + 'foo' => 'bar' + ), $this->resolver->resolve(array())); + } + + public function testResolveSucceedsIfOptionRequiredAndValueAllowed() + { + $this->resolver->setRequired(array( + 'one', 'two', + )); + $this->resolver->setAllowedValues(array( + 'two' => array('2'), + )); + + $options = array( + 'one' => '1', + 'two' => '2' + ); + + $this->assertEquals($options, $this->resolver->resolve($options)); + } } From 104dcf251dcfd97a5035b57b9248a6b0c4e258e6 Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Thu, 24 May 2012 05:31:42 +0200 Subject: [PATCH 3/5] [OptionsResolver] fixed bugs concerning required options --- .../OptionsResolver/OptionsResolver.php | 64 ++++++++++++------- 1 file changed, 42 insertions(+), 22 deletions(-) diff --git a/src/Symfony/Component/OptionsResolver/OptionsResolver.php b/src/Symfony/Component/OptionsResolver/OptionsResolver.php index cebaf0eef7..0feeeddfa7 100644 --- a/src/Symfony/Component/OptionsResolver/OptionsResolver.php +++ b/src/Symfony/Component/OptionsResolver/OptionsResolver.php @@ -19,6 +19,7 @@ use Symfony\Component\OptionsResolver\Exception\MissingOptionsException; * Helper for merging default and concrete option values. * * @author Bernhard Schussek + * @author Tobias Schultze */ class OptionsResolver { @@ -35,7 +36,7 @@ class OptionsResolver private $knownOptions = array(); /** - * The options required to be passed to resolve(). + * The options without defaults that are required to be passed to resolve(). * @var array */ private $requiredOptions = array(); @@ -71,6 +72,7 @@ class OptionsResolver foreach ($defaultValues as $option => $value) { $this->defaultOptions->overload($option, $value); $this->knownOptions[$option] = true; + unset($this->requiredOptions[$option]); } return $this; @@ -97,6 +99,7 @@ class OptionsResolver foreach ($defaultValues as $option => $value) { $this->defaultOptions->set($option, $value); $this->knownOptions[$option] = true; + unset($this->requiredOptions[$option]); } return $this; @@ -105,10 +108,12 @@ class OptionsResolver /** * Sets optional options. * - * This method is identical to `setDefaults`, only that no default values - * are configured for the options. If these options are not passed to - * resolve(), they will be missing in the final options array. This can be - * helpful if you want to determine whether an option has been set or not. + * This method declares a valid option names without setting default values for + * them. If these options are not passed to {@link resolve()} and no default has + * been set for them, they will be missing in the final options array. This can + * be helpful if you want to determine whether an option has been set or not + * because otherwise {@link resolve()} would trigger an exception for unknown + * options. * * @param array $optionNames A list of option names. * @@ -132,7 +137,8 @@ class OptionsResolver /** * Sets required options. * - * If these options are not passed to resolve(), an exception will be thrown. + * If these options are not passed to resolve() and no default has been set for + * them, an exception will be thrown. * * @param array $optionNames A list of option names. * @@ -148,7 +154,10 @@ class OptionsResolver } $this->knownOptions[$option] = true; - $this->requiredOptions[$option] = true; + // set as required if no default has been set already + if (!isset($this->defaultOptions[$option])) { + $this->requiredOptions[$option] = true; + } } return $this; @@ -163,12 +172,13 @@ class OptionsResolver * * @return OptionsResolver The resolver instance. * - * @throws InvalidOptionsException If an option has not been defined for - * which an allowed value is set. + * @throws InvalidOptionsException If an option has not been defined + * (see {@link isKnown()}) for which + * an allowed value is set. */ public function setAllowedValues(array $allowedValues) { - $this->validateOptionNames($allowedValues); + $this->validateOptionsExistence($allowedValues); $this->allowedValues = array_replace($this->allowedValues, $allowedValues); @@ -191,7 +201,7 @@ class OptionsResolver */ public function addAllowedValues(array $allowedValues) { - $this->validateOptionNames($allowedValues); + $this->validateOptionsExistence($allowedValues); $this->allowedValues = array_merge_recursive($this->allowedValues, $allowedValues); @@ -224,7 +234,7 @@ class OptionsResolver */ public function isRequired($option) { - return isset($this->requiredOptions[$option]) && !isset($this->defaultOptions[$option]); + return isset($this->requiredOptions[$option]); } /** @@ -243,7 +253,8 @@ class OptionsResolver */ public function resolve(array $options) { - $this->validateOptionNames($options); + $this->validateOptionsExistence($options); + $this->validateOptionsCompleteness($options); // Make sure this method can be called multiple times $combinedOptions = clone $this->defaultOptions; @@ -266,15 +277,13 @@ class OptionsResolver * Validates that the given option names exist and throws an exception * otherwise. * - * @param array $optionNames An list of option names as keys. + * @param array $options An list of option names as keys. * - * @throws InvalidOptionsException If any of the options has not been - * defined. - * @throws MissingOptionsException If a required option is missing. + * @throws InvalidOptionsException If any of the options has not been defined. */ - private function validateOptionNames(array $optionNames) + private function validateOptionsExistence(array $options) { - $diff = array_diff_key($optionNames, $this->knownOptions); + $diff = array_diff_key($options, $this->knownOptions); if (count($diff) > 0) { ksort($this->knownOptions); @@ -286,8 +295,19 @@ class OptionsResolver implode('", "', array_keys($this->knownOptions)) )); } + } - $diff = array_diff_key($this->requiredOptions, $optionNames); + /** + * Validates that all required options are given and throws an exception + * otherwise. + * + * @param array $options An list of option names as keys. + * + * @throws MissingOptionsException If a required option is missing. + */ + private function validateOptionsCompleteness(array $options) + { + $diff = array_diff_key($this->requiredOptions, $options); if (count($diff) > 0) { ksort($diff); @@ -305,8 +325,8 @@ class OptionsResolver * * @param array $options A list of option values. * - * @throws InvalidOptionsException If any of the values does not match the - * allowed values of the option. + * @throws InvalidOptionsException If any of the values does not match the + * allowed values of the option. */ private function validateOptionValues(array $options) { From a54ea1b6b24c15476211b8648ce1095241cd596e Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Thu, 24 May 2012 05:32:58 +0200 Subject: [PATCH 4/5] [OptionsResolver] small optimization in Options class --- src/Symfony/Component/OptionsResolver/Options.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/Symfony/Component/OptionsResolver/Options.php b/src/Symfony/Component/OptionsResolver/Options.php index 37adef7737..079e0a4050 100644 --- a/src/Symfony/Component/OptionsResolver/Options.php +++ b/src/Symfony/Component/OptionsResolver/Options.php @@ -139,17 +139,15 @@ class Options implements \ArrayAccess, \Iterator, \Countable throw new OptionDefinitionException('Options cannot be overloaded anymore once options have been read.'); } - $newValue = $value; - // Reset lazy flag and locks by default unset($this->lock[$option]); unset($this->lazy[$option]); // If an option is a closure that should be evaluated lazily, store it // inside a LazyOption instance. - if ($this->isEvaluatedLazily($value)) { + if (self::isEvaluatedLazily($value)) { $currentValue = isset($this->options[$option]) ? $this->options[$option] : null; - $newValue = new LazyOption($value, $currentValue); + $value = new LazyOption($value, $currentValue); // Store locks for lazy options to detect cyclic dependencies $this->lock[$option] = false; @@ -158,7 +156,7 @@ class Options implements \ArrayAccess, \Iterator, \Countable $this->lazy[$option] = true; } - $this->options[$option] = $newValue; + $this->options[$option] = $value; } /** From bad4a1f76cb990be742f9dc16cfdb3c1a52c27ab Mon Sep 17 00:00:00 2001 From: Tobias Schultze Date: Thu, 24 May 2012 07:20:57 +0200 Subject: [PATCH 5/5] [OptionsResolver] CS fix in LazyOption --- src/Symfony/Component/OptionsResolver/LazyOption.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Symfony/Component/OptionsResolver/LazyOption.php b/src/Symfony/Component/OptionsResolver/LazyOption.php index adff659b40..dd52c5a70b 100644 --- a/src/Symfony/Component/OptionsResolver/LazyOption.php +++ b/src/Symfony/Component/OptionsResolver/LazyOption.php @@ -11,8 +11,6 @@ namespace Symfony\Component\OptionsResolver; -use Closure; - /** * An option that is evaluated lazily using a closure. * @@ -22,7 +20,7 @@ class LazyOption { /** * The underlying closure. - * @var Closure + * @var \Closure */ private $closure; @@ -43,7 +41,7 @@ class LazyOption * * @see evaluate() */ - public function __construct(Closure $closure, $previousValue) + public function __construct(\Closure $closure, $previousValue) { $this->closure = $closure; $this->previousValue = $previousValue;