merged branch vicb/config/master/fix (PR #3365)

Commits
-------

0a176eb [FrameworkBundle] Fix configuration errors
6745b28 [Config] Throw exceptions on invalid definition
fb27de0 [Config] cleanup

Discussion
----------

[Config] Cleanup, error detection, fixes

see #3357

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

by stloyd at 2012-02-15T10:56:00Z

@vicb As you added new exceptions, IMO you should add some tests to cover it.

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

by vicb at 2012-02-15T10:56:50Z

good point, I'll do.

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

by vicb at 2012-02-15T13:49:44Z

@stloyd that was a great idea, I realized I had miss a case. It has been added and should be covered by UT + fixes made.

I am done with the fixes, should be ready to merge.

And time to give the `PrototypedArrayNode` some more usability now.

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

@@ -217,15 +217,14 @@ class Configuration implements ConfigurationInterface
                             ->fixXmlConfig('resource')
                             ->children()
                                 ->arrayNode('resources')
-                                    ->addDefaultsIfNotSet()
                                     ->defaultValue(array('FrameworkBundle:Form'))
+                                    ->prototype('scalar')->end()
                                     ->validate()
                                         ->ifTrue(function($v) {return !in_array('FrameworkBundle:Form', $v); })
                                         ->then(function($v){
                                             return array_merge(array('FrameworkBundle:Form'), $v);
                                         })
                                     ->end()
-                                    ->prototype('scalar')->end()
                                 ->end()
                             ->end()
                         ->end()
@@ -235,7 +234,6 @@ class Configuration implements ConfigurationInterface
                         ->arrayNode('assets_base_urls')
                             ->performNoDeepMerging()
                             ->addDefaultsIfNotSet()
-                            ->defaultValue(array('http' => array(), 'ssl' => array()))
                             ->beforeNormalization()
                                 ->ifTrue(function($v) { return !is_array($v); })
                                 ->then(function($v) { return array($v); })
@@ -290,7 +288,6 @@ class Configuration implements ConfigurationInterface
                                     ->arrayNode('base_urls')
                                         ->performNoDeepMerging()
                                         ->addDefaultsIfNotSet()
-                                        ->defaultValue(array('http' => array(), 'ssl' => array()))
                                         ->beforeNormalization()
                                             ->ifTrue(function($v) { return !is_array($v); })
                                             ->then(function($v) { return array($v); })

src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php

@@ -54,8 +54,8 @@ class Configuration implements ConfigurationInterface
                     ->fixXmlConfig('resource')
                     ->children()
                         ->arrayNode('resources')
-                            ->addDefaultsIfNotSet()
                             ->defaultValue(array('form_div_layout.html.twig'))
+                            ->prototype('scalar')->end()
                             ->setExample(array('MyBundle::form.html.twig'))
                             ->validate()
                                 ->ifTrue(function($v) { return !in_array('form_div_layout.html.twig', $v); })
@@ -63,7 +63,6 @@ class Configuration implements ConfigurationInterface
                                     return array_merge(array('form_div_layout.html.twig'), $v);
                                 })
                             ->end()
-                            ->prototype('scalar')->end()
                         ->end()
                     ->end()
                 ->end()

src/Symfony/Component/Config/Definition/BaseNode.php

@@ -56,7 +56,7 @@ abstract class BaseNode implements NodeInterface
     }
 
     /**
-     * Sets info message
+     * Sets info message.
      *
      * @param string $info The info text
      */
@@ -66,7 +66,7 @@ abstract class BaseNode implements NodeInterface
     }
 
     /**
-     * Returns info message
+     * Returns info message.
      *
      * @return string The info text
      */
