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

Cannot use multiple requiring rules #647

Open
striker4150 opened this issue Jan 24, 2024 · 3 comments
Open

Cannot use multiple requiring rules #647

striker4150 opened this issue Jan 24, 2024 · 3 comments

Comments

@striker4150
Copy link

striker4150 commented Jan 24, 2024

✏️ Describe the bug
Adding multiple requiring rules to a property/parameter only results in the last added requiring rule being applied. For example, if a RequiredUnless rule is used alongside a RequiredWith rule, only the RequiredWith rule will be applied (assuming that the RequiredWith rule is last).

The offending lines seem to be:

$this->rules = $this->rules->reject(function (ValidationRule $rule) use ($classes) {
foreach ($classes as $class) {
if ($class instanceof RequiringRule && $rule instanceof RequiringRule) {
return true;
}

↪️ To Reproduce

it('cannot add multiple requiring rules', function () {
    class BaseData extends Data
    {
        public function __construct(
            #[RequiredUnless('requires_a', false), RequiredWith('b')]
            public string $a,
            public string $b,
            public bool $requires_a,
        ) {
        }
    }

    dd(BaseData::getValidationRules([]));
});

✅ Expected behavior
In the scenario above, I expect the a property to have the requiring rules ['required_unless:requires_a,false', 'required_with:b']. However, the a property only has the required_with:b. Laravel does offer the ability to apply multiple requiring rules at once, as demonstrated here:

Validator::make(
  [
    "a" => "a",
    "b" => "b",
    "requires_a" => false
  ],
  [
    "a" => ["required_unless:requires_a,false", "required_with:b"]
  ]
)->passes();
// Returns true

Validator::make(
  [
    "a" => "",
    "b" => "b",
    "requires_a" => true
  ],
  [
    "a" => ["required_unless:requires_a,false", "required_with:b"]
  ]
)->passes();
// Returns false (it's required with b AND requires_a is true)

Validator::make(
  [
    "a" => "",
    "b" => "b",
    "requires_a" => false,
  ],
  [
    "a" => ["required_unless:requires_a,false", "required_with:b"]
  ]
)->passes();
// Returns false (it's required with b)

Validator::make(
  [
    "a" => "",
    "b" => "",
    "requires_a" => true,
  ],
  [
    "a" => ["required_unless:requires_a,false", "required_with:b"]
  ]
)->passes();
// Returns false (requires_a is true)

Validator::make(
  [
    "a" => "",
    "b" => "",
    "requires_a" => false
  ],
  [
    "a" => ["required_unless:requires_a,false", "required_with:b"]
  ]
)->passes();
// Returns true

I expect requiring rules to be applied even if they are of the same type. The only requiring rule that I would expect to not be duplicated is the plain Required rule.

🖥️ Versions

Laravel: 10.x
Laravel Data: 3.11.0
PHP: 8.1.15

@rubenvanassche
Copy link
Member

Yeah this is a difficult one, and probably one for v5 it basically requires a rewrite of the whole rule inferring system.

The RequiredRuleInferrer always adds a required rule because the type is not nullable.

Then the AttributesRuleInferrer adds the required attributes, it then will remove the required rule we've added earlier for RequiredUnless, then when adding RequiredWith we again remove a required rule if present. Sadly enough this is RequiredUnless.

I've quickly tried a special method when adding the attributes, to not replace the requires but that also fails in a lot of tests.

We need a new RuleInferrer for this, basically one that completely replaces the existing ones we have and merges them into one rule inferrer so that we can be smarter about inferring rules. At the moment we don't have an idea what's happening in the other rule inferrers.

Technically, this shouldn't be a breaking change since we could just add that rule inferrer, I'll take a look in the coming weeks if I can find the time to implement such a thing. PR's are also always welcome.

I've already added a test which tests this case:

it('is possible to have multiple required rules', function () {
DataValidationAsserter::for(new class () extends Data {
#[RequiredUnless('is_required', false), RequiredWith('make_required')]
public string $property;
public string $make_required;
public bool $is_required;
})->assertRules([
'property' => ['string', 'required_unless:is_required', 'required_with:make_required'],
'make_required' => ['required', 'string'],
'is_required' => ['boolean'],
]);
})->skip('Add a new ruleinferrer to rule them all and make these cases better');

@spatie-bot
Copy link

Dear contributor,

because this issue seems to be inactive for quite some time now, I've automatically closed it. If you feel this issue deserves some attention from my human colleagues feel free to reopen it.

@spatie-bot
Copy link

Dear contributor,

because this issue seems to be inactive for quite some time now, I've automatically closed it. If you feel this issue deserves some attention from my human colleagues feel free to reopen it.

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

No branches or pull requests

3 participants