Skip to content
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

[5.4] Attempt to simplify validation with an edge case #18376

Merged
merged 4 commits into from
Mar 16, 2017
Merged

[5.4] Attempt to simplify validation with an edge case #18376

merged 4 commits into from
Mar 16, 2017

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Mar 16, 2017

This PR attempts to solve the issue with using required_* rules while the ConvertEmptyStringToNull middleware is applied.

Given the following data & rules:

validator(
    [
        'address_one' => null,
        'address_two' => null,
    ],
    [
        'address_one' => ['nullable','required_with:address_two', 'min:6'],
        'address_two' => ['nullable','required_with:address_one', 'min:6'],
    ]
);

With this PR, using nullable won't skip the required_with validation if the value is null and will still fail, this solves the case when you accept null as a value for all rules but rules that imply the field is required.

@miklcct
Copy link
Contributor

miklcct commented Apr 11, 2017

Doesn't nullable mean NULL is acceptable in addition to other valid values?

In the given example, I expect that the validation passes because both fields are nullable and really NULL. However, if both fields are empty strings or empty collections, the validation would fail.

This fix has broken my app assuming that NULL is always a valid value when nullable is specified even in presence of implicit checks, and I have to patch my validator to revert to the pre-fix behaviour.

@decadence
Copy link
Contributor

@themsaid is this affecting required rule or it is only for conditional required rules?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants