merged branch jeanmonod/config-with-numeric-checks (PR symfony#4714)

Commits
-------

71db836 Better config validation handling for numerical values:  * New node type Integer and Float  * New expressions: min() and max()

Discussion
----------

[2.2] [Config] Better handling for numerical values:

* New node type Integer and Float
* New expressions: ifLessThan(), ifGreaterThan(), ifInRange(), ifNotInRange()

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

by fabpot at 2012-07-03T08:50:22Z

As I said on PR symfony#4713, adding more method clutters the API without any big benefits. I'm -1 on the PR.

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

by jeanmonod at 2012-07-03T08:54:36Z

I have been discuss it with @schmittjoh at the sflive, he was thinking it could be a good addition.
IMHO I think that if we want to encourage the usage of bundle configuration validation, we should make it as easy as possible to use...

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

by jeanmonod at 2012-07-03T08:59:42Z

A real life example:

    ->scalarNode('max_nb_items')
        ->validate()
            ->ifTrue(function($v){
                return !is_int($v) || (is_int($v) && $v<1);
            })
            ->thenInvalid('Must be a positive integer')
        ->end()
    ->end()

could be replaced by

    ->integerNode('max_nb_items')
        ->validate()
            ->ifLessThan(1);
            ->thenInvalid('Must be a positive integer')
        ->end()
    ->end()

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

by gnutix at 2012-07-03T09:03:06Z

I agree with @jeanmonod on this matter, the bundle configuration validation is already kind of a hassle to understand (and read), so it would be a good addition IMHO.

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

by fabpot at 2012-07-03T10:54:32Z

@schmittjoh What's your point of view?

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

by schmittjoh at 2012-07-03T14:10:37Z

The integer and float nodes are valuable additions imo which I wanted to add myself several times but simply did not have the time for.

As for the changes to the expression builder, I was not really passionate about them in Paris, but I did not mind either way. However, looking at this PR, I think they would be better implemented as methods on the definition builders, and validated directly by the nodes:

```php
$builder->integerNode('foo')->range(1, 4)->end();
$builder->integerNode('foo')->mustBeGreaterThan(5)->end();
```

This will also allow for these constraints to be introspected and added to generated documentation.

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

by fabpot at 2012-07-03T17:57:25Z

@jeanmonod Can you take into account the comments by @schmittjoh?

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

by jeanmonod at 2012-07-03T19:40:24Z

@fabpot Yes, I will try to move those 4 checks.

@schmittjoh If I put those tests into the ScalarNodeDefinition did you think it's ok? And did I have to make anything special for the documentation introspection?

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

by schmittjoh at 2012-07-03T19:56:00Z

You can take a look at the EnumNodeDefinition, and the EnumNode. They are
pretty simple, and should give you a good idea of how to implement it.

On Tue, Jul 3, 2012 at 9:46 PM, Jeanmonod David <
[email protected]
> wrote:

> @fabpot Yes, I will try to move those 4 checks.
>
> @schmittjoh If I put those tests into the ScalarNodeDefinition did you
> think it's ok? And did I have to make anything special for the
> documentation introspection?
>
> ---
> Reply to this email directly or view it on GitHub:
> symfony#4714 (comment)
>

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

by jeanmonod at 2012-07-03T21:37:18Z

OK, I just refactor as requested. At the end, I didn't add the range() check. It can be easily done by chaining min and max, like this:

    $builder->integerNode('foo')->min(1)->max(4)->end();

@schmittjoh can you have a look?

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

by schmittjoh at 2012-07-03T21:48:17Z

Have you tested the builder API? Did you maybe forget to commit something?

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

by jeanmonod at 2012-07-03T21:52:45Z

Yes you are right, I forget the definition

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

by jeanmonod at 2012-07-03T22:15:45Z

OK, I realize now that I misunderstood the concept. I was thinking that a node was able to do self validation. But no, I will have to move my code to the node definition. So let's wait for a new commit...

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

by jeanmonod at 2012-07-06T06:13:55Z

@schmittjoh I just push the move to definition and the new abstract class Numeric. Can you review it?

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

by jeanmonod at 2012-07-10T05:12:59Z

@schmittjoh, @fabpot
I fix all the mention points, can you have a look at the final result?

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

by schmittjoh at 2012-07-10T06:38:20Z

There are still some excessive blank lines if you want to be perfect, but overall looks good now.

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

by jeanmonod at 2012-07-10T07:05:54Z

@schmittjoh I think the comments of @Baachi are not well placed in the diff. I execute php-cs-fix on all code, so level of perfectness is already good ;)