@@ -78,7 +78,7 @@ abstract class BaseNode implements NodeInterface
     /**
      * Sets the example configuration for this node.
      *
-     * @param array $example
+     * @param string|array $example
      */
     public function setExample($example)
     {
@@ -88,7 +88,7 @@ abstract class BaseNode implements NodeInterface
     /**
      * Retrieves the example configuration for this node.
      *
-     * @return mixed The example
+     * @return string|array The example
      */
     public function getExample()
     {

src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php

@@ -13,6 +13,7 @@ namespace Symfony\Component\Config\Definition\Builder;
 
 use Symfony\Component\Config\Definition\ArrayNode;
 use Symfony\Component\Config\Definition\PrototypedArrayNode;
+use Symfony\Component\Config\Definition\Exception\InvalidDefinitionException;
 
 /**
  * This class provides a fluent interface for defining an array node.
@@ -50,7 +51,7 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition
     }
 
     /**
-     * Set a custom children builder
+     * Sets a custom children builder.
      *
      * @param NodeBuilder $builder A custom NodeBuilder
      */
@@ -60,7 +61,7 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition
     }
 
     /**
-     * Returns a builder to add children nodes
+     * Returns a builder to add children nodes.
      *
      * @return NodeBuilder
      */
@@ -78,16 +79,16 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition
      */
     public function prototype($type)
     {
-        $builder = $this->getNodeBuilder();
+        return $this->prototype = $this->getNodeBuilder()->node(null, $type)->setParent($this);
-        $this->prototype = $builder->node(null, $type);
-        $this->prototype->parent = $this;
-
-        return $this->prototype;
     }
 
     /**
      * Adds the default value if the node is not set in the configuration.
      *
+     * This method is applicable to concrete nodes only (not to prototype nodes).
+     * If this function has been called and the node is not set during the finalization
+     * phase, it's default value will be derived from its children default values.
+     *
      * @return ArrayNodeDefinition
      */
     public function addDefaultsIfNotSet()
@@ -100,6 +101,8 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition
     /**
      * Requires the node to have at least one element.
      *
+     * This method is applicable to prototype nodes only.
+     *
      * @return ArrayNodeDefinition
      */
     public function requiresAtLeastOneElement()
@@ -139,7 +142,7 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition
     }
 
     /**
-     * Set the attribute which value is to be used as key.
+     * Sets the attribute which value is to be used as key.
      *
      * This is useful when you have an indexed array that should be an
      * associative array. You can select an item from within the array
@@ -148,17 +151,19 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition
      *
      *     array(
      *         array('id' => 'my_name', 'foo' => 'bar'),
-     *     )
+     *     );
      *
      *   becomes
      *
      *     array(
      *         'my_name' => array('foo' => 'bar'),
-     *     )
+     *     );
      *
      * If you'd like "'id' => 'my_name'" to still be present in the resulting
      * array, then you can set the second argument of this method to false.
      *
+     * This method is applicable to prototype nodes only.
+     *
      * @param string  $name          The name of the key
      * @param Boolean $removeKeyItem Whether or not the key item should be removed.
      *
@@ -216,12 +221,12 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition
     }
 
     /**
-     * Append a node definition.
+     * Appends a node definition.
      *
      *     $node = new ArrayNodeDefinition()
      *         ->children()
-     *             ->scalarNode('foo')
+     *             ->scalarNode('foo')->end()
-     *             ->scalarNode('baz')
+     *             ->scalarNode('baz')->end()
      *         ->end()
      *         ->append($this->getBarNodeDefinition())
      *     ;
@@ -254,13 +259,57 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition
      */
     protected function createNode()
     {
-        if (null == $this->prototype) {
+        if (null === $this->prototype) {
-            $node = new ArrayNode($this->name, $this->parent);
+            if (null !== $this->key) {
-        } else {
+                throw new InvalidDefinitionException(
-            $node = new PrototypedArrayNode($this->name, $this->parent);
+                    sprintf('%s::useAttributeAsKey() is not applicable to concrete nodes.', __CLASS__)
+                );
             }
 
+            if (true === $this->atLeastOne) {
+                throw new InvalidDefinitionException(
+                    sprintf('%s::requiresAtLeastOneElement() is not applicable to concrete nodes.', __CLASS__)
+                );
+            }
+
+            if ($this->default) {
+                throw new InvalidDefinitionException(
+                    sprintf('%s::defaultValue() is not applicable to concrete nodes.', __CLASS__)
+                );
+            }
+
+            $node = new ArrayNode($this->name, $this->parent);
             $node->setAddIfNotSet($this->addDefaults);
+            
+            foreach ($this->children as $child) {
+                $child->parent = $node;
+                $node->addChild($child->getNode());
+            }
+        } else {
+            if ($this->addDefaults) {
+                throw new InvalidDefinitionException(
+                    sprintf('%s::addDefaultsIfNotSet() is not applicable to prototype nodes.', __CLASS__)
+                );
+            }
+
+            $node = new PrototypedArrayNode($this->name, $this->parent);
+
+            if (null !== $this->key) {
+                $node->setKeyAttribute($this->key, $this->removeKeyItem);
+            }
+
+            if (true === $this->atLeastOne) {
+                $node->setMinNumberOfElements(1);
+            }
+
+            if ($this->default) {
+                $node->setDefaultValue($this->defaultValue);
+            }
+
+            $this->prototype->parent = $node;
+            $node->setPrototype($this->prototype->getNode());
+        }
+
         $node->setAllowNewKeys($this->allowNewKeys);
         $node->addEquivalentValue(null, $this->nullEquivalent);
         $node->addEquivalentValue(true, $this->trueEquivalent);
@@ -283,28 +332,6 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition
             $node->setFinalValidationClosures($this->validation->rules);
         }
 
-        if (null == $this->prototype) {
-            foreach ($this->children as $child) {
-                $child->parent = $node;
-                $node->addChild($child->getNode());
-            }
-        } else {
-            if (null !== $this->key) {
-                $node->setKeyAttribute($this->key, $this->removeKeyItem);
-            }
-
-            if (true === $this->atLeastOne) {
-                $node->setMinNumberOfElements(1);
-            }
-
-            if (null !== $this->defaultValue) {
-                $node->setDefaultValue($this->defaultValue);
-            }
-
-            $this->prototype->parent = $node;
-            $node->setPrototype($this->prototype->getNode());
-        }
-
         return $node;
     }
 

