[OptionsResolver] Improved implementation and clarity of the Options class

src/Symfony/Component/OptionsResolver/Options.php

@@ -12,8 +12,10 @@
 namespace Symfony\Component\OptionsResolver;
 
 use ArrayAccess;
+use Closure;
 use Iterator;
 use OutOfBoundsException;
+use Countable;
 use Symfony\Component\OptionsResolver\Exception\OptionDefinitionException;
 
 /**
@@ -21,7 +23,7 @@ use Symfony\Component\OptionsResolver\Exception\OptionDefinitionException;
  *
  * @author Bernhard Schussek <bschussek@gmail.com>
  */
-class Options implements ArrayAccess, Iterator
+class Options implements ArrayAccess, Iterator, Countable
 {
     /**
      * A list of option values and LazyOption instances.
@@ -29,6 +31,12 @@ class Options implements ArrayAccess, Iterator
      */
     private $options = array();
 
+    /**
+     * A list storing the names of all LazyOption instances as keys.
+     * @var array
+     */
+    private $lazy = array();
+
     /**
      * A list of Boolean locks for each LazyOption.
      * @var array
@@ -36,13 +44,150 @@ class Options implements ArrayAccess, Iterator
     private $lock = array();
 
     /**
-     * Whether the options have already been resolved.
+     * Whether at least one option has already been read.
      *
-     * Once resolved, no new options can be added or changed anymore.
+     * Once reading, the options cannot be changed anymore. This is
+     * necessary in order to avoid inconsistencies during the resolving
+     * process. If any option is changed after reading, all evaluated
+     * lazy options that depend on this option would become invalid.
      *
      * @var Boolean
      */
-    private $resolved = false;
+    private $reading = false;
+
+    /**
+     * Sets the value of a given option.
+     *
+     * You can set lazy options by passing a closure with the following
+     * signature:
+     *
+     * <code>
+     * function (Options $options)
+     * </code>
+     *
+     * This closure will be evaluated once the option is read using
+     * {@link get()}. The closure has access to the resolved values of
+     * other options through the passed {@link Options} instance.
+     *
+     * @param string $option The name of the option.
+     * @param mixed  $value  The value of the option.
+     *
+     * @throws OptionDefinitionException If options have already been read.
+     *                                   Once options are read, the container
+     *                                   becomes immutable.
+     */
+    public function set($option, $value)
+    {
+        // Setting is not possible once an option is read, because then lazy
+        // options could manipulate the state of the object, leading to
+        // inconsistent results.
+        if ($this->reading) {
+            throw new OptionDefinitionException('Options cannot be set anymore once options have been read.');
+        }
+
+        // Setting is equivalent to overloading while discarding the previous
+        // option value
+        unset($this->options[$option]);
+
+        $this->overload($option, $value);
+    }
+
+    /**
+     * Replaces the contents of the container with the given options.
+     *
+     * This method is a shortcut for {@link clear()} with subsequent
+     * calls to {@link set()}.
+     *
+     * @param array $options The options to set.
+     *
+     * @throws OptionDefinitionException If options have already been read.
+     *                                   Once options are read, the container
+     *                                   becomes immutable.
+     */
+    public function replace(array $options)
+    {
+        if ($this->reading) {
+            throw new OptionDefinitionException('Options cannot be replaced anymore once options have been read.');
+        }
+
+        $this->options = array();
+        foreach ($options as $option => $value) {
+            $this->options[$option] = $value;
+        }
+    }
+
+    /**
+     * Overloads the value of a given option.
+     *
+     * Contrary to {@link set()}, this method keeps the previous default
+     * value of the option so that you can access it if you pass a closure.
+     * Passed closures should have the following signature:
+     *
+     * <code>
+     * function (Options $options, $previousValue)
+     * </code>
+     *
+     * The second parameter passed to the closure is the previous default
+     * value of the option.
+     *
+     * @param string $option The option name.
+     * @param mixed  $value  The option value.
+     *
+     * @throws OptionDefinitionException If options have already been read.
+     *                                   Once options are read, the container
+     *                                   becomes immutable.
+     */
+    public function overload($option, $value)
+    {
+        if ($this->reading) {
+            throw new OptionDefinitionException('Options cannot be overloaded anymore once options have been read.');
+        }
+
+        $newValue = $value;
+
+        // If an option is a closure that should be evaluated lazily, store it
+        // inside a LazyOption instance.
+        if ($this->isEvaluatedLazily($value)) {
+            $currentValue = isset($this->options[$option]) ? $this->options[$option] : null;
+            $newValue = new LazyOption($value, $currentValue);
+
+            // Store locks for lazy options to detect cyclic dependencies
+            $this->lock[$option] = false;
+
+            // Store which options are lazy for more efficient resolving
+            $this->lazy[$option] = true;
+        }
+
+        $this->options[$option] = $newValue;
+    }
+
+    /**
+     * Returns the value of the given option.
+     *
+     * If the option was a lazy option, it is evaluated now.
+     *
+     * @param  string $option The option name.
+     *
+     * @return mixed The option value.
+     *
+     * @throws OutOfBoundsException      If the option does not exist.
+     * @throws OptionDefinitionException If a cyclic dependency is detected
+     *                                   between two lazy options.
+     */
+    public function get($option)
+    {
+        $this->reading = true;
+
+        if (!array_key_exists($option, $this->options)) {
+            throw new OutOfBoundsException('The option "' . $option . '" does not exist.');
+        }
+
+        if (isset($this->lazy[$option])) {
+            $this->resolve($option);
+        }
+
+        return $this->options[$option];
+    }
 
     /**
      * Returns whether the given option exists.
@@ -50,18 +195,84 @@ class Options implements ArrayAccess, Iterator
      * @param  string $option The option name.
      *
      * @return Boolean Whether the option exists.
+     */
+    public function has($option)
+    {
+        return isset($this->options[$option]);
+    }
+
+    /**
+     * Removes the option with the given name.
+     *
+     * @param string $option The option name.
+     *
+     * @throws OptionDefinitionException If options have already been read.
+     *                                   Once options are read, the container
+     *                                   becomes immutable.
+     */
+    public function remove($option)
+    {
+        if ($this->reading) {
+            throw new OptionDefinitionException('Options cannot be removed anymore once options have been read.');
+        }
+
+        unset($this->options[$option]);
+        unset($this->lock[$option]);
+    }
+
+    /**
+     * Removes all options.
+     *
+     * @throws OptionDefinitionException If options have already been read.
+     *                                   Once options are read, the container
+     *                                   becomes immutable.
+     */
+    public function clear()
+    {
+        if ($this->reading) {
+            throw new OptionDefinitionException('Options cannot be cleared anymore once options have been read.');
+        }
+
+        $this->options = array();
+    }
+
+    /**
+     * Returns the values of all options.
+     *
+     * Lazy options are evaluated at this point.
+     *
+     * @return array The option values.
+     */
+    public function all()
+    {
+        $this->reading = true;
+
+        // Create a copy because resolve() modifies the array
+        $lazy = $this->lazy;
+
+        foreach ($lazy as $option => $isLazy) {
+            $this->resolve($option);
+        }
+
+        return $this->options;
+    }
+
+    /**
+     * Equivalent to {@link has()}.
+     *
+     * @param  string $option The option name.
+     *
+     * @return Boolean Whether the option exists.
      *
      * @see ArrayAccess::offsetExists()
      */
     public function offsetExists($option)
     {
-        return isset($this->options[$option]);
+        return $this->has($option);
     }
 
     /**
-     * Returns the value of the given option.
+     * Equivalent to {@link get()}.
-     *
-     * After reading an option for the first time, this object becomes
      *
      * @param  string $option The option name.
      *
@@ -75,31 +286,11 @@ class Options implements ArrayAccess, Iterator
      */
     public function offsetGet($option)
     {
-        if (!array_key_exists($option, $this->options)) {
+        return $this->get($option);
-            throw new OutOfBoundsException('The option "' . $option . '" does not exist');
-        }
-
-        $this->resolved = true;
-
-        if ($this->options[$option] instanceof LazyOption) {
-            if ($this->lock[$option]) {
-                $conflicts = array_keys(array_filter($this->lock, function ($locked) {
-                    return $locked;
-                }));
-
-                throw new OptionDefinitionException('The options "' . implode('", "', $conflicts) . '" have a cyclic dependency');
-            }
-
-            $this->lock[$option] = true;
-            $this->options[$option] = $this->options[$option]->evaluate($this);
-            $this->lock[$option] = false;
-        }
-
-        return $this->options[$option];
     }
 
     /**
-     * Sets the value of a given option.
+     * Equivalent to {@link set()}.
      *
      * @param string $option The name of the option.
      * @param mixed  $value  The value of the option. May be a closure with a
@@ -109,41 +300,15 @@ class Options implements ArrayAccess, Iterator
      *                                   Once options are read, the container
      *                                   becomes immutable.
      *
-     * @see DefaultOptions::add()
      * @see ArrayAccess::offsetSet()
      */
     public function offsetSet($option, $value)
     {
-        // Setting is not possible once an option is read, because then lazy
+        $this->set($option, $value);
-        // options could manipulate the state of the object, leading to
-        // inconsistent results.
-        if ($this->resolved) {
-            throw new OptionDefinitionException('Options cannot be set after reading options');
-        }
-
-        $newValue = $value;
-
-        // If an option is a closure that should be evaluated lazily, store it
-        // inside a LazyOption instance.
-        if ($newValue instanceof \Closure) {
-            $reflClosure = new \ReflectionFunction($newValue);
-            $params = $reflClosure->getParameters();
-            $isLazyOption = count($params) >= 1 && null !== $params[0]->getClass() && __CLASS__ === $params[0]->getClass()->getName();
-
-            if ($isLazyOption) {
-                $currentValue = isset($this->options[$option]) ? $this->options[$option] : null;
-                $newValue = new LazyOption($newValue, $currentValue);
-            }
-
-            // Store locks for lazy options to detect cyclic dependencies
-            $this->lock[$option] = false;
-        }
-
-        $this->options[$option] = $newValue;
     }
 
     /**
-     * Removes an option with the given name.
+     * Equivalent to {@link remove()}.
      *
      * @param string $option The option name.
      *
@@ -155,12 +320,7 @@ class Options implements ArrayAccess, Iterator
      */
     public function offsetUnset($option)
     {
-        if ($this->resolved) {
+        $this->remove($option);
-            throw new OptionDefinitionException('Options cannot be unset after reading options');
-        }
-
-        unset($this->options[$option]);
-        unset($this->lock[$option]);
     }
 
     /**
@@ -202,4 +362,75 @@ class Options implements ArrayAccess, Iterator
     {
         reset($this->options);
     }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function count()
+    {
+        return count($this->options);
+    }
+
+    /**
+     * Evaluates the given option if it is a lazy option.
+     *
+     * The evaluated value is written into the options array. The closure for
+     * evaluating the option is discarded afterwards.
+     *
+     * @param string $option The option to evaluate.
+     *
+     * @throws OptionDefinitionException If the option has a cyclic dependency
+     *                                   on another option.
+     */
+    private function resolve($option)
+    {
+        if ($this->options[$option] instanceof LazyOption) {
+            if ($this->lock[$option]) {
+                $conflicts = array_keys(array_filter($this->lock, function ($locked)
+                {
+                    return $locked;
+                }));
+
+                throw new OptionDefinitionException('The options "' . implode('", "',
+                    $conflicts) . '" have a cyclic dependency.');
+            }
+
+            $this->lock[$option] = true;
+            $this->options[$option] = $this->options[$option]->evaluate($this);
+            $this->lock[$option] = false;
+
+            // The option now isn't lazy anymore
+            unset($this->lazy[$option]);
+        }
+    }
+
+    /**
+     * Returns whether the option is a lazy option closure.
+     *
+     * Lazy option closure expect an {@link Options} instance
+     * in their first parameter.
+     *
+     * @param  mixed $value The option value to test.
+     *
+     * @return Boolean Whether it is a lazy option closure.
+     */
+    private static function isEvaluatedLazily($value)
+    {
+        if (!$value instanceof Closure) {
+            return false;
+        }
+
+        $reflClosure = new \ReflectionFunction($value);
+        $params = $reflClosure->getParameters();
+
+        if (count($params) < 1) {
+            return false;
+        }
+
+        if (null === $params[0]->getClass()) {
+            return false;
+        }
+
+        return __CLASS__ === $params[0]->getClass()->getName();
+    }
 }