@fabpot Do you want some more complements before merging?

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

by fabpot at 2012-07-10T07:07:21Z

@jeanmonod I'm going to review the code once more and it will be merged for 2.2. Thanks for your work.

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

by fabpot at 2012-09-18T13:58:48Z

@jeanmonod Can you squash your commits before I merge? Thanks.

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

by jeanmonod at 2012-09-18T14:36:59Z

@fabpot Squash done

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

by fabpot at 2012-09-19T04:07:13Z

@jeanmonod One last thing: can you submit a PR on symfony/symfony-docs that update the documentation with the new possibilities?

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

by jeanmonod at 2012-09-20T05:32:01Z

@fabpot OK, Documentation PR done here: symfony/symfony-docs#1732

Author: fabpot

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

@@ -0,0 +1,32 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Config\Definition\Builder;
+
+use Symfony\Component\Config\Definition\FloatNode;
+
+/**
+ * This class provides a fluent interface for defining a float node.
+ *
+ * @author Jeanmonod David <[email protected]>
+ */
+class FloatNodeDefinition extends NumericNodeDefinition
+{
+    /**
+     * Instantiate a Node
+     *
+     * @return FloatNode The node
+     */
+    protected function instantiateNode()
+    {
+        return new FloatNode($this->name, $this->parent, $this->min, $this->max);
+    }
+}

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

@@ -0,0 +1,32 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Config\Definition\Builder;
+
+use Symfony\Component\Config\Definition\IntegerNode;
+
+/**
+ * This class provides a fluent interface for defining an integer node.
+ *
+ * @author Jeanmonod David <[email protected]>
+ */
+class IntegerNodeDefinition extends NumericNodeDefinition
+{
+    /**
+     * Instantiate a Node
+     *
+     * @return IntegerNode The node
+     */
+    protected function instantiateNode()
+    {
+        return new IntegerNode($this->name, $this->parent, $this->min, $this->max);
+    }
+}

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

@@ -31,6 +31,8 @@ public function __construct()
             'variable'    => __NAMESPACE__.'\\VariableNodeDefinition',
             'scalar'      => __NAMESPACE__.'\\ScalarNodeDefinition',
             'boolean'     => __NAMESPACE__.'\\BooleanNodeDefinition',
+            'integer'     => __NAMESPACE__.'\\IntegerNodeDefinition',
+            'float'       => __NAMESPACE__.'\\FloatNodeDefinition',
             'array'       => __NAMESPACE__.'\\ArrayNodeDefinition',
             'enum'        => __NAMESPACE__.'\\EnumNodeDefinition',
         );
@@ -86,6 +88,30 @@ public function booleanNode($name)
         return $this->node($name, 'boolean');
     }
 
+    /**
+     * Creates a child integer node.
+     *
+     * @param string $name the name of the node
+     *
+     * @return IntegerNodeDefinition The child node
+     */
+    public function integerNode($name)
+    {
+        return $this->node($name, 'integer');
+    }
+
+    /**
+     * Creates a child float node.
+     *
+     * @param string $name the name of the node
+     *
+     * @return FloatNodeDefinition The child node
+     */
+    public function floatNode($name)
+    {
+        return $this->node($name, 'float');
+    }
+
     /**
      * Creates a child EnumNode.
      *

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

@@ -0,0 +1,58 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Config\Definition\Builder;
+
+/**
+ * Abstract class that contain common code of integer and float node definition.
+ *
+ * @author David Jeanmonod <[email protected]>
+ */
+abstract class NumericNodeDefinition extends ScalarNodeDefinition
+{
+
+    protected $min;
+    protected $max;
+
+    /**
+     * Ensure the value is smaller than the given reference
+     *
+     * @param mixed $max
+     *
+     * @return NumericNodeDefinition
+     */
+    public function max($max)
+    {
+        if (isset($this->min) && $this->min > $max) {
+            throw new \InvalidArgumentException(sprintf('You cannot define a max(%s) as you already have a min(%s)', $max, $this->min));
+        }
+        $this->max = $max;
+
+        return $this;
+    }
+
+    /**
+     * Ensure the value is bigger than the given reference
+     *
+     * @param mixed $min
+     *
+     * @return NumericNodeDefinition
+     */
+    public function min($min)
+    {
+        if (isset($this->max) && $this->max < $min) {
+            throw new \InvalidArgumentException(sprintf('You cannot define a min(%s) as you already have a max(%s)', $min, $this->max));
+        }
+        $this->min = $min;
+
+        return $this;
+    }
+}

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