src/Symfony/Component/Config/Definition/Builder/ExprBuilder.php

@@ -35,7 +35,7 @@ class ExprBuilder
     }
 
     /**
-     * Mark the expression as being always used.
+     * Marks the expression as being always used.
      *
      * @return ExprBuilder
      */

src/Symfony/Component/Config/Definition/Builder/NodeBuilder.php

@@ -36,7 +36,7 @@ class NodeBuilder implements NodeParentInterface
     }
 
     /**
-     * Set the parent node
+     * Set the parent node.
      *
      * @param ParentNodeDefinitionInterface $parent The parent node
      *
@@ -50,7 +50,7 @@ class NodeBuilder implements NodeParentInterface
     }
 
     /**
-     * Creates a child array node
+     * Creates a child array node.
      *
      * @param string $name The name of the node
      *
@@ -130,12 +130,14 @@ class NodeBuilder implements NodeParentInterface
     }
 
     /**
-     * Append a node definition.
+     * Appends a node definition.
+     *
+     * Usage:
      *
      *     $node = new ArrayNodeDefinition('name')
      *         ->children()
-     *             ->scalarNode('foo')
+     *             ->scalarNode('foo')->end()
-     *             ->scalarNode('baz')
+     *             ->scalarNode('baz')->end()
      *             ->append($this->getBarNodeDefinition())
      *         ->end()
      *     ;
@@ -160,7 +162,7 @@ class NodeBuilder implements NodeParentInterface
     }
 
     /**
-     * Add or override a node Type
+     * Adds or overrides a node Type.
      *
      * @param string $type The name of the type
      * @param string $class The fully qualified name the node definition class
@@ -175,7 +177,7 @@ class NodeBuilder implements NodeParentInterface
     }
 
     /**
-     * Returns the class name of the node definition
+     * Returns the class name of the node definition.
      *
      * @param string $type The node type
      *

src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php

@@ -52,7 +52,7 @@ abstract class NodeDefinition implements NodeParentInterface
     }
 
     /**
-     * Sets the parent node
+     * Sets the parent node.
      *
      * @param NodeParentInterface $parent The parent
      */
