Change validation to consider the empty string a valid value

[kraih]

This appears to be the default assumption of our users. The empty
string can now be validated with regexes and other checks.
Additionally non-browser users and form elements without any value
at all can now be considered during validation.

[kraih]

Calling for a vote @mojolicious/core.

[kraih]

To be more precise, this is about the case where you have a query string like ?foo=&bar=baz. The question is if $validation->optional('foo') and $validation->required('foo') should accept the empty string for further validation.

[kalikiana]

I actually ran into this in os-autoinst/openQA#2812 where we have an API which accepts an optional parameter that must be numeric or empty. If the empty string is given, the value is cleared. Other values can be updated without changing or clearing said value, however - so empty is not the same as undefined. I actually defined it as ->optional('size_limit_gb')->like(qr/^(|[0-9]+)\z/) thinking this would consider empty strings.

[marcusramberg]

Can't make up my mind if this is better or not. Going to have to vote neutral.

[jhthorsen]

Looks sane to me 👍

[Grinnz]

I am voting neutral. I think it would be good to provide more flexibility by doing this, but also I'm not sure about the implications for day-to-day use with forms as submitted by browsers.

[CandyAngel]

tl;dr Voting yes

It seems to trip new users up quite a bit. Perhaps it is because it looks like you are asserting whether the topic has to be part of the input or can be omitted, but it is actually considering the value as well (which is really what the checks are for).

This change introduces a clearer separation between "is the topic in the input" and "does the value of the topic meet this criteria" (e.g. is not an empty string). It also makes using Mojolicious::Validator nicer to use outside of validating web forms (like validating CSV files from crazy systems that have unstable headers!) where an empty string in a required topic is valid.

Any code currently using optional to work around this will continue to work. Just trying to think if anyone could be relying on the not-empty-string required behaviour without any additional checks in a way that isn't already flawed.

My vote is yes because having optional and required being used for checking the presence of a topic and the checks being used to check the value is a simple and easy-to-follow concept.

[jberger]

I was going to vote neutral, but @CandyAngel 's argument persuaded me. Voting yes (👍 )

[daleif]

Doesn't this get messy as an interface when you have an optional value that is allowed to be the empty string, but if it is not then extra requirement.

Seems messy if everything then has to go though regex

I agree on the old setup

optional('field') -> size(3,10)

was annoying because field was undef it it had no value, but the requirement worked if the field had a value.

[tim-2]

I agree with daleif. How do I handle the case that e.g. I expect an optional number. The browser always transmits an empty input field as empty string.

In this minimal example I always get has_error for an optional param. That doesn't seem right.

use Mojolicious::Lite;

get '/' => sub {
    my $c = shift;

    my $v = $c->validation;
    $v->optional('n')->num(0, 9);
    
    $c->stash(
        msg => $v->has_data ? $v->has_error ? 'has_error' : 'ok' : 'nodata',
    );
} => 'index';

app->start;

__DATA__

@@ index.html.ep
%= $msg
<form>
    %= number_field 'n'
    %= submit_button
</form>

[kraih]

@tim-2 That case could probably only be handled with another alternative to required and optional.

[daleif]

Would it even be possible to do chaining in the case where the value can either be empty or need to match conditions?

I had to change mine into

$v -> optional('field');
$v -> size(3,50)  if $v -> param;

which works but is perhaps not elegant.

[kraih]

There should probably be something like $v->optional('foo')->not_empty->size(3, 50). The not_empty method would simply remove empty string values from the foo input values.

[tim-2]

Yes, this would be a nice solution.

[kraih]

We can actually resolve this with a new filter. f410199...c0347bd

[daleif]

So just returning undef is enough to end the chain? (I'm still new at this so pardon if that is obvious)

(one question, not 100% related, are a check allowed to change the value passed on? I had a check that verifies a date (input via a string like 2020-03-21) and it seemed convenient to just replace the input with the generated DateTime object)

[CandyAngel]

@dalief

Checks don't change the value, but filters can.

You can try to convert it to an object in a filter and then check it is of that object. I do this but with Time::Piece and strptime.

$v->required(create_on => 'trim', 'dateify')->date;

[daleif]

@CandyAngel thanks, that might be a better solution

I guess the answer should be: checks should not change the value, but filters can.

it is not too hard changing the value in a check (as I just did ;-), but one probably should not.

[daleif]

I for one does did not expect the empty string to now be valid under required.

As a new user I would appreciate if in the documentation required explicitly stated that and make sure a value is defined. I do not consider the empty string to be a part of a value is present. At leat such that users don't get nasty surprises.

Just updated my mojolicious installation and spent an hour debugging my form validation and could not figure out why the empty string no longer triggered the required error.

As it sits now, what is the required check even useful for in form validation? The only thing it can check if whether a field is not part of the submission (plus it can apply filters).

Can a normal form validation even trigger the required error without naming a field name not present in the form?

[CandyAngel]

Just updated my mojolicious installation and spent an hour debugging my form validation and could not figure out why the empty string no longer triggered the required error.

This is what I was thinking of when I said:

Just trying to think if anyone could be relying on the not-empty-string required behaviour without any additional checks in a way that isn't already flawed.

What I meant by "already flawed" was by not doing any additional checks that should make an empty string fail anyway, such as size or num.

Can a normal form validation even trigger the required error without naming a field name not present in the form?

No, because that is what required is checking in the validation: that the named field (topic) is present in its input (from the form). It sounds like you are presuming this could never happen, but it can.

[daleif]

I still think it is best to explicitly specify this in the docs to avoid confusion.

Granted I have not checked many other validation frameworks, but those I had come by trigger required on empty input.

[daleif]

Additionally the not_empty filter is not really well suited for an example like the one @tim-2 provided above. Say I have a field that can be either empty or has to be a valid URL.

$v -> input({'foo' => ''}); 
$v -> optional('foo','not_empty`) -> url;

here foo will never pass because not_empty converts the empty string to undef and this it it never added to passed.

$v -> input({'foo' => ''}); 
$v -> optional('foo');

but here is passes.

not_empty is good combined with required, that seems to solve my issues with that, bringing it back to normal.

[CandyAngel]

Say I have a field that can be either empty or has to be a valid URL.
here foo will never pass because not_empty converts the empty string to undef and this it it never added to passed.

But foo does pass validation. There is just no value for it in passed because you told it to remove it if it is an empty string (using the not_empty filter) and then said its presence is optional.

Looks like you really want:
$v->required('foo')->empty_or_url

EDIT #1: Clarified and added context