[Validator ]AbstractComparisonValidator propertyPath and NULL #29831

sgehrig · 2019-01-10T08:40:02Z

Symfony version(s) affected: 4.2.2

Description
Not sure whether this is a bug or the intended behaviour - but at least it seems to be inconsistent with my understanding on how null values are handled in the validator component.

Given two ?\DateTimeImmutable properties (nullable) in a class to define a timespan where NULL reflects unbounded, I'd like to use two comparison constraints to ensure that date 1 is always less than or equal to date 2. The AbstractComparisonValidator however only deals with NULL on the owning side of the constraint and does not handle NULL on the propertyPath constraint leading to validation errors with the LessThanOrEqual because the date 1 value (a \DateTimeImmutable) is not less than or equal to NULL. The GreaterThanOrEqual constraint is no problem - by coincidence - because a \DateTimeImmutable in date 2 is always greater than NULL in date 1.

How to reproduce

use Symfony\Component\Validator\Constraints as Assert;

class UpdateValidity
{
    /** 
     * @Assert\LessThanOrEqual(propertyPath="validUntil")
     *
     * @var \DateTimeImmutable|null
     */
    public $validFrom;

    /**
     * @Assert\GreaterThanOrEqual(propertyPath="validFrom")
     *
     * @var \DateTimeImmutable|null
     */
    public $validUntil;
}

$u = new UpdateValidity();
$u->validFrom = new \DateTimeImmutable('2019-01-01');
$u->validUntil = new \DateTimeImmutable('2019-12-31');
$validator->validate($u); // OK - validation succeeds

$u->validFrom = new \DateTimeImmutable('2020-01-01');
$validator->validate($u); // OK - validation fails

$u->validFrom = null;
$validator->validate($u); // OK - validation succeeds (by coincidence)

$u->validFrom = new \DateTimeImmutable('2019-01-01');
$u->validUntil = null;
$validator->validate($u); // Not OK - validation fails but should succeed in my opinion

Possible Solution

// vendor/symfony/validator/Constraints/AbstractComparisonValidator.php:50:62
if ($path = $constraint->propertyPath) {
    if (null === $object = $this->context->getObject()) {
        return;
    }

    try {
        $comparedValue = $this->getPropertyAccessor()->getValue($object, $path);
    } catch (NoSuchPropertyException $e) {
        throw new ConstraintDefinitionException(sprintf('Invalid property path "%s" provided to "%s" constraint: %s', $path, \get_class($constraint), $e->getMessage()), 0, $e);
    }
    // <new>
    if (null === $comparedValue) {
        return;
    }
    // </new>
} else {
    $comparedValue = $constraint->value;
}

Additional context
If required one could add a NotBlank or NotNull to the property to ensure that it must not be NULL.

The text was updated successfully, but these errors were encountered:

because of symfony/symfony#29831

xabbuh · 2019-01-10T18:23:50Z

Is this the same as #29719?

sgehrig · 2019-01-10T18:41:17Z

No. #29719 is about obtaining the parent object (either getObject() or getRoot()). This is about the value returned from the propertyPath.

xabbuh · 2019-01-10T20:33:49Z

Can you try out #29839?

sgehrig · 2019-01-11T07:54:28Z

@xabbuh: Should it be

return null === $value2 || $value1 <= $value2;

instead of

return null !== $value2 && $value1 <= $value2;

for LessThanOrEqualValidator (and accordingly for all other validators affected)?
Behause compareValues() will return false if $value2 is null but should return true.

xabbuh · 2019-01-11T08:07:17Z

Why would you expect it to return true? null means there is no value. I think implicitly assuming 0 in this case does not look right.

sgehrig · 2019-01-11T09:00:39Z

I'd assume that, if the value returned by propertyPath is null, the validator will just ignore the comparison. Just as the validator ignores the comparison if the owning side value (that's where the constraint is attached to) is null.

That's why my initial proposal was to handle this generally in the AbstractComparisonValidator (lines 50 to 62).

xabbuh · 2019-01-11T09:14:32Z

IMO that would be unexpected behaviour for the Equal/NotEqual and IdenticalTo/NotIdenticalTo constraints.

sgehrig · 2019-01-11T09:18:33Z

That's true. What do you think would be the expected behaviour for the (greater-than/less-than)/-or-equal validators? Would you follow my assumption?

sgehrig · 2019-01-23T08:00:41Z

A workaround this issue can be using the Expression constraint like that:

use Symfony\Component\Validator\Constraints as Assert;

class UpdateValidity
{
    /** 
     * @Assert\Expression(
     *      expression="value === null or this.validUntil === null or value <= this.validUntil",
     *      message="This value should be less than or equal to valid until."
     * )
     *
     * @var \DateTimeImmutable|null
     */
    public $validFrom;

    /**
     * @Assert\Expression(
     *      expression="value === null or this.validFrom === null or value >= this.validFrom",
     *      message="This value should be greater than or equal to valid from."
     * )
     *
     * @var \DateTimeImmutable|null
     */
    public $validUntil;
}

Just in case someone runs into the same problem.

xabbuh · 2019-01-28T09:31:03Z

That's tricky, I think there are good reasons to expect one or the other. I have no real preference.

sgehrig · 2019-01-28T09:35:41Z

Adding a property to allow defining the NULL behaviour would perhaps be the best option then. Especially because this would be backwards compatible. But that's a feature request then, right?

xabbuh · 2019-01-28T09:45:47Z

yes, that's not something for a bugfix

…aths (xabbuh) This PR was merged into the 3.4 branch. Discussion ---------- [Validator] fix comparisons with null values at property paths | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #29831 | License | MIT | Doc PR | Commits ------- ffa5db7 fix comparisons with null values at property paths

sgehrig · 2020-04-03T06:58:07Z

As #29839 has been merged already, I assume this bug can be marked as "fixed". Right?

sgehrig added a commit to IISHF/web2019 that referenced this issue Jan 10, 2019

changes comparision constraints to expression constraints

a6df595

because of symfony/symfony#29831

xabbuh added Validator Bug labels Jan 10, 2019

carsonbot added the Status: Needs Review label Jan 10, 2019

xabbuh added Status: Waiting feedback and removed Status: Needs Review labels Jan 10, 2019

xabbuh mentioned this issue Jan 10, 2019

[Validator] fix comparisons with null values at property paths #29839

Merged

xabbuh removed the Status: Waiting feedback label Jan 11, 2019

xabbuh closed this as completed Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Validator ]AbstractComparisonValidator propertyPath and NULL #29831

[Validator ]AbstractComparisonValidator propertyPath and NULL #29831

sgehrig commented Jan 10, 2019 •

edited

Loading

xabbuh commented Jan 10, 2019

sgehrig commented Jan 10, 2019

xabbuh commented Jan 10, 2019

sgehrig commented Jan 11, 2019 •

edited

Loading

xabbuh commented Jan 11, 2019

sgehrig commented Jan 11, 2019 •

edited

Loading

xabbuh commented Jan 11, 2019

sgehrig commented Jan 11, 2019

sgehrig commented Jan 23, 2019

xabbuh commented Jan 28, 2019

sgehrig commented Jan 28, 2019

xabbuh commented Jan 28, 2019

sgehrig commented Apr 3, 2020 •

edited

Loading

[Validator ]AbstractComparisonValidator propertyPath and NULL #29831

[Validator ]AbstractComparisonValidator propertyPath and NULL #29831

Comments

sgehrig commented Jan 10, 2019 • edited Loading

xabbuh commented Jan 10, 2019

sgehrig commented Jan 10, 2019

xabbuh commented Jan 10, 2019

sgehrig commented Jan 11, 2019 • edited Loading

xabbuh commented Jan 11, 2019

sgehrig commented Jan 11, 2019 • edited Loading

xabbuh commented Jan 11, 2019

sgehrig commented Jan 11, 2019

sgehrig commented Jan 23, 2019

xabbuh commented Jan 28, 2019

sgehrig commented Jan 28, 2019

xabbuh commented Jan 28, 2019

sgehrig commented Apr 3, 2020 • edited Loading

sgehrig commented Jan 10, 2019 •

edited

Loading

sgehrig commented Jan 11, 2019 •

edited

Loading

sgehrig commented Jan 11, 2019 •

edited

Loading

sgehrig commented Apr 3, 2020 •

edited

Loading