@@ -64,7 +64,7 @@ abstract class NodeDefinition implements NodeParentInterface
     }
 
     /**
-     * Sets info message
+     * Sets info message.
      *
      * @param string $info The info text
      *
@@ -78,9 +78,9 @@ abstract class NodeDefinition implements NodeParentInterface
     }
 
     /**
-     * Sets example configuration
+     * Sets example configuration.
      *
-     * @param example $example
+     * @param string|array $example
      *
      * @return NodeDefinition
      */
@@ -229,20 +229,6 @@ abstract class NodeDefinition implements NodeParentInterface
         return $this->defaultValue(false);
     }
 
-    /**
-     * Gets the builder for normalization rules.
-     *
-     * @return NormalizationBuilder
-     */
-    protected function normalization()
-    {
-        if (null === $this->normalization) {
-            $this->normalization = new NormalizationBuilder($this);
-        }
-
-        return $this->normalization;
-    }
-
     /**
      * Sets an expression to run before the normalization.
      *
@@ -265,20 +251,6 @@ abstract class NodeDefinition implements NodeParentInterface
         return $this;
     }
 
-    /**
-     * Gets the builder for validation rules.
-     *
-     * @return ValidationBuilder
-     */
-    protected function validation()
-    {
-        if (null === $this->validation) {
-            $this->validation = new ValidationBuilder($this);
-        }
-
-        return $this->validation;
-    }
-
     /**
      * Sets an expression to run for the validation.
      *
@@ -293,20 +265,6 @@ abstract class NodeDefinition implements NodeParentInterface
         return $this->validation()->rule();
     }
 
-    /**
-     * Gets the builder for merging rules.
-     *
-     * @return MergeBuilder
-     */
-    protected function merge()
-    {
-        if (null === $this->merge) {
-            $this->merge = new MergeBuilder($this);
-        }
-
-        return $this->merge;
-    }
-
     /**
      * Sets whether the node can be overwritten.
      *
@@ -321,10 +279,54 @@ abstract class NodeDefinition implements NodeParentInterface
         return $this;
     }
 
+    /**
+     * Gets the builder for validation rules.
+     *
+     * @return ValidationBuilder
+     */
+    protected function validation()
+    {
+        if (null === $this->validation) {
+            $this->validation = new ValidationBuilder($this);
+        }
+
+        return $this->validation;
+    }
+
+    /**
+     * Gets the builder for merging rules.
+     *
+     * @return MergeBuilder
+     */
+    protected function merge()
+    {
+        if (null === $this->merge) {
+            $this->merge = new MergeBuilder($this);
+        }
+
+        return $this->merge;
+    }
+
+    /**
+     * Gets the builder for normalization rules.
+     *
+     * @return NormalizationBuilder
+     */
+    protected function normalization()
+    {
+        if (null === $this->normalization) {
+            $this->normalization = new NormalizationBuilder($this);
+        }
+
+        return $this->normalization;
+    }
+
     /**
      * Instantiate and configure the node according to this definition
      *
      * @return NodeInterface $node The node instance
+     *
+     * @throws Symfony\Component\Config\Definition\Exception\InvalidDefinitionException When the definition is invalid
      */
     abstract protected function createNode();
 

src/Symfony/Component/Config/Definition/Builder/TreeBuilder.php

@@ -35,12 +35,9 @@ class TreeBuilder implements NodeParentInterface
      */
     public function root($name, $type = 'array', NodeBuilder $builder = null)
     {
-        $builder = null === $builder ? new NodeBuilder() : $builder;
+        $builder = $builder ?: new NodeBuilder();
 
-        $this->root = $builder->node($name, $type);
+        return $this->root = $builder->node($name, $type)->setParent($this);
-        $this->root->setParent($this);
-
-        return $this->root;
     }
 
     /**

src/Symfony/Component/Config/Definition/Exception/InvalidDefinitionException.php

@@ -0,0 +1,21 @@
+<?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\Config\Definition\Exception;
+
+/**
+ * Thrown when an error is detected in a node Definition.
+ *
+ * @author Victor Berchet <victor.berchet@suumit.com>
+ */
+class InvalidDefinitionException extends Exception
+{
+}

src/Symfony/Component/Config/Definition/PrototypedArrayNode.php