@@ -0,0 +1,44 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Config\Definition;
+
+use Symfony\Component\Config\Definition\Exception\InvalidTypeException;
+
+/**
+ * This node represents a float value in the config tree.
+ *
+ * @author Jeanmonod David <[email protected]>
+ */
+class FloatNode extends NumericNode
+{
+    /**
+     * {@inheritDoc}
+     */
+    protected function validateType($value)
+    {
+        // Integers are also accepted, we just cast them
+        if (is_int($value)) {
+            $value = (float) $value;
+        }
+
+        if (!is_float($value)) {
+            $ex = new InvalidTypeException(sprintf(
+                'Invalid type for path "%s". Expected float, but got %s.',
+                $this->getPath(),
+                gettype($value)
+            ));
+            $ex->setPath($this->getPath());
+
+            throw $ex;
+        }
+    }
+}

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

@@ -0,0 +1,39 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Config\Definition;
+
+use Symfony\Component\Config\Definition\Exception\InvalidTypeException;
+
+/**
+ * This node represents an integer value in the config tree.
+ *
+ * @author Jeanmonod David <[email protected]>
+ */
+class IntegerNode extends NumericNode
+{
+    /**
+     * {@inheritDoc}
+     */
+    protected function validateType($value)
+    {
+        if (!is_int($value)) {
+            $ex = new InvalidTypeException(sprintf(
+                'Invalid type for path "%s". Expected int, but got %s.',
+                $this->getPath(),
+                gettype($value)
+            ));
+            $ex->setPath($this->getPath());
+
+            throw $ex;
+        }
+    }
+}

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

@@ -0,0 +1,56 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Config\Definition;
+
+use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
+
+/**
+ * This node represents a numeric value in the config tree
+ *
+ * @author David Jeanmonod <[email protected]>
+ */
+class NumericNode extends ScalarNode
+{
+    protected $min;
+    protected $max;
+
+    public function __construct($name, NodeInterface $parent = null, $min = null, $max = null)
+    {
+        parent::__construct($name, $parent);
+        $this->min = $min;
+        $this->max = $max;
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    protected function finalizeValue($value)
+    {
+        $value = parent::finalizeValue($value);
+
+        $errorMsg = null;
+        if (isset($this->min) && $value < $this->min) {
+            $errorMsg = sprintf('The value %s is too small for path "%s". Should be greater than: %s', $value, $this->getPath(), $this->min);
+        }
+        if (isset($this->max) && $value > $this->max) {
+            $errorMsg = sprintf('The value %s is too big for path "%s". Should be less than: %s', $value, $this->getPath(), $this->max);
+        }
+        if (isset($errorMsg)) {
+            $ex = new InvalidConfigurationException($errorMsg);
+            $ex->setPath($this->getPath());
+            throw $ex;
+        }
+
+        return $value;
+    }
+
+}

src/Symfony/Component/Config/Tests/Definition/Builder/ExprBuilderTest.php

@@ -171,7 +171,7 @@ protected function getTestBuilder()
     /**
      * Close the validation process and finalize with the given config
      * @param TreeBuilder $testBuilder The tree builder to finalize
-     * @param array $config The config you want to use for the finalization, if nothing provided
+     * @param array       $config      The config you want to use for the finalization, if nothing provided
      *                       a simple array('key'=>'value') will be used
      * @return array The finalized config values
      */
@@ -200,6 +200,7 @@ protected function returnClosure($val)
 
     /**
      * Assert that the given test builder, will return the given value
+     *
      * @param mixed       $value       The value to test
      * @param TreeBuilder $treeBuilder The tree builder to finalize
      * @param mixed       $config      The config values that new to be finalized

src/Symfony/Component/Config/Tests/Definition/Builder/NodeBuilderTest.php

@@ -76,6 +76,17 @@ public function testNodeTypesAreNotCaseSensitive()
 
         $this->assertEquals(get_class($node1), get_class($node2));
     }
+
+    public function testNumericNodeCreation()
+    {
+        $builder = new NodeBuilder();
+
+        $node = $builder->integerNode('foo')->min(3)->max(5);
+        $this->assertEquals('Symfony\Component\Config\Definition\Builder\IntegerNodeDefinition', get_class($node));
+
+        $node = $builder->floatNode('bar')->min(3.0)->max(5.0);
+        $this->assertEquals('Symfony\Component\Config\Definition\Builder\FloatNodeDefinition', get_class($node));
+    }
 }
 
 class SomeNodeDefinition extends BaseVariableNodeDefinition