[10.x] Fix validation of attributes that depend on previous excluded attribute

[onlime]

When using a validation rule that depends on another attribute, e.g. required_if, and that other attribute had applied exclude rule, then the order in $rules matters. If the excluded attribute comes first, it is already excluded during the Validator::passes() run, before the require_if rule can check that attributes value.

Of my understanding, the rule order should not matter, so the following rules should give us the same validation:

// WORKING:
        [
            'profile_id' => ['nullable', 'required_if:type,profile', 'integer'],
            'type'       => ['required', 'string', 'exclude'],
        ],

// NON-WORKING, as 'type' is excluded before 'required_if' is checked in 'profile_id':
        [
            'type'       => ['required', 'string', 'exclude'],
            'profile_id' => ['nullable', 'required_if:type,profile', 'integer'],
        ],

I have tested this with the following PEST test:

<?php

it('passes validation with exclude rule', function (array $rules, array $data, array|false $expectedValidatedData) {
    $validator = Validator::make($data, $rules);
    expect($validator->passes())->toBeTrue()
        ->and($validator->validated())->toBe($expectedValidatedData);
})->with([
    'no-profile' => [
        [
            'type'       => ['required', 'string', 'exclude'],
            'profile_id' => ['nullable', 'required_if:type,profile', 'integer'],
        ],
        [
            'type'       => 'denied',
            'profile_id' => null,
        ],
        [
            'profile_id' => null,
        ],
    ],
    'profile-with-id' => [
        [
            'type'       => ['required', 'string', 'exclude'],
            'profile_id' => ['nullable', 'required_if:type,profile', 'integer'],
        ],
        [
            'type'       => 'profile',
            'profile_id' => 1,
        ],
        [
            'profile_id' => 1,
        ],
    ],
    'profile-missing-id' => [
        // should not validate! but would need to move 'profile_id' rules before 'type'
        [
            'type'       => ['required', 'string', 'exclude'],
            'profile_id' => ['nullable', 'required_if:type,profile', 'integer'],
        ],
        [
            'type'       => 'profile',
            'profile_id' => null,
        ],
        [
            'profile_id' => null,
        ],
    ],
]);

it('fails validation with exclude rule', function (array $rules, array $data) {
    $validator = Validator::make($data, $rules);
    expect($validator->passes())->toBeFalse();
})->with([
    'profile-missing-id' => [
        [
            'profile_id' => ['nullable', 'required_if:type,profile', 'integer'],
            'type'       => ['required', 'string', 'exclude'],
        ],
        [
            'type'       => 'profile',
            'profile_id' => null,
        ],
    ],
]);

expected:

   FAIL  Tests\Feature\Rules\ExcludeTest
  ✓ it passes validation with exclude rule with dataset "no-profile"                                    0.10s  
  ✓ it passes validation with exclude rule with dataset "profile-with-id"                               0.04s  
  ⨯ it passes validation with exclude rule with dataset "profile-missing-id"                            0.04s  
  ✓ it fails validation with exclude rule with dataset "profile-missing-id"                             0.04s  
  ───────────────────────────────────────────────────────────────────────────────────────────────────────────  
   FAILED  Tests\Feature\Rules\ExcludeTest > it passes validation with exclude rule with dataset "profile-m…   
  Failed asserting that false is true.

but the actual result is:

   PASS  Tests\Feature\Rules\ExcludeTest
  ✓ it passes validation with exclude rule with dataset "no-profile"                                    0.10s  
  ✓ it passes validation with exclude rule with dataset "profile-with-id"                               0.04s  
  ✓ it passes validation with exclude rule with dataset "profile-missing-id"                            0.04s  
  ✓ it fails validation with exclude rule with dataset "profile-missing-id"                             0.04s  

  Tests:    4 passed (7 assertions)
  Duration: 0.31s

Can this considered to be a bug or is there some logic behind it? with my fix in this PR, phpunit tests/Validation/ framework tests on latest 10.x still run through nicely.

[taylorotwell]

This seems like entirely different logic? shouldBeExcluded is not considered at all anymore? Please mark as ready for review when you want me to take another look.

[driesvints]

@onlime are you still working on this?

[onlime]

This seems like entirely different logic? shouldBeExcluded is not considered at all anymore? Please mark as ready for review when you want me to take another look.

Yes, shouldBeExcluded is still respected and needed by breaking out of the inner rules loop. I don't think this is a different logic, as the attribute is already in the $excludeAttributes array, just doing the cleanup (removal of all exclude attributes) as a final step instead of doing it on the go. I don't think this breaks any code out there that follows the docs, but it might be considered a breaking change and may be targeted to 11.x, as people may have workarounds in place for this weird behavior.

I still think the order of attributes should not matter and an attribute should be able to depend on another's value, even if that other attribute has the exclude rule applied. If that's intended behavior, it should be clearly pointed out in the docs.

Honestly, I am a bit overwhelmed finding the perfect fix for this. That whole passes() logic seems quite confusing and I think it should be completely refactored. Also the naming of $this->rules (array of rules definitions for attributes) vs. the internal $rules (single validation rules) in the same method is misleading.

Maybe the original author can comment my PR and test this more thoroughly?

[taylorotwell]

Seems like we would need some tests on this? I also see we still remove excluded attributes on line 419. Should we still be doing that?

Mark as ready for review when I should look again.

[onlime]

This was completed in #48122 which was merged into Laravel v10.21.0