@@ -14,6 +14,7 @@ namespace Symfony\Component\Config\Definition;
 use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
 use Symfony\Component\Config\Definition\Exception\DuplicateKeyException;
 use Symfony\Component\Config\Definition\Exception\UnsetKeyException;
+use Symfony\Component\Config\Definition\Exception\Exception;
 
 /**
  * Represents a prototyped Array node in the config tree.
@@ -39,6 +40,7 @@ class PrototypedArrayNode extends ArrayNode
         parent::__construct($name, $parent);
 
         $this->minNumberOfElements = 0;
+        $this->defaultValue = array();
     }
 
     /**
@@ -53,21 +55,25 @@ class PrototypedArrayNode extends ArrayNode
     }
 
     /**
-     * The name of the attribute which value should be used as key.
+     * Sets the attribute which value is to be used as key.
      *
-     * This is only relevant for XML configurations, and only in combination
+     * This is useful when you have an indexed array that should be an
-     * with a prototype based node.
+     * associative array. You can select an item from within the array
+     * to be the key of the particular item. For example, if "id" is the
+     * "key", then:
      *
-     * For example, if "id" is the keyAttribute, then:
+     *     array(
-     *
+     *         array('id' => 'my_name', 'foo' => 'bar'),
-     *     array('id' => 'my_name', 'foo' => 'bar')
+     *     );
      *
      *  becomes
      *
-     *     'my_name' => array('foo' => 'bar')
+     *      array(
+     *          'my_name' => array('foo' => 'bar'),
+     *      );
      *
-     * If $remove is false, the resulting array will still have the
+     * If you'd like "'id' => 'my_name'" to still be present in the resulting
-     * "'id' => 'my_name'" item in it.
+     * array, then you can set the second argument of this method to false.
      *
      * @param string  $attribute The name of the attribute which value is to be used as a key
      * @param Boolean $remove Whether or not to remove the key
@@ -121,7 +127,7 @@ class PrototypedArrayNode extends ArrayNode
      */
     public function getDefaultValue()
     {
-        return $this->defaultValue ?: array();
+        return $this->defaultValue;
     }
 
     /**
@@ -153,7 +159,7 @@ class PrototypedArrayNode extends ArrayNode
      */
     public function addChild(NodeInterface $node)
     {
-        throw new \RuntimeException('A prototyped array node can not have concrete children.');
+        throw new Exception('A prototyped array node can not have concrete children.');
     }
 
     /**
@@ -161,7 +167,7 @@ class PrototypedArrayNode extends ArrayNode
      *
      * @param mixed $value
      *
-     * @return mixed The finalised value
+     * @return mixed The finalized value
      *
      * @throws UnsetKeyException
      * @throws InvalidConfigurationException if the node doesn't have enough children

tests/Symfony/Tests/Component/Config/Definition/Builder/ArrayNodeDefinitionTest.php

@@ -32,6 +32,38 @@ class ArrayNodeDefinitionTest extends \PHPUnit_Framework_TestCase
         $this->assertTrue(in_array($child, $this->getField($parent, 'children')));
     }
 
+    /**
+     * @expectedException Symfony\Component\Config\Definition\Exception\InvalidDefinitionException
+     * @dataProvider providePrototypeNodeSpecificCalls
+     */
+    public function testPrototypeNodeSpecificOption($method, $args)
+    {
+        $node = new ArrayNodeDefinition('root');
+
+        call_user_method_array($method, $node, $args);
+
+        $node->getNode();
+    }
+
+    public function providePrototypeNodeSpecificCalls()
+    {
+        return array(
+            array('defaultValue', array(array())),
+            array('requiresAtLeastOneElement', array()),
+            array('useAttributeAsKey', array('foo'))
+        );
+    }
+
+    /**
+     * @expectedException Symfony\Component\Config\Definition\Exception\InvalidDefinitionException
+     */
+    public function testConcreteNodeSpecificOption()
+    {
+        $node = new ArrayNodeDefinition('root');
+        $node->addDefaultsIfNotSet()->prototype('array');
+        $node->getNode();
+    }
+
     protected function getField($object, $field)
     {
         $reflection = new \ReflectionProperty($object, $field);