opened
yyaremenko
username_0
4. Create a controller, submit the form described above with data
`['title' => '']`

**Expectation:**
Form is invalid and has a proper error.

**Reality:**
The form is valid.

**The Reason**  
Currently, the code in **LengthValidator** only has the following lines mentioning **allowEmptyString**:

`
        if (null !== $constraint->min && null === $constraint->allowEmptyString) {
            @trigger_error(sprintf('Using the "%s" constraint with the "min" option without setting the "allowEmptyString" one is deprecated and defaults to true. In 5.0, it will become optional and default to false.', Length::class), E_USER_DEPRECATED);
        }

        if (null === $value || ('' === $value && ($constraint->allowEmptyString ?? true))) {
            return;
        }
`
The first block only triggers error if 'min' option is provided. In my example above 'min' is not provided.

The second block inner part (return) is not reached as well, because
`null === $value` evaluates to `false`
`('' === $value && ($constraint->allowEmptyString ?? true)))`
evaluates to `false`:
1) `'' === $value` evaluates to `true`
2) `$constraint->allowEmptyString ?? true` evaluates to `false`
3) `true && false` evaluates to `false`

Further down the code, **allowEmptyString** is never checked. Only **charset**, **min** and **max** validations are checked.

**Possible Solution** 
Write code which validates if string is not empty if **allowEmptyString** is false.
**Also, write tests.**
[Validator] LengthValidator: allowEmptyString option set to false allows empty strings
issue
created
xabbuh
username_1
I think this was done on purpose (see https://github.com/symfony/symfony/pull/31528#issuecomment-493498538). If you don't want to allow empty strings, it's expected to set the `min` option to `1`.
comment
created
yyaremenko
username_0
I believe it is illogical and confusing to have ```allowEmptyString```, option which can only work in pair with ```min``` option, but not to have an error thrown when ```allowEmptyString``` is used without ```min``` option.
Also, nothing about the dependence (```allowEmptyString only works when min is given```) is said in the documentation. I would propose one of the following solutions:

1. Either fix the code so that ```allowEmptyString``` option could work as a stand-alone option.
2. Or, fix the code so that an error is thrown when ```allowEmptyString``` is used without ```min``` option, and also update the documentation correspondingly.
comment
created
yyaremenko
username_0
Otherwise, you end up in the current situation: you provide a constraint option, and that option is just silently ignored by the validation, and you don't have any idea why.
comment
created
yyaremenko
username_0
Also, Christian, could you please give me a hint where can I see the tests for currently available validators? I can't find any, maybe I'm missing a thing, maybe they are in some non-master branch or something.
comment
created
xabbuh
username_1
I submitted a documentation PR to clarify the behaviour: symfony/symfony-docs#13668
comment
created
xabbuh
username_1
@username_0 By the way, you can find the tests here: https://github.com/symfony/symfony/tree/master/src/Symfony/Component/Validator/Tests/Constraints
comment
created
xabbuh
username_1
see also #36818
comment
created
yyaremenko
username_0
@username_1 thank you so much!
Could you also give a comment on the second issue (an empty string being converted to null and thus not handled properly by the LengthValidator)? The problem I encounter right now is that I use the Form component to validate incoming JSON (unserialized to array) data (I write a JSON API), and as I write a test which sends an empty string for a field, which has Length constraint with **min = 3, max = 10 and allowEmptyString = false**, the test passes (but it must fail, as the string is empty). The test only fails when I add NotBlank() constraint - but this is, again, illogical, as the Length constraint with the config I described must not allow empty strings.
comment
created
xabbuh
username_1
Did you set the `empty_data` option to the empty string?
comment
created
yyaremenko
username_0
No, I did not. This is not the solution of the problem.

The question here is that the Length validator does not work properly out of the box. If I set a Length constraint with the parameters I described earlier, I expect to get an error whenever I submit an empty string.

Like, if I say '**do not allow an empty string**' I expect a validator to return an **error when I submit an empty string**. I can't imagine anything more obvious than that.

If, in addition to the configuration described earlier, I also have to explicitly set **empty_data** option  to an empty string - this means the validator is corrupt. Adding an 'external' option to make validator work properly is a workaround, not a fix.

If you can't / don't want to fix this - at least this is expected to be described in the documentation, like 'allowEmptyString only works with min parameter and if empty_data is set to an empty string'.
comment
created
xabbuh
username_1
This is not related to the validator, but to how the Form component works. If you use the validator without the Form component (e.g. when using it in an API context where the input for the validator is coming from the serializer), it will do exactly this.
comment
created
yyaremenko
username_0
Yes, I totally understand everything you describe. The Form component translates a string into null, and then this null is passed to the `LenghtValidator`, which fails the empty string comparison, because, well, it receives null.

And that is exactly the problem - as a developer, I don't know and is not expected to know / remember about this entire process. If a `LengthValidator` expects to have `empty_data` explicitly set, when using in Form, this must be described in the documentation. Does this make sense?
comment
created
xabbuh
username_1
Documenting somewhere that the `TextType` will return `null` by default instead of the empty string does make sense to me.
comment
created
yyaremenko
username_0
But a developer still will encounter the problem - for instance, in my case (REST API), the field type is `null`, because I don't actually have an HTML form, I just need a validation. The developer will look at the `LengthValidator` documentation, not at the Form field types documentation.
comment
created
xabbuh
username_1
Well, but `null` is ignored by nearly all validators and they will need to be combined with `NotNull`. Otherwise, being able to define constraints for optional attributes wouldn't be possible.
comment
created
yyaremenko
username_0
I'm sorry I'm not sure if I either understand you correctly or I express my ideas in an understandable way.

Let me try to make things clear.

1. The scope of the problem we are talking about now is exactly the `LengthValidator`, not any other validator.

2. The problem reason is misleading `allowEmptyString` parameter, which does not work correctly, when set to `false` unless you provide an external `empty_data` parameter with an empty string.

3. The problem itself is that a developer expects to get an error when he sets `allowEmptyString` to `false` and provides an empty string, but the error is never returned, because of the Form component converting an empty string to `null`.

4. My exact example
I write a JSON API and use Form component to validate the incoming data, I don't have any HTML form
```
        $builder
            ->add('title', **null**, [
                'constraints' => [
                    new Length([
                        'min' => 3,
                        'max' => 255,
                        'allowEmptyString' => false,
                    ])
                ]
            ])
```
See, the form field type here is `null`. The validator does not work (it allows empty string). I go to the LengthValidator documentation, and it says nothing about that I have to also provide `empty_data` parameter with an empty string value.

5. You can't fix this, because of how Form component works, but you can have the Length constraint documentation updated for `allowEmptyString` parameter, giving a corresponding warning to mandatory provide `empty_data` when using this constraint inside forms.
comment
created
xabbuh
username_1
Sorry, I misread your last comment. To clarify something here: The fact that you pass `null` as the form type here does not mean that no concrete form type will be used. You simply declare that you do not care and let the Form component choose what it thinks matches the best here (which probably is the `TextType` in this case).

I suggest to open an issue in the documentation repository to discuss your idea so the other documentation maintainers can give their opinion too (as they may not follow all the discussions here).
comment
created
yyaremenko
username_0
Okay, I see. Thank you for the clarification regarding the From component. 
Yes, I completely agree with you. Should I open an issue and link to this one?
comment
created
xabbuh
username_1
please do so
comment
closed
fabpot
username_2
issue