Skip to content

[12.x] Introduce Rule::anyOf() for Validating Against Multiple Rule Sets (#54880) #54946

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

Closed
wants to merge 19 commits into from

Conversation

brianferri
Copy link
Contributor

@brianferri brianferri commented Mar 8, 2025

please refer to #55191

Copy link

github-actions bot commented Mar 8, 2025

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@brianferri brianferri changed the title [12.x] Introduce Rule::oneOf() for Validating Against Multiple Rule Sets (https://github.com/laravel/framework/discussions/54880) [12.x] Introduce Rule::oneOf() for Validating Against Multiple Rule Sets (#54880) Mar 8, 2025
@brianferri brianferri marked this pull request as ready for review March 9, 2025 12:30
@brianferri brianferri marked this pull request as draft March 13, 2025 14:40
@brianferri brianferri changed the title [12.x] Introduce Rule::oneOf() for Validating Against Multiple Rule Sets (#54880) [12.x] Introduce Rule::anyOf() for Validating Against Multiple Rule Sets (#54880) Mar 13, 2025
@brianferri brianferri marked this pull request as ready for review March 13, 2025 16:56
@taylorotwell
Copy link
Member

taylorotwell commented Mar 17, 2025

The fact that this simple version of the code doesn't work is confusing to me. I don't know why it should be limited to array fields.

$validator = new Validator(
    resolve('translator'),
    [
        'email' => 'test@example.com'
    ],
    [
        'email' => Rule::anyOf([
            ['required', 'min:20'],
            ['required', 'email'],
        ])
    ],
);

$this->assertTrue($validator->passes());

Added a failing test for this.

@brianferri
Copy link
Contributor Author

brianferri commented Mar 18, 2025

The fact that this simple version of the code doesn't work is confusing to me. I don't know why it should be limited to array fields.

Conceptually nothing should impede that; I had simply opted to expect the exception that the validator would throw in case it found something it didn't like:
https://github.com/laravel/framework/pull/54946/files#diff-85c77d67e8673fff45e60c4c9a8e1ee50af508a614364e7dd1c95152b25fedeaR29-R37
as it would throw:

TypeError: Illuminate\Validation\Factory::make(): Argument #1 ($data) must be of type array, string given

laravel-framework/src/Illuminate/Validation/Factory.php:105
laravel-framework/src/Illuminate/Support/Facades/Facade.php:361
laravel-framework/src/Illuminate/Validation/Rules/AnyOf.php:62
laravel-framework/src/Illuminate/Validation/Validator.php:902
laravel-framework/src/Illuminate/Validation/Validator.php:678
laravel-framework/src/Illuminate/Validation/Validator.php:481
laravel-framework/tests/Validation/ValidationAnyOfRuleTest.php:72

The obvious and easiest solution is just wrapping the $value being validated for each rule set with an Arr::wrap().
However, this begs the question, should the same thing be applied to the $ruleSets as well and allow for:

$validator = new Validator(
            resolve('translator'),
            [
                'email' => '20charstringtestvalidation'
            ],
            [
                'email' => Rule::anyOf([
                    'required|min:20',
                    'required|email',
                ])
            ],
        );

        $this->assertTrue($validator->passes());
example change
diff --git a/src/Illuminate/Validation/Rules/AnyOf.php b/src/Illuminate/Validation/Rules/AnyOf.php
index 7086a30c4f..04a20854cb 100644
--- a/src/Illuminate/Validation/Rules/AnyOf.php
+++ b/src/Illuminate/Validation/Rules/AnyOf.php
@@ -4,6 +4,7 @@
 
 use Illuminate\Contracts\Validation\Rule;
 use Illuminate\Contracts\Validation\ValidatorAwareRule;
+use Illuminate\Support\Arr;
 use Illuminate\Support\Facades\Validator;
 use InvalidArgumentException;
 
@@ -59,8 +60,8 @@ public function passes($attribute, $value)
 
         foreach ($this->ruleSets as $ruleSet) {
             $validator = Validator::make(
-                $value,
-                $ruleSet,
+                Arr::wrap($value),
+                Arr::wrap($ruleSet),
                 $this->validator->customMessages,
                 $this->validator->customAttributes
             );

Thanks for the suggestion! I already have changes locally that are ready to be pushed upon confirmation 😄

@taylorotwell
Copy link
Member

taylorotwell commented Mar 19, 2025

Yes, I would probably expect that to work as well. Thanks @brianferri

@brianferri
Copy link
Contributor Author

Failure seems to be unrelated to this PR

@taylorotwell
Copy link
Member

@brianferri just added another simple test that should pass but fails.

@taylorotwell taylorotwell marked this pull request as draft March 22, 2025 17:22
brianferri and others added 3 commits March 24, 2025 00:38
* feat: add more validation tests

* wip: add failing test

* wip: add basic string rule validations

* chore: rename object fields for better debugging

* refactor: rename ruleSets to rules

* fix: respect array rule validation

---------

Co-authored-by: Christian Ascone <ascone.christian@gmail.com>

* fix: this should be passing because AnyOf has no type relevance and 'required' only checks to see if the field has something in it

---------

Co-authored-by: Christian Ascone <ascone.christian@gmail.com>

---------

Co-authored-by: Christian Ascone <ascone.christian@gmail.com>
@brianferri brianferri marked this pull request as ready for review March 27, 2025 08:49
@brianferri brianferri deleted the branch laravel:12.x March 27, 2025 12:54
@brianferri brianferri closed this Mar 27, 2025
@brianferri brianferri deleted the 12.x branch March 27, 2025 12:54
@brianferri brianferri restored the 12.x branch March 27, 2025 12:56
@brianferri brianferri deleted the 12.x branch March 27, 2025 12:57
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.

2 participants