symfony · fabpot · May 25, 2012 · May 24, 2012 · May 24, 2012 · May 24, 2012
diff --git 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;

diff --git a/src/Symfony/Component/OptionsResolver/Options.php b/src/Symfony/Component/OptionsResolver/Options.php
@@ -139,17 +139,15 @@ public function overload($option, $value)
             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 @@ public function overload($option, $value)
             $this->lazy[$option] = true;
         }
 
-        $this->options[$option] = $newValue;
+        $this->options[$option] = $value;
     }
 
     /**

diff --git a/src/Symfony/Component/OptionsResolver/OptionsResolver.php b/src/Symfony/Component/OptionsResolver/OptionsResolver.php
@@ -19,6 +19,7 @@
  * 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 @@ public function setDefaults(array $defaultValues)
         foreach ($defaultValues as $option => $value) {
             $this->defaultOptions->overload($option, $value);
             $this->knownOptions[$option] = true;
+            unset($this->requiredOptions[$option]);
         }
 
         return $this;
@@ -97,6 +99,7 @@ public function replaceDefaults(array $defaultValues)
         foreach ($defaultValues as $option => $value) {
             $this->defaultOptions->set($option, $value);
             $this->knownOptions[$option] = true;
+            unset($this->requiredOptions[$option]);
         }
 
         return $this;
@@ -105,10 +108,12 @@ public function replaceDefaults(array $defaultValues)
     /**
      * 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 @@ public function setOptional(array $optionNames)
     /**
      * 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 @@ public function setRequired(array $optionNames)
             }
 
             $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 @@ public function setRequired(array $optionNames)
      *
      * @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(array_keys($allowedValues));
+        $this->validateOptionsExistence($allowedValues);
 
         $this->allowedValues = array_replace($this->allowedValues, $allowedValues);
 
@@ -191,7 +201,7 @@ public function setAllowedValues(array $allowedValues)
      */
     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 @@ public function isKnown($option)
      */
     public function isRequired($option)
     {
-        return isset($this->requiredOptions[$option]) && !isset($this->defaultOptions[$option]);
+        return isset($this->requiredOptions[$option]);
     }
 
     /**
@@ -243,7 +253,8 @@ public function isRequired($option)
      */
     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 @@ public function resolve(array $options)
      * 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.
-     * @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)
     {
-        ksort($this->knownOptions);
-
-        $knownOptions = array_keys($this->knownOptions);
-        $diff = array_diff($optionNames, $knownOptions);
-
-        sort($diff);
+        $diff = array_diff_key($options, $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)));
-            }
-
-            throw new InvalidOptionsException(sprintf('The option "%s" does not exist. Known options are: "%s"', current($diff), implode('", "', $knownOptions)));
+            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))
+            ));
         }
+    }
 
-        ksort($this->requiredOptions);
-
-        $requiredOptions = array_keys($this->requiredOptions);
-        $diff = array_diff($requiredOptions, $optionNames);
-
-        sort($diff);
+    /**
+     * 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) {
-            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))
+            ));
         }
     }
 
@@ -313,8 +325,8 @@ private function validateOptionNames(array $optionNames)
      *
      * @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)
     {

diff --git a/src/Symfony/Component/OptionsResolver/Tests/OptionResolverTest.php b/src/Symfony/Component/OptionsResolver/Tests/OptionResolverTest.php
@@ -372,4 +372,49 @@ public function testNotRequiredIfRequiredAndDefaultValue()
 
         $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));
+    }
 }