From fb27de0f8a3e8fa03118eb6c0fa2c801f111491d Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Wed, 15 Feb 2012 00:06:34 +0100 Subject: [PATCH 1/3] [Config] cleanup --- .../Component/Config/Definition/BaseNode.php | 8 +- .../Builder/ArrayNodeDefinition.php | 76 +++++++-------- .../Config/Definition/Builder/ExprBuilder.php | 2 +- .../Config/Definition/Builder/NodeBuilder.php | 16 ++-- .../Definition/Builder/NodeDefinition.php | 92 +++++++++---------- .../Config/Definition/Builder/TreeBuilder.php | 7 +- .../Config/Definition/PrototypedArrayNode.php | 29 +++--- 7 files changed, 118 insertions(+), 112 deletions(-) diff --git a/src/Symfony/Component/Config/Definition/BaseNode.php b/src/Symfony/Component/Config/Definition/BaseNode.php index b76e9fc3ee..cd533e1964 100644 --- a/src/Symfony/Component/Config/Definition/BaseNode.php +++ b/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() { diff --git a/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php b/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php index f402a9627f..69064e2ec4 100644 --- a/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php +++ b/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php @@ -50,7 +50,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 +60,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 +78,16 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition */ public function prototype($type) { - $builder = $this->getNodeBuilder(); - $this->prototype = $builder->node(null, $type); - $this->prototype->parent = $this; - - return $this->prototype; + return $this->prototype = $this->getNodeBuilder()->node(null, $type)->setParent($this); } /** * 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 +100,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 +141,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 +150,19 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition * * array( * array('id' => 'my_name', 'foo' => 'bar'), - * ) + * ); * - * becomes + * 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 +220,12 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition } /** - * Append a node definition. + * Appends a node definition. * * $node = new ArrayNodeDefinition() * ->children() - * ->scalarNode('foo') - * ->scalarNode('baz') + * ->scalarNode('foo')->end() + * ->scalarNode('baz')->end() * ->end() * ->append($this->getBarNodeDefinition()) * ; @@ -254,10 +258,30 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition */ protected function createNode() { - if (null == $this->prototype) { + if (null === $this->prototype) { $node = new ArrayNode($this->name, $this->parent); + + foreach ($this->children as $child) { + $child->parent = $node; + $node->addChild($child->getNode()); + } } else { $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->setAddIfNotSet($this->addDefaults); @@ -283,28 +307,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; } diff --git a/src/Symfony/Component/Config/Definition/Builder/ExprBuilder.php b/src/Symfony/Component/Config/Definition/Builder/ExprBuilder.php index d185089c7e..7bf87db866 100644 --- a/src/Symfony/Component/Config/Definition/Builder/ExprBuilder.php +++ b/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 */ diff --git a/src/Symfony/Component/Config/Definition/Builder/NodeBuilder.php b/src/Symfony/Component/Config/Definition/Builder/NodeBuilder.php index e1dd6bee5e..3212040f96 100644 --- a/src/Symfony/Component/Config/Definition/Builder/NodeBuilder.php +++ b/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('baz') + * ->scalarNode('foo')->end() + * ->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 * diff --git a/src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php b/src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php index ede11d6121..0393378f2a 100644 --- a/src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php +++ b/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,6 +279,48 @@ 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 * diff --git a/src/Symfony/Component/Config/Definition/Builder/TreeBuilder.php b/src/Symfony/Component/Config/Definition/Builder/TreeBuilder.php index f16f439212..472cfdef99 100644 --- a/src/Symfony/Component/Config/Definition/Builder/TreeBuilder.php +++ b/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); - $this->root->setParent($this); - - return $this->root; + return $this->root = $builder->node($name, $type)->setParent($this); } /** diff --git a/src/Symfony/Component/Config/Definition/PrototypedArrayNode.php b/src/Symfony/Component/Config/Definition/PrototypedArrayNode.php index 0bebefae67..7d5fa1a098 100644 --- a/src/Symfony/Component/Config/Definition/PrototypedArrayNode.php +++ b/src/Symfony/Component/Config/Definition/PrototypedArrayNode.php @@ -39,6 +39,7 @@ class PrototypedArrayNode extends ArrayNode parent::__construct($name, $parent); $this->minNumberOfElements = 0; + $this->defaultValue = array(); } /** @@ -53,21 +54,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 - * with a prototype based node. + * This is useful when you have an indexed array that should be an + * 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 * - * becomes + * array( + * 'my_name' => array('foo' => 'bar'), + * ); * - * 'my_name' => array('foo' => 'bar') - * - * If $remove is false, the resulting array will still have the - * "'id' => 'my_name'" item in it. + * 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. * * @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 +126,7 @@ class PrototypedArrayNode extends ArrayNode */ public function getDefaultValue() { - return $this->defaultValue ?: array(); + return $this->defaultValue; } /** @@ -161,7 +166,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 From 6745b28b3d07a1014367a04949e2acc04dddc778 Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Wed, 15 Feb 2012 00:48:14 +0100 Subject: [PATCH 2/3] [Config] Throw exceptions on invalid definition --- .../Builder/ArrayNodeDefinition.php | 29 +++++++++++++++-- .../Definition/Builder/NodeDefinition.php | 2 ++ .../Exception/InvalidDefinitionException.php | 21 ++++++++++++ .../Config/Definition/PrototypedArrayNode.php | 3 +- .../Builder/ArrayNodeDefinitionTest.php | 32 +++++++++++++++++++ 5 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 src/Symfony/Component/Config/Definition/Exception/InvalidDefinitionException.php diff --git a/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php b/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php index 69064e2ec4..669ced43a6 100644 --- a/src/Symfony/Component/Config/Definition/Builder/ArrayNodeDefinition.php +++ b/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. @@ -259,13 +260,38 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition protected function createNode() { if (null === $this->prototype) { - $node = new ArrayNode($this->name, $this->parent); + if (null !== $this->key) { + throw new InvalidDefinitionException( + 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) { @@ -284,7 +310,6 @@ class ArrayNodeDefinition extends NodeDefinition implements ParentNodeDefinition $node->setPrototype($this->prototype->getNode()); } - $node->setAddIfNotSet($this->addDefaults); $node->setAllowNewKeys($this->allowNewKeys); $node->addEquivalentValue(null, $this->nullEquivalent); $node->addEquivalentValue(true, $this->trueEquivalent); diff --git a/src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php b/src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php index 0393378f2a..561143bc61 100644 --- a/src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php +++ b/src/Symfony/Component/Config/Definition/Builder/NodeDefinition.php @@ -325,6 +325,8 @@ abstract class NodeDefinition implements NodeParentInterface * 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(); diff --git a/src/Symfony/Component/Config/Definition/Exception/InvalidDefinitionException.php b/src/Symfony/Component/Config/Definition/Exception/InvalidDefinitionException.php new file mode 100644 index 0000000000..98310dae02 --- /dev/null +++ b/src/Symfony/Component/Config/Definition/Exception/InvalidDefinitionException.php @@ -0,0 +1,21 @@ + + * + * 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 + */ +class InvalidDefinitionException extends Exception +{ +} diff --git a/src/Symfony/Component/Config/Definition/PrototypedArrayNode.php b/src/Symfony/Component/Config/Definition/PrototypedArrayNode.php index 7d5fa1a098..e49d0d49f0 100644 --- a/src/Symfony/Component/Config/Definition/PrototypedArrayNode.php +++ b/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. @@ -158,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.'); } /** diff --git a/tests/Symfony/Tests/Component/Config/Definition/Builder/ArrayNodeDefinitionTest.php b/tests/Symfony/Tests/Component/Config/Definition/Builder/ArrayNodeDefinitionTest.php index 7b34b17bdd..337d8ee428 100644 --- a/tests/Symfony/Tests/Component/Config/Definition/Builder/ArrayNodeDefinitionTest.php +++ b/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); From 0a176ebc9882e1f4f02b7ff0bc9bbf5ab183095c Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Wed, 15 Feb 2012 00:49:15 +0100 Subject: [PATCH 3/3] [FrameworkBundle] Fix configuration errors --- .../FrameworkBundle/DependencyInjection/Configuration.php | 5 +---- .../Bundle/TwigBundle/DependencyInjection/Configuration.php | 3 +-- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php index bf3a414640..d96705d1ff 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php +++ b/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); }) diff --git a/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php index 74d52bdf98..4e5fe7ce6b 100644 --- a/src/Symfony/Bundle/TwigBundle/DependencyInjection/Configuration.php +++ b/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()