[OptionsResolver] Slightly tweaked the performance of the Options class

src/Symfony/Component/OptionsResolver/LazyOption.php

@@ -33,22 +33,22 @@ class LazyOption
     /**
      * Creates a new lazy option.
      *
-     * @param Closure $closure The closure used for initializing the
-     *                               option value.
+     * @param \Closure $closure    The closure used for initializing the
+     *                             option value.
      * @param mixed $previousValue The previous value of the option. This
-     *                               value is passed to the closure when it is
-     *                               evaluated.
+     *                             value is passed to the closure when it is
+     *                             evaluated.
      *
      * @see evaluate()
      */
-    public function __construct(\Closure $closure, $previousValue)
+    public function __construct(\Closure $closure, $previousValue = null)
     {
         $this->closure = $closure;
         $this->previousValue = $previousValue;
     }
 
     /**
-     * Evaluates the underyling closure and returns its result.
+     * Evaluates the underlying closure and returns its result.
      *
      * The given Options instance is passed to the closure as first argument.
      * The previous default value set in the constructor is passed as second
@@ -60,10 +60,14 @@ public function __construct(\Closure $closure, $previousValue)
      */
     public function evaluate(Options $options)
     {
-        if ($this->previousValue instanceof self) {
-            $this->previousValue = $this->previousValue->evaluate($options);
+        $previousValue = $this->previousValue;
+        $closure = $this->closure;
+
+        if ($previousValue instanceof self) {
+            $previousValue = $this->previousValue->evaluate($options);
         }
 
-        return $this->closure->__invoke($options, $this->previousValue);
+        // Performs a bit better than __invoke() and call_user_func()
+        return $closure($options, $previousValue);
     }
 }

src/Symfony/Component/OptionsResolver/Options.php

@@ -139,23 +139,32 @@ public function overload($option, $value)
             throw new OptionDefinitionException('Options cannot be overloaded anymore once options have been read.');
         }
 
-        // 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 (self::isEvaluatedLazily($value)) {
-            $currentValue = isset($this->options[$option]) ? $this->options[$option] : null;
-            $value = new LazyOption($value, $currentValue);
+        if ($value instanceof \Closure) {
+            $reflClosure = new \ReflectionFunction($value);
+            $params = $reflClosure->getParameters();
 
-            // Store locks for lazy options to detect cyclic dependencies
-            $this->lock[$option] = false;
+            if (isset($params[0]) && null !== ($class = $params[0]->getClass()) && __CLASS__ === $class->name) {
+                $currentValue = isset($this->options[$option]) ? $this->options[$option] : null;
+                $value = new LazyOption($value, $currentValue);
 
-            // Store which options are lazy for more efficient resolving
-            $this->lazy[$option] = true;
+                // 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] = $value;
+
+                return;
+            }
         }
 
+        // Reset lazy flag and locks by default
+        unset($this->lock[$option]);
+        unset($this->lazy[$option]);
+
         $this->options[$option] = $value;
     }
 
@@ -251,8 +260,14 @@ public function all()
         // Create a copy because resolve() modifies the array
         $lazy = $this->lazy;
 
+        // Performance-wise this is slightly better than
+        // while (null !== $option = key($this->lazy))
         foreach ($lazy as $option => $isLazy) {
-            $this->resolve($option);
+            // When resolve() is called, potentially multiple lazy options
+            // are evaluated, so check again if the option is still lazy.
+            if (isset($this->lazy[$option])) {
+                $this->resolve($option);
+            }
         }
 
         return $this->options;
@@ -329,7 +344,7 @@ public function offsetUnset($option)
      */
     public function current()
     {
-        return $this->offsetGet($this->key());
+        return $this->get($this->key());
     }
 
     /**
@@ -385,52 +400,23 @@ public function count()
      */
     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;
-        }
+        if ($this->lock[$option]) {
+            $conflicts = array();
 
-        $reflClosure = new \ReflectionFunction($value);
-        $params = $reflClosure->getParameters();
+            foreach ($this->lock as $option => $locked) {
+                if ($locked) {
+                    $conflicts[] = $option;
+                }
+            }
 
-        if (count($params) < 1) {
-            return false;
+            throw new OptionDefinitionException('The options "' . implode('", "', $conflicts) . '" have a cyclic dependency.');
         }
 
-        if (null === $params[0]->getClass()) {
-            return false;
-        }
+        $this->lock[$option] = true;
+        $this->options[$option] = $this->options[$option]->evaluate($this);
+        $this->lock[$option] = false;
 
-        return __CLASS__ === $params[0]->getClass()->name;
+        // The option now isn't lazy anymore
+        unset($this->lazy[$option]);
     }
 }

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

@@ -0,0 +1,44 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\OptionsResolver\Tests;
+
+use Symfony\Component\OptionsResolver\LazyOption;
+use Symfony\Component\OptionsResolver\Options;
+
+/**
+ * @author Bernhard Schussek <bschussek@gmail.com>
+ */
+class LazyOptionTest extends \PHPUnit_Framework_TestCase
+{
+    public function testDontCacheEvaluatedPreviousValue()
+    {
+        $previousValue = new LazyOption(function (Options $options) {
+            return $options['foo'];
+        });
+
+        $lazyOption = new LazyOption(function (Options $options, $previousValue) {
+            return $previousValue;
+        }, $previousValue);
+
+        // If provided with two different option sets, two different results
+        // should be returned
+        $options1 = new Options();
+        $options1['foo'] = 'bar';
+
+        $this->assertSame('bar', $lazyOption->evaluate($options1));
+
+        $options2 = new Options();
+        $options2['foo'] = 'boo';
+
+        $this->assertSame('boo', $lazyOption->evaluate($options2));
+    }
+}