src/Symfony/Component/OptionsResolver/OptionsResolver.php

@@ -69,7 +69,7 @@ class OptionsResolver
     public function setDefaults(array $defaultValues)
     {
         foreach ($defaultValues as $option => $value) {
-            $this->defaultOptions[$option] = $value;
+            $this->defaultOptions->overload($option, $value);
             $this->knownOptions[$option] = true;
         }
 
@@ -95,8 +95,7 @@ class OptionsResolver
     public function replaceDefaults(array $defaultValues)
     {
         foreach ($defaultValues as $option => $value) {
-            unset($this->defaultOptions[$option]);
+            $this->defaultOptions->set($option, $value);
-            $this->defaultOptions[$option] = $value;
             $this->knownOptions[$option] = true;
         }
 
@@ -204,7 +203,7 @@ class OptionsResolver
      *
      * @param  array $options  The custom option values.
      *
-     * @return array  A list of options and their values.
+     * @return array A list of options and their values.
      *
      * @throws InvalidOptionsException   If any of the passed options has not
      *                                   been defined or does not contain an
@@ -222,16 +221,16 @@ class OptionsResolver
 
         // Override options set by the user
         foreach ($options as $option => $value) {
-            $combinedOptions[$option] = $value;
+            $combinedOptions->set($option, $value);
         }
 
         // Resolve options
-        $combinedOptions = iterator_to_array($combinedOptions);
+        $resolvedOptions = $combinedOptions->all();
 
         // Validate against allowed values
-        $this->validateOptionValues($combinedOptions);
+        $this->validateOptionValues($resolvedOptions);
 
-        return $combinedOptions;
+        return $resolvedOptions;
     }
 
     /**

src/Symfony/Component/OptionsResolver/Tests/OptionResolverTest.php

@@ -87,6 +87,7 @@ class OptionsResolverTest extends \PHPUnit_Framework_TestCase
 
         $this->resolver->setDefaults(array(
             'two' => function (Options $options) use ($test) {
+                /* @var \PHPUnit_Framework_TestCase $test */
                 $test->assertFalse(isset($options['one']));
 
                 return '2';
@@ -111,6 +112,7 @@ class OptionsResolverTest extends \PHPUnit_Framework_TestCase
 
         $this->resolver->setDefaults(array(
             'two' => function (Options $options) use ($test) {
+                /* @var \PHPUnit_Framework_TestCase $test */
                 $test->assertTrue(isset($options['one']));
 
                 return $options['one'] . '2';
@@ -154,6 +156,7 @@ class OptionsResolverTest extends \PHPUnit_Framework_TestCase
 
         $this->resolver->setDefaults(array(
             'one' => function (Options $options) use ($test) {
+                /* @var \PHPUnit_Framework_TestCase $test */
                 $test->fail('Previous closure should not be executed');
             },
         ));

src/Symfony/Component/OptionsResolver/Tests/OptionsTest.php

@@ -15,6 +15,9 @@ use Symfony\Component\OptionsResolver\Options;
 
 class OptionsTest extends \PHPUnit_Framework_TestCase
 {
+    /**
+     * @var Options
+     */
     private $options;
 
     protected function setUp()
@@ -41,12 +44,20 @@ class OptionsTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals(0, $this->options['foo']);
     }
 
+    public function testCountable()
+    {
+        $this->options->set('foo', 0);
+        $this->options->set('bar', 1);
+
+        $this->assertCount(2, $this->options);
+    }
+
     /**
      * @expectedException \OutOfBoundsException
      */
     public function testGetNonExisting()
     {
-        $this->options['foo'];
+        $this->options->get('foo');
     }
 
     /**
@@ -54,115 +65,137 @@ class OptionsTest extends \PHPUnit_Framework_TestCase
      */
     public function testSetNotSupportedAfterGet()
     {
-        $this->options['foo'] = 'bar';
+        $this->options->set('foo', 'bar');
-        $this->options['foo'];
+        $this->options->get('foo');
-        $this->options['foo'] = 'baz';
+        $this->options->set('foo', 'baz');
     }
 
     /**
      * @expectedException Symfony\Component\OptionsResolver\Exception\OptionDefinitionException
      */
-    public function testUnsetNotSupportedAfterGet()
+    public function testRemoveNotSupportedAfterGet()
     {
-        $this->options['foo'] = 'bar';
+        $this->options->set('foo', 'bar');
-        $this->options['foo'];
+        $this->options->get('foo');
-        unset($this->options['foo']);
+        $this->options->remove('foo');
     }
 
-    public function testLazyOption()
+    public function testSetLazyOption()
     {
         $test = $this;
 
-        $this->options['foo'] = function (Options $options) use ($test) {
+        $this->options->set('foo', function (Options $options) use ($test) {
            return 'dynamic';
-        };
+        });
 
-        $this->assertEquals('dynamic', $this->options['foo']);
+        $this->assertEquals('dynamic', $this->options->get('foo'));
     }
 
-    public function testLazyOptionWithEagerPreviousValue()
+    public function testSetDiscardsPreviousValue()
     {
         $test = $this;
 
         // defined by superclass
-        $this->options['foo'] = 'bar';
+        $this->options->set('foo', 'bar');
 
         // defined by subclass
-        $this->options['foo'] = function (Options $options, $previousValue) use ($test) {
+        $this->options->set('foo', function (Options $options, $previousValue) use ($test) {
-           $test->assertEquals('bar', $previousValue);
+            /* @var \PHPUnit_Framework_TestCase $test */
+            $test->assertNull($previousValue);
 
-           return 'dynamic';
+            return 'dynamic';
-        };
+        });
 
-        $this->assertEquals('dynamic', $this->options['foo']);
+        $this->assertEquals('dynamic', $this->options->get('foo'));
     }
 
-    public function testLazyOptionWithLazyPreviousValue()
+    public function testOverloadKeepsPreviousValue()
     {
         $test = $this;
 
         // defined by superclass
-        $this->options['foo'] = function (Options $options) {
+        $this->options->set('foo', 'bar');
-            return 'bar';
-        };
 
         // defined by subclass
-        $this->options['foo'] = function (Options $options, $previousValue) use ($test) {
+        $this->options->overload('foo', function (Options $options, $previousValue) use ($test) {
-           $test->assertEquals('bar', $previousValue);
+            /* @var \PHPUnit_Framework_TestCase $test */
-
+            $test->assertEquals('bar', $previousValue);
-           return 'dynamic';
-        };
-
-        $this->assertEquals('dynamic', $this->options['foo']);
-    }
-
-    public function testLazyOptionWithEagerDependency()
-    {
-        $test = $this;
-
-        $this->options['foo'] = 'bar';
-
-        $this->options['bam'] = function (Options $options) use ($test) {
-            $test->assertEquals('bar', $options['foo']);
 
             return 'dynamic';
-        };
+        });
 
-        $this->assertEquals('bar', $this->options['foo']);
+        $this->assertEquals('dynamic', $this->options->get('foo'));
-        $this->assertEquals('dynamic', $this->options['bam']);
     }
 
-    public function testLazyOptionWithLazyDependency()
+    public function testPreviousValueIsEvaluatedIfLazy()
     {
         $test = $this;
 
-        $this->options['foo'] = function (Options $options) {
+        // defined by superclass
+        $this->options->set('foo', function (Options $options) {
             return 'bar';
-        };
+        });
 
-        $this->options['bam'] = function (Options $options) use ($test) {
+        // defined by subclass
-            $test->assertEquals('bar', $options['foo']);
+        $this->options->overload('foo', function (Options $options, $previousValue) use ($test) {
+            /* @var \PHPUnit_Framework_TestCase $test */
+            $test->assertEquals('bar', $previousValue);
 
             return 'dynamic';
-        };
+        });
 
-        $this->assertEquals('bar', $this->options['foo']);
+        $this->assertEquals('dynamic', $this->options->get('foo'));
-        $this->assertEquals('dynamic', $this->options['bam']);
+    }
+
+    public function testLazyOptionCanAccessOtherOptions()
+    {
+        $test = $this;
+
+        $this->options->set('foo', 'bar');
+
+        $this->options->set('bam', function (Options $options) use ($test) {
+            /* @var \PHPUnit_Framework_TestCase $test */
+            $test->assertEquals('bar', $options->get('foo'));
+
+            return 'dynamic';
+        });
+
+        $this->assertEquals('bar', $this->options->get('foo'));
+        $this->assertEquals('dynamic', $this->options->get('bam'));
+    }
+
+    public function testLazyOptionCanAccessOtherLazyOptions()
+    {
+        $test = $this;
+
+        $this->options->set('foo', function (Options $options) {
+            return 'bar';
+        });
+
+        $this->options->set('bam', function (Options $options) use ($test) {
+            /* @var \PHPUnit_Framework_TestCase $test */
+            $test->assertEquals('bar', $options->get('foo'));
+
+            return 'dynamic';
+        });
+
+        $this->assertEquals('bar', $this->options->get('foo'));
+        $this->assertEquals('dynamic', $this->options->get('bam'));
     }
 
     /**
      * @expectedException Symfony\Component\OptionsResolver\Exception\OptionDefinitionException
      */
-    public function testLazyOptionDisallowCyclicDependencies()
+    public function testFailForCyclicDependencies()
     {
-        $this->options['foo'] = function (Options $options) {
+        $this->options->set('foo', function (Options $options) {
-            $options['bam'];
+            $options->get('bam');
-        };
+        });
 
-        $this->options['bam'] = function (Options $options) {
+        $this->options->set('bam', function (Options $options) {
-            $options['foo'];
+            $options->get('foo');
-        };
+        });
 
-        $this->options['foo'];
+        $this->options->get('foo');
     }
 }