Skip to content

[config] cannotBeEmpty does not work with booleanNode #13736

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jaytaph opened this issue Feb 18, 2015 · 9 comments
Closed

[config] cannotBeEmpty does not work with booleanNode #13736

jaytaph opened this issue Feb 18, 2015 · 9 comments
Labels

Comments

@jaytaph
Copy link
Contributor

jaytaph commented Feb 18, 2015

An issue that somebody found:

       $rootNode
            ->children()
                ->booleanNode('use_mock')
                    ->isRequired()
                    ->cannotBeEmpty()
                ->end()

when using: use_mock: false in the config, it will throw a InvalidConfigurationException: The path "config.use_mock" cannot contain an empty value, but got false.

This will be thrown at line:

if (!$this->allowEmptyValue && empty($value)) {
where an empty() check on the boolean false will actually return true.

Is this a bug, or are there other ways to deal with booleanNodes in this situation?

Gist: https://gist.github.com/jaytaph/08906a3f38ca9f0867f8

(Same issue occurs when using a scalarNode() and using "0" as the config value)

@florianv
Copy link
Contributor

What do you want to achieve ? This is the normal behavior of the empty() function: false is considered empty like '0' and some others like the empty string.

For scalar nodes, if you want to invalidate empty strings but keep '0' valid, you can use this instead

$rootNode
    ->children()
        ->scalarNode('scalar')
        ->isRequired()
        ->validate()
            ->ifTrue(function ($v) {return '' === $v;})
            ->thenInvalid('Cannot be the empty string')
         ->end()
    ->end();

@jaytaph
Copy link
Contributor Author

jaytaph commented Feb 19, 2015

@florianv I understand how empty() works, and I understand how to work around this issue.

The main issue here, is that one does expect the functionality of "cannotBeEmpty" to work only in certain cases.. on a booleanNode, using either true or false are indeed values, and from a user point of view, you have complied to this rule. The fact that is uses empty() is an implementation detail (a very deep implementation detail), that is not obvious, documented or otherwise from the outside world.

I would propose that cannotBeEmpty() either be documented better to emphasise that cannotBeEmpty might use empty() as an implementation detail, and therefore even supplying a perfectly correct value would still result in an exception, OR change the implementation to make sure that bool(false) or bool(true) will indeed work, as the name implies.

(there are precedents in this matter btw.. there is a FormUtil:isEmpty that evaluates empty arrays, and boolean(0) to true. In fact, it will only return false when data === null or when data === empty string.
I think such a isEmpty() method this would be a much better match in this case as well..

@unkind
Copy link
Contributor

unkind commented Feb 19, 2015

OR change the implementation to make sure that bool(false) or bool(true) will indeed work, as the name implies

Can you clarify what does "empty bool" mean for you? What do you expect from this constraint?

I'd expect an exception as cannotBeEmpty makes no sense with bool for me, i.e. it looks like wrong config definition.

@jaytaph
Copy link
Contributor Author

jaytaph commented Feb 19, 2015

Why is that? Because cannotBeEmpty implies that you must specify a value. 'False' is indeed a value, even though an implementation decided otherwise.

It would be perfectly valid to ask the user to set an option on the config, which they must explicitly do so, without a default value. Imho, it makes no sense to me to be able to use cannotbeempty on a boolean, where you can only supply the value 'true'.

I would propose this to change. In a boolean context false is a perfectly valid option (in fact, it makes up 50% of the valid options).

@unkind
Copy link
Contributor

unkind commented Feb 19, 2015

Why is that? Because cannotBeEmpty implies that you must specify a value.

isRequired does it.

@jaytaph
Copy link
Contributor Author

jaytaph commented Feb 19, 2015

Yes. You are correct. Cannotbeempty implies that the value cannot be empty. Specifying a value false, is in fact suplying a value. The confusing part is that false is in this case threated as an empty. I understand that this is due to the implementation if empty(), but imho its not the same thing in this context

@SoboLAN
Copy link

SoboLAN commented Feb 26, 2015

I think @jaytaph is right and that this can be expanded to include any scalar value that the empty() function considers to be empty but is technically not (false, 0, 0.0, "0").

@wouterj
Copy link
Member

wouterj commented Feb 26, 2015

I agree that the current implementation is wrong. It's not only with false, but also with strings or numbers (the ones @SoboLAN showed). numerOfX: 0 should not be invalid for an integer node with ->cannotBeEmpty(). Currently, it is.

@xabbuh
Copy link
Member

xabbuh commented Jul 1, 2015

see #15170

fabpot added a commit that referenced this issue Aug 1, 2015
This PR was merged into the 2.3 branch.

Discussion
----------

[Config] type specific check for emptiness

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #13736
| License       | MIT
| Doc PR        |

Commits
-------

0199fbf [Config] type specific check for emptiness
@fabpot fabpot closed this as completed Aug 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants