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

jeanmonod · 2012-07-03T05:37:17Z

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

fabpot · 2012-07-03T08:50:22Z

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

jeanmonod · 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...

jeanmonod · 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()

gnutix · 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.

fabpot · 2012-07-03T10:54:32Z

@schmittjoh What's your point of view?

schmittjoh · 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:

$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.

fabpot · 2012-07-03T17:57:25Z

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

jeanmonod · 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?

schmittjoh · 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 <
reply@reply.github.com

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:
#4714 (comment)

jeanmonod · 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?

schmittjoh · 2012-07-03T21:46:02Z

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

@@ -28,6 +30,9 @@
 */
 class ScalarNode extends VariableNode
 {
+    protected $min;


Can you move all of this code to an abstract sub-class, NumericNode for example?

Yes, this can be done, but is it really useful? All scalars can all handle the min max, something like:

$builder->stringNode('word_range')->min('a')->max('d')->end();

Can be valid (even if nobody is never going to use it...)

As it does not apply to all scalars, I think it should be a sub-class. For strings, I think a regular expression would be more intuitive :)

schmittjoh · 2012-07-03T21:48:17Z

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

jeanmonod · 2012-07-03T21:52:45Z

Yes you are right, I forget the definition

schmittjoh · 2012-07-03T21:56:17Z

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

+ *
+ * @author Jeanmonod David <david.jeanmonod@gmail.com>
+ */
+class FloatNode extends ScalarNode


Should we also allow integers here, and cast them?

Yes, why not. Where is the best place for doing that? In the finalizeValue() method?

normalizeValue would be the place.

I'm not sure about that. In BaseNode::normalize() it first validate the type and then it call the normalizeValue(), so the type check fail before I'm able to cast it to float...
What I can do is to be less restrictive in the validateType(). What do you prefer?

jeanmonod · 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...

stof · 2012-07-03T22:48:46Z

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

@@ -136,6 +136,8 @@ public function ifNotInArray(array $array)
        return $this;
    }

+
+


please remove these extra empty lines

jeanmonod · 2012-07-06T06:13:55Z

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

Baachi · 2012-07-06T06:25:02Z

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

+            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;


Missing blank line here.

jeanmonod · 2012-07-10T05:12:59Z

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

schmittjoh · 2012-07-10T06:38:20Z

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

jeanmonod · 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?

fabpot · 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.

fabpot · 2012-09-18T13:58:48Z

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

pborreli · 2012-09-18T14:01:16Z

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

+class FloatNodeDefinition extends NumericNodeDefinition
+{
+    /**
+     * Instantiate a Node


need a trailing dot.

* New node type Integer and Float * New expressions: min() and max()

jeanmonod · 2012-09-18T14:36:59Z

@fabpot Squash done

Tobion · 2012-09-18T16:13:56Z

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

+     *
+     * @return NumericNodeDefinition
+     */
+    public function max($max)


missing phpdoc for \InvalidArgumentException (same on min).

fabpot · 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?

jeanmonod · 2012-09-20T05:32:01Z

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

@schmittjoh

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 #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 < reply@reply.github.com > 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: > #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

@schmittjoh

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 #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 < reply@reply.github.com > 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/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

schmittjoh reviewed Jul 3, 2012
View reviewed changes

stof reviewed Jul 3, 2012
View reviewed changes

Baachi reviewed Jul 6, 2012
View reviewed changes

pborreli reviewed Sep 18, 2012
View reviewed changes

Better config validation handling for numerical values:

71db836

* New node type Integer and Float * New expressions: min() and max()

Tobion reviewed Sep 18, 2012
View reviewed changes

jeanmonod mentioned this pull request Sep 20, 2012

[Config] Documentation of the new numerical type handling symfony/symfony-docs#1732

Merged

fabpot merged commit 71db836 into symfony:master Sep 20, 2012

svdv22 mentioned this pull request Jul 5, 2024

[Config] Add support for stringNode in config component #57660

Closed

		@@ -136,6 +136,8 @@ public function ifNotInArray(array $array)
		return $this;
		}

Uh oh!

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

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

Uh oh!

Conversation

jeanmonod commented Jul 3, 2012

Uh oh!

fabpot commented Jul 3, 2012

Uh oh!

jeanmonod commented Jul 3, 2012

Uh oh!

jeanmonod commented Jul 3, 2012

Uh oh!

gnutix commented Jul 3, 2012

Uh oh!

fabpot commented Jul 3, 2012

Uh oh!

schmittjoh commented Jul 3, 2012

Uh oh!

fabpot commented Jul 3, 2012

Uh oh!

jeanmonod commented Jul 3, 2012

Uh oh!

schmittjoh commented Jul 3, 2012

Uh oh!

jeanmonod commented Jul 3, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmittjoh commented Jul 3, 2012

Uh oh!

jeanmonod commented Jul 3, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeanmonod commented Jul 3, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeanmonod commented Jul 6, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeanmonod commented Jul 10, 2012

Uh oh!

schmittjoh commented Jul 10, 2012

Uh oh!

jeanmonod commented Jul 10, 2012

Uh oh!

fabpot commented Jul 10, 2012

Uh oh!

fabpot commented Sep 18, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeanmonod commented Sep 18, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Sep 19, 2012

Uh oh!

jeanmonod commented Sep 20, 2012

Uh oh!

Uh oh!