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

NotBlank validator should not invalidate null values #27876

Open
Taluu opened this Issue Jul 6, 2018 · 22 comments

Comments

Projects
None yet
8 participants
@Taluu
Contributor

Taluu commented Jul 6, 2018

Description
In most validators, there's a failsafe mecanism that prevent the validator from validating if it's a null value, as there's a validator for that (NotNull). I think NotBlank shouldn't bother on null values, especially as NotBlank and NotNull are used together to prevent null values too.

Not really a feature request, but not really a bug too... more like a RFC but for all versions. But I guess this would be a BC break though. :/ So maybe add a deprecated on the validator if it's a null value or something, and remove the null invalidation from the NotBlank on 5.0 ?

Currently, we need to reimplement the validator just to skip null values...

Possible Solution
Either deprecate the validation on null value in NotBlank, or do as in most validators, (if null value, then skip). But as I mentionned, this would probably be a bc break.

@stof

This comment has been minimized.

Member

stof commented Jul 6, 2018

That would be a huge BC break.
And due to the form component setting properties to null when submitting an empty field, it might even be confusing to change the behavior of NotBlank. So I'm not sure it is worth it.

@theofidry

This comment has been minimized.

Contributor

theofidry commented Jul 8, 2018

Maybe we could add a NullOrNotBlank and potentially rename NotBlank to NotNullNotBlank. I'm not too happy with the names but IMO it's the original name that is messed up and we can't change it without BC breaks.

On a side note, I witnessed at least 10 Symfony projects where they declared a NotBlank ignoring null values, so I also strongly feel there is an issue here.

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Jul 8, 2018

especially as NotBlank and NotNull are used together to prevent null values too.

Can you provide a source for this? We always use just NotBlank. Usage for NotNull is really rare in projects I've seen.

rename NotBlank to NotNullNotBlank

How is "not blank" value ever null value? Blank includes null.

I witnessed at least 10 Symfony projects where they declared a NotBlank ignoring null values,

For what use case?

@theofidry

This comment has been minimized.

Contributor

theofidry commented Jul 8, 2018

@ostrolucky because there is a need to check for non null empty values. null is often given either as "reset" or "not provided" value.

You can always argue that a form (for an API or not) - and I agree on paper. But in practice for a lot of front-ends omitting non provided fields as opposed to sending null requires too much work for what it's worth, hence this need in the back-end.

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Jul 8, 2018

If you have a text type field in your form and don't type anything there, it's transformed to null. If you omit that field completely, it's again null. There is no difference. This kind of problem is meant to be solved with validation groups.

@theofidry

This comment has been minimized.

Contributor

theofidry commented Jul 8, 2018

@ostrolucky I don't really see how that invalidates the points above tbh

@xabbuh

This comment has been minimized.

Member

xabbuh commented Jul 8, 2018

If I don't misunderstand the proposal that would pretty much the same as having the Length constraint with min set to 1, isn't it?

@theofidry

This comment has been minimized.

Contributor

theofidry commented Jul 8, 2018

@xabbuh

This comment has been minimized.

Member

xabbuh commented Jul 8, 2018

How do you use it with other types if you do not want to fail on null? I mean do you rely on implicit type casts here?

@theofidry

This comment has been minimized.

Contributor

theofidry commented Jul 8, 2018

No we do explicit casts in your blank validator. We don't rely on empty() and if the value is null we don't validate it with this constraint

@xabbuh

This comment has been minimized.

Member

xabbuh commented Jul 9, 2018

Well, my point is I don't see a use case for the NotBlank constraint when you are not validating a string. For other types there are better constraints IMO. Based on this assumption we only need to discuss whether or not we need to distinguish null from the empty string which brings us back to the question if we need a shortcut for the Length constraint with a minimum length of 1.

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Jul 9, 2018

@xabbuh : The issue with the Length constraint is that it ignores empty strings:

if (null === $value || '' === $value) {
return;
}

@theofidry

This comment has been minimized.

Contributor

theofidry commented Jul 9, 2018

Which makes no sense to me neither tbh...

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Jul 9, 2018

You can also use NotEqualTo with ''

@theofidry

This comment has been minimized.

Contributor

theofidry commented Jul 9, 2018

@ostrolucky there is workarounds, we are not arguying otherwise. We would like however to make those constraints more intuitive

@Neirda24

This comment has been minimized.

Neirda24 commented Jul 13, 2018

Definitely agreeing with @theofidry . When using any Constraints in symfony the first rule is "Do not validate what you are not supposed to", so I don't understand why there is a NotBlank validating null and a NotNull. maybe add something like allowNull in the NotBlank constraints but set it to deprecated...

We can always find a way to go around this of course. But to stay consistent with any other constraints I feel like null should not be tested by NotBlank...

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Jul 13, 2018

I don't agree for one. You can't deprecate that without majority agreeing. All I see is loud minority. What you guys want can be achieved with other constraints like Length and NotEqualTo, what NotBlank does currently can't be replaced with any other single constraint, only with multiple.

@theofidry

This comment has been minimized.

Contributor

theofidry commented Jul 13, 2018

Again, we are not arguing we cannot achieve what we want. There is several workarounds available for them and we already discussed about them here. Our point is that the default behaviour is confusing and intuitive. That you consider it to be a loud minority and not a design/UX issue is another story and up for debate and why this issue has been created.

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Jul 13, 2018

Seems intuitive to me. Null is blank, NotBlank prevents blank values.

@Neirda24

This comment has been minimized.

Neirda24 commented Jul 13, 2018

I know this can be a huge debate but for me "the absence of value" does not mean "a blank value". Also empty in PHP also applies to arrays. So [] is empty but is not blank... (another reason why I also find it not intuitive).

@Taluu

This comment has been minimized.

Contributor

Taluu commented Jul 13, 2018

Adding an option allowNull (or something like that), defaulting to false would be a good BC imo, and would solve the current problem (and why not remove it on 5.0 ?). As @Neirda24 said, a null value is "no value" rather than "blank value"...

@zanbaldwin

This comment has been minimized.

Contributor

zanbaldwin commented Sep 19, 2018

This is definitely an issue, especially as null values are common in APIs which Symfony is being used to build more and more.

Rather than deprecating existing validators/constraints (NotBlank should probably remain as it is because the Form component was historically built for web forms), it might be a good idea to create a new one (as mentioned above) that compliments the already existing two:

  • A NotEmpty constraint would work exactly the same as NotBlank except allowing null values.
  • The combination of NotNull and NotEmpty would be equivalent to NotBlank.
  • APIs get a really useful constraint, whilst preserving backwards-compatibility.

I had a little play with what that might look like and came up with the following that should cover most data types an API would receive. It probably needs tidying up a bit if anyone thinks this is a good idea.

/**
 * @Annotation
 * @Target({"PROPERTY", "METHOD", "ANNOTATION"})
 */
class NotEmpty extends Constraint
{
    public const IS_EMPTY_ERROR = '1034929d-a83b-4817-a722-0a4a9266067a';

    protected static $errorNames = array(
        self::IS_EMPTY_ERROR => 'IS_EMPTY_ERROR',
    );

    public $message = 'This value should not be empty, use null instead.';

    public function validatedBy(): string
    {
        return NotEmptyValidator::class;
    }
}
class NotEmptyValidator extends ConstraintValidator
{
    public function validate($value, Constraint $constraint)
    {
        if (!$constraint instanceof NotEmpty) {
            throw new UnexpectedTypeException($constraint, NotEmpty::class);
        }

        if ($value === null
            // Allow zero numerical values, etc.
            || (\is_scalar($value) && !\is_string($value))
            // Pure whitespace counts as being empty?
            || (\is_string($value) && \trim($value, " \r\n\t") !== '')
        ) {
            return;
        }

        if (empty($value) || (\is_string($value) && \trim($value, " \r\n\t") === '')) {
            $this->context->buildViolation($constraint->message)
                ->setParameter('{{ value }}', $this->formatValue($value))
                ->setCode(NotEmpty::IS_EMPTY_ERROR)
                ->addViolation();
        }
    }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment