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

[6.x] Add exclude_if validation rule #30835

Merged
merged 7 commits into from Dec 27, 2019
Merged

[6.x] Add exclude_if validation rule #30835

merged 7 commits into from Dec 27, 2019

Conversation

@SjorsO
Copy link
Contributor

SjorsO commented Dec 13, 2019

The goal of this PR is to make it easy to exclude attributes from a request based on the value of other attributes. This is useful when having to validate data from a form where certain checkboxes hide or show other inputs.

For example, imagine a form with an has_doctor_appointment checkbox, that when checked, toggles an appointment_date and doctor_name input. A user can check the checkbox, fill in a date, and then uncheck the checkbox. The date input is no longer visible, but still contains a value. When using Redux Form, even when the input is not visible, the value still gets posted.

Usually, you would validate the form above something like this:

// Post data
{"has_appointment": false, "appointment_date": "2019-12-13"}
public function post(Request $request)
{
    $data = $request->validate([
        'has_doctor_appointment' => 'required|bool',
        'appointment_date' => 'required_if:has_doctor_appointment,true|date',
        'doctor_name' => 'required_if:has_doctor_appointment,true|string'
    ]);

    // $data === ['has_doctor_appointment' => false, 'appointment_date' => '2019-12-13']

    SomeModel::create($data);
}

The database now contains a record where has_appointment is false, but that has an appointment_date. This is not right. As far as i know, the only way to ensure you never store data like this is by adding statements like this to your controller/validator:

if (! $data['has_doctor_appointment') {
    $data['appointment_date'] = null;
    $data['doctor_name'] = null;
}

With the exclude_if validation rule, you can validate like this instead:

// Post data:
{"has_appointment": false, "appointment_date": "2019-12-13"}
public function post(Request $request)
{
    $data = $request->validate([
        'has_doctor_appointment' => 'required|bool',
        'appointment_date' => 'exclude_if:has_appointment,false|required|date',
        'doctor_name' => 'exclude_if:has_appointment,false|required|string',
    ]);

    // $data === ['has_appointment' => false]

    SomeModel::create($data);
}

The idea for this validation rule comes from a project we are working on at the office. It has multiple large forms, that all have many checkboxes that toggle other inputs. The way we solve this now is by making sure the inputs are reset when they are hidden. When they are reset Redux Form doesn't include them in the post data. Making sure the fields are properly reset has been troublesome in the past. This approach also does not prevent users from theoretically posting their own incorrect data to the endpoint.

With this new validation rule, the front-end doesn't have to reset any of the fields, since the back-end can easily exclude values.


This PR is still a work in progress. I would like to get some feedback on it before i spend more time on it.

Todo:

  • More tests
  • Nested attributes 1 level deep
  • exclude_unless rule

Not tested:
Nested attributes 2+ levels deep (I'm not sure if this is supported at all)

SjorsO added 2 commits Dec 13, 2019
cs
@GrahamCampbell GrahamCampbell changed the title [6.x] WIP - add exclude_if validation rule [6.x] [WIP] Add exclude_if validation rule Dec 13, 2019
@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Dec 13, 2019

I could see the utility here. How hard would it be to support nested attributes?

@SjorsO

This comment has been minimized.

Copy link
Contributor Author

SjorsO commented Dec 13, 2019

@taylorotwell Less difficult than i thought it would be. The ValidationRuleParser makes it a lot easier by exploding all the rules. It also helps that parent attributes always appear before any nested attributes.

Working example 1:

// Post data
{"has_appointments": false, "appointments": ["2019-05-15", "invalid date"]}
public function post(Request $request)
{
    $data = $request->validate([
        'has_appointments' => ['required', 'bool'],
        'appointments.*' => ['exclude_if:has_appointments,false', 'required', 'date'],
    ]);

    // $data === ['has_appointments' => false]
}

Working example 2:

// Post data
{
    "has_appointments": false,
    "appointments": [
        {"date": "2019-05-15", "name": "Bob"},
        {"date": "invalid date", "name": "Bob"},
    ]
}
public function post(Request $request)
{
    $data = $request->validate([
        'has_appointments' => ['required', 'bool'],
        'appointments' => ['exclude_if:has_appointments,false', 'required', 'array'],
        'appointments.*.date' => ['required', 'date'],
        'appointments.*.name' => ['required', 'string'],
    ]);

    // $data === ['has_appointments' => false]
}

I tried to write some tests for even deeper nesting, but they were behaving very strangely. It seemed to be completely ignoring validation rules for attributes nested twice. I was either doing something wrong or that is a problem not related to this PR.

SjorsO added 4 commits Dec 18, 2019
cs
@SjorsO

This comment has been minimized.

Copy link
Contributor Author

SjorsO commented Dec 18, 2019

@taylorotwell I think this is ready to merge

@SjorsO SjorsO changed the title [6.x] [WIP] Add exclude_if validation rule [6.x] Add exclude_if validation rule Dec 19, 2019
@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Dec 27, 2019

What about this?

I tried to write some tests for even deeper nesting, but they were behaving very strangely. It seemed to be completely ignoring validation rules for attributes nested twice. I was either doing something wrong or that is a problem not related to this PR.

@taylorotwell taylorotwell merged commit f9ea321 into laravel:6.x Dec 27, 2019
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@SjorsO

This comment has been minimized.

Copy link
Contributor Author

SjorsO commented Dec 28, 2019

What about this?

I tried to write some tests for even deeper nesting, but they were behaving very strangely. It seemed to be completely ignoring validation rules for attributes nested twice. I was either doing something wrong or that is a problem not related to this PR.

I'll create a PR in the next few that show off the weird behavior

@baceto90

This comment has been minimized.

Copy link

baceto90 commented Jan 3, 2020

This is very useful, but required_if and exclude_if validations is very similar. Why not just change the current behavior of required_if validation rule?

@martinbean

This comment has been minimized.

Copy link
Contributor

martinbean commented Jan 7, 2020

@SjorsO This is amazing and perfect for an issue I have where a user can optionally make a product rentable.

As you say: I have a checkbox in the UI to determine if the product should be available for rent, and then separate inputs for the rental price and period. Thank you for contributing this!

@spawnia

This comment has been minimized.

Copy link
Contributor

spawnia commented Jan 21, 2020

I think this feature doesn't really fit within validation. It rather seems like it is sanitization of inputs.

It might be better to keep those separate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.