spatie / laravel-data

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

RequiredIf rule not applied to Optional properties initialized with 'new Optional' #877

Closed RasmusGodske closed 3 weeks ago

RasmusGodske commented 3 weeks ago

✏️ Describe the bug Optional properties are not properly handled when using the RequiredIf rule, specifically when the optional property is initialized with new Optional. When the RequiredIf rule is applied AND the condition is met, the validation does not enforce the "required" status for properties initialized with new Optional, while it does work correctly for properties declared as Optional|string without initialization.

↪️ To Reproduce Provide us a pest test like this one which shows the problem:

enum SomeEnumType: string {
  case FOO = 'foo'
}

it('this test should create an error during validation', function () {

    class UpdateProductData extends Data
    {
        public function __construct(
             #[Required]
             #[WithCast(EnumCast::class)]
             public SomeEnumType $type,

            // This creates no rules
            #[RequiredIf('type', [SomeEnumType::FOO])]
            public Optional|string $bar = new Optional,

            // This creates rules as expected
            #[RequiredIf('type', [SomeEnumType::FOO])]
            public Optional|string $foobar 
        ) {
        }
    }

    // This will throw an error that $foobar is required, however ignore that $bar is also required.
    dd(UpdateProductData::validateAndCreate([
       'type' => SomeEnumType::FOO->value
     ]));

In this example, the you would expect both attributes: $bar and $foobar to be required, since $type == 'foo'. However only $foobar have a required rule.

This bug might extend to other validation rules such as RequireWith, however I have not tested that part.

✅ Expected behavior It would be expected that both attributes $bar and $foobar, would be required.

🖥️ Versions

Laravel: 10.0 Laravel Data: 4.10.0 PHP: 8.3.9

rubenvanassche commented 3 weeks ago

Technically I would not initialize an optional property, the package will do this for you (I'm even thinking of making the optional constructor private in v5). But it could be a beginner mistake so the issue should be fixed.

RasmusGodske commented 3 weeks ago

I get that, however the problem with that solution, is the fact the intelisense would act out, because according to it, the field is required. You would have to provide it either a Optional object or an int. By initializing it to Optional, the intellisense also see it as optional.

Initializing the property

class UpdateProductData extends Data
{
    public function __construct(
        #[Required]
        #[WithCast(EnumCast::class)]
        public SomeEnumType $type,

        #[RequiredIf('type', [SomeEnumType::FOO])]
        public Optional|string $bar = new Optional,
    ) {}
}

// InteliSense will accept this
$updateProductData = new UpdateProductData(
   SomeEnumType::FOO
)

Not initializing the property

class UpdateProductData extends Data
{
    public function __construct(
        #[Required]
        #[WithCast(EnumCast::class)]
        public SomeEnumType $type,

        #[RequiredIf('type', [SomeEnumType::FOO])]
        public Optional|string $bar;
    ) {}
}

// InteliSense will show error
// Expected 2 arguments. Found 1.intelephense(P1005)
$updateProductData = new UpdateProductData(
   SomeEnumType::FOO
)