spatie / laravel-data

Powerful data objects for Laravel
https://spatie.be/docs/laravel-data/
MIT License
1.28k stars 208 forks source link

Cannot use multiple requiring rules #647

Open striker4150 opened 9 months ago

striker4150 commented 9 months ago

✏️ 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: https://github.com/spatie/laravel-data/blob/324970e910414b92b43a239843efd06fa51090e9/src/Support/Validation/PropertyRules.php#L40-L44

↪️ 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 commented 8 months ago

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: https://github.com/spatie/laravel-data/blob/e954107a25eebe903228f34bf961f148114520d5/tests/ValidationTest.php#L279-L290

spatie-bot commented 4 months ago

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 commented 1 week ago

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.