merged branch Tobion/optionsresolver-optim (PR #4388)

Commits
-------

bad4a1f [OptionsResolver] CS fix in LazyOption
a54ea1b [OptionsResolver] small optimization in Options class
104dcf2 [OptionsResolver] fixed bugs concerning required options
1bfcff4 [OptionsResolver] added failing test cases to demonstrate two bugs
37a3a29 [OptionsResolver] optimized validation

Discussion
----------

[OptionsResolver] fixed two bugs and applied optimization

The first commit optimizes the validation in OptionsResolver by removing several unneeded method calls (without changing anything semantically).

Then I recognized two bugs in the current code that I wrote failing test cases for in the second commit.
1. setAllowedValues wrongly validated missing options
2. required options with defaults were considered missing by `resolve` (contrary to the `isRequired` method)

The third commit fixes these bugs.

The forth commit applies a small optimization in Options and uses a static function call for a static function.

---------------------------------------------------------------------------

by travisbot at 2012-05-24T03:39:34Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1418785) (merged a54ea1b6 into b07fb3c4).

---------------------------------------------------------------------------

by travisbot at 2012-05-24T05:22:33Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1419232) (merged bad4a1f7 into b07fb3c4).

---------------------------------------------------------------------------

by bschussek at 2012-05-24T06:20:02Z

I just tested this on my machine, and static calls are a tiny bit faster here, although this is really irrelevant for practical use. Even though I dislike useless micro-optimizations like this, I'm ok with this PR in general.

---------------------------------------------------------------------------

by Tobion at 2012-05-24T13:23:11Z

I didn't say that's an optimization in the first place. (The optimization was the removal of a variable assignment)
I just changed it because in other PRs I've been told, static functions should be called in a static way.

---------------------------------------------------------------------------

by Tobion at 2012-05-24T23:36:13Z

Please merge before 4387

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;

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;
     }
 
     /**

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 <bschussek@gmail.com>
+ * @author Tobias Schultze <http://tobion.de>
  */
 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
+     * This method declares a valid option names without setting default values for
-     * are configured for the options. If these options are not passed to
+     * them. If these options are not passed to {@link resolve()} and no default has
-     * resolve(), they will be missing in the final options array. This can be
+     * been set for them, they will be missing in the final options array. This can
-     * helpful if you want to determine whether an option has been set or not.
+     * 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
+     * @throws InvalidOptionsException If an option has not been defined
-     *                                 which an allowed value is set.
+     *                                 (see {@link isKnown()}) for which
+     *                                 an allowed value is set.
      */
     public function setAllowedValues(array $allowedValues)
     {
-        $this->validateOptionNames(array_keys($allowedValues));
+        $this->validateOptionsExistence($allowedValues);
 
         $this->allowedValues = array_replace($this->allowedValues, $allowedValues);
 
@@ -191,7 +201,7 @@ class OptionsResolver
      */
     public function addAllowedValues(array $allowedValues)
     {
-        $this->validateOptionNames(array_keys($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(array_keys($options));
+        $this->validateOptionsExistence($options);
+        $this->validateOptionsCompleteness($options);
 
         // Make sure this method can be called multiple times
         $combinedOptions = clone $this->defaultOptions;
@@ -266,44 +277,45 @@ class OptionsResolver
      * Validates that the given option names exist and throws an exception
      * otherwise.
      *
-     * @param array $optionNames A list of option names.
+     * @param array $options An list of option names as keys.
+     *
+     * @throws InvalidOptionsException If any of the options has not been defined.
+     */
+    private function validateOptionsExistence(array $options)
+    {
+        $diff = array_diff_key($options, $this->knownOptions);
+
+        if (count($diff) > 0) {
+            ksort($this->knownOptions);
+            ksort($diff);
+
+            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))
+            ));
+        }
+    }
+
+    /**
+     * Validates that all required options are given and throws an exception
+     * otherwise.
+     *
+     * @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.
      */
-    private function validateOptionNames(array $optionNames)
+    private function validateOptionsCompleteness(array $options)
     {
-        ksort($this->knownOptions);
+        $diff = array_diff_key($this->requiredOptions, $options);
-
-        $knownOptions = array_keys($this->knownOptions);
-        $diff = array_diff($optionNames, $knownOptions);
-
-        sort($diff);
 
         if (count($diff) > 0) {
-            if (count($diff) > 1) {
+            ksort($diff);
-                throw new InvalidOptionsException(sprintf('The options "%s" do not exist. Known options are: "%s"', implode('", "', $diff), implode('", "', $knownOptions)));
-            }
 
-            throw new InvalidOptionsException(sprintf('The option "%s" does not exist. Known options are: "%s"', current($diff), implode('", "', $knownOptions)));
+            throw new MissingOptionsException(sprintf(
-        }
+                count($diff) > 1 ? 'The required options "%s" are missing.' : 'The required option "%s" is  missing.',
-
+                implode('", "', array_keys($diff))
-        ksort($this->requiredOptions);
+            ));
-
-        $requiredOptions = array_keys($this->requiredOptions);
-        $diff = array_diff($requiredOptions, $optionNames);
-
-        sort($diff);
-
-        if (count($diff) > 0) {
-            if (count($diff) > 1) {
-                throw new MissingOptionsException(sprintf('The required options "%s" are missing.',
-                    implode('",
-                "', $diff)));
-            }
-
-            throw new MissingOptionsException(sprintf('The required option "%s" is  missing.', current($diff)));
         }
     }
 
@@ -313,8 +325,8 @@ class OptionsResolver
      *
      * @param array $options A list of option values.
      *
-     * @throws InvalidOptionsException  If any of the values does not match the
+     * @throws InvalidOptionsException If any of the values does not match the
-     *                                  allowed values of the option.
+     *                                 allowed values of the option.
      */
     private function validateOptionValues(array $options)
     {

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));
+    }
 }