[DependencyInjection] Fix DefinitionDecorator::getArgument() for replacements

jmikola · jmikola · commit 80f0b980baa4 · 2011-10-27T18:03:51.000-04:00
While Definition::getArgument() could be used to fetch replaced values, it relied upon bad comparison logic (e.g. "index_1" &gt; 1). Additionally, storing original arguments and replacements in the same array makes Definition::getArguments()'s bounds check unreliable. A single argument and its replacement would count twice, allowing getArgument(2) to pass the bounds check and result in an array index error.

With this new method, fetching of replacement arguments is more straightforward and bounds checking functions as it should.
diff --git a/src/Symfony/Component/DependencyInjection/DefinitionDecorator.php b/src/Symfony/Component/DependencyInjection/DefinitionDecorator.php
@@ -146,6 +146,33 @@ public function setPublic($boolean)
         return parent::setPublic($boolean);
     }
 
+    /**
+     * Gets an argument to pass to the service constructor/factory method.
+     *
+     * If replaceArgument() has been used to replace an argument, this method
+     * will return the replacement value.
+     *
+     * @param integer $index
+     *
+     * @return mixed The argument value
+     *
+     * @api
+     */
+    public function getArgument($index)
+    {
+        if (array_key_exists('index_'.$index, $this->arguments)) {
+            return $this->arguments['index_'.$index];
+        }
+
+        $lastIndex = count(array_filter(array_keys($this->arguments), 'is_int')) - 1;
+
+        if ($index < 0 || $index > $lastIndex) {
+            throw new \OutOfBoundsException(sprintf('The index "%d" is not in the range [0, %d].', $index, $lastIndex));
+        }
+
+        return $this->arguments[$index];
+    }
+
     /**
      * You should always use this method when overwriting existing arguments
      * of the parent definition.
diff --git a/tests/Symfony/Tests/Component/DependencyInjection/DefinitionDecoratorTest.php b/tests/Symfony/Tests/Component/DependencyInjection/DefinitionDecoratorTest.php
@@ -79,4 +79,32 @@ public function testReplaceArgumentShouldRequireIntegerIndex()
 
         $def->replaceArgument('0', 'foo');
     }
+
+    public function testReplaceArgument()
+    {
+        $def = new DefinitionDecorator('foo');
+
+        $def->setArguments(array(0 => 'foo', 1 => 'bar'));
+        $this->assertEquals('foo', $def->getArgument(0));
+        $this->assertEquals('bar', $def->getArgument(1));
+
+        $this->assertSame($def, $def->replaceArgument(1, 'baz'));
+        $this->assertEquals('foo', $def->getArgument(0));
+        $this->assertEquals('baz', $def->getArgument(1));
+
+        $this->assertEquals(array(0 => 'foo', 1 => 'bar', 'index_1' => 'baz'), $def->getArguments());
+    }
+
+    /**
+     * @expectedException OutOfBoundsException
+     */
+    public function testGetArgumentShouldCheckBounds()
+    {
+        $def = new DefinitionDecorator('foo');
+
+        $def->setArguments(array(0 => 'foo'));
+        $def->replaceArgument(0, 'foo');
+
+        $def->getArgument(1);
+    }
 }
