The behavior of the RequiredRuleInferrer is confusing

[alexrififi]

✏️ Describe the bug
If you make a property of a Data of type bool, then the Required auto rule will not be applied to it.

↪️ To Reproduce

use Illuminate\Validation\ValidationException;
use Spatie\LaravelData\Attributes\Validation\Required;
use Spatie\LaravelData\Data;
use Spatie\LaravelData\Exceptions\CannotCreateData;

it('successfully validated', function () {
    class SomeData1 extends Data
    {
        public function __construct(
            public string $just_string,
            public bool $i_am_bool,
        ) {
        }
    }

    $payload = ['just_string' => fake()->text()];
    $validated = SomeData1::validate($payload);
    expect($validated['just_string'])->toEqual($payload['just_string']);
});

it('validated, but not created', function () {
    class SomeData2 extends Data
    {
        public function __construct(
            public string $just_string,
            public bool $i_am_bool,
        ) {
        }
    }

    SomeData2::validateAndCreate(['just_string' => fake()->text()]);
})->throws(CannotCreateData::class);

it('fail validation', function () {
    class SomeData3 extends Data
    {
        public function __construct(
            public string $just_string,
            #[Required]
            public bool $i_am_bool,
        ) {
        }
    }

    SomeData3::validateAndCreate(['just_string' => fake()->text()]);
})->throws(ValidationException::class);

✅ Expected behavior

I expect that I won't have to manually add the Required attribute

🖥️ Versions

Laravel: 10.46.0
Laravel Data: 4.2.0
PHP: 8.3.1

---

The problem is here

laravel-data/src/RuleInferrers/RequiredRuleInferrer.php

Lines 38 to 40 in 6e20d6f

	if ($rules->hasType(BooleanType::class)) {
	return false;
	}

[rubenvanassche]

Yeah this has always been like this and has to do with checkboxes, they do not send a value when not being checked and thus validation would fail since the key is missing.

That being said, the behavior looks weird after all those years. Data isn't a request to data solution anymore, we're powering event sourcing objects with it, not even using a request in between. These days when using Inertia with react/vue, the boolean fields will be present from the start so I agree that this change we've made in the past now feels weird.

The problem is, that just adding the required rule is a massive breaking change since a lot of validation which was previously not throwing error will do now.

Since this is in a RuleInferrer, we could implement a new one combing all RuleInferrers like I suggested in #647, you could than manually use this rule inferrer and remove the other ones. in v5 this could become the default rule inferrer.

The problem, I'm currently working on typescript-transformer 3 and do not want to spend too much time on data (if that's even possible). So it will have to wait or someone should send in a well tested PR.

[verner-lall]

Another one that seems not to spec is a required array type throws validation error when the array is empty, which is a perfectly acceptable array. If someone needs there to be a minimum amount of elements in the array, they can add Min rule.

Would it not be better to infer a "present" rule instead of "required" for this?

class ItemTask extends Data {
  public function __construct(
          public readonly string $task,
          public readonly array $data,
  ) {}
}

ItemTask::validateAndCreate(['task' => 'lmao', 'data' => []]); 
// Illuminate\Validation\ValidationException  The data field is required.

[rubenvanassche]

Jup that's how Laravel works:

it('bla', function (){
    $v= Validator::make([
        'array' => [],
    ], [
        'array' => ['required', 'array']
    ]);

    dd($v->errors());
});

You'll need to use present is such cases.

[verner-lall]

I understand, I'm suggesting that the inferred rule should maybe rather be present than required for arrays, since it would be madness to make it nullable if i also accept an empty array. Can't fix it with an annotation either, would have to override the rules in this case to have the class types correspond to what is needed.

public static function rules(ValidationContext $context): array
{
    return ['data' => 'present|array'];
}

Edit: I realize I'm wrong in case of the annotation, sorry about that. It seems to work correctly with latest version...

[rubenvanassche]

I don't want to change how the default behaviour works in Laravel, making a property non nullable makes a property required in data. Even if it is an array.

Adding an annotation is working as far as I'm aware:

it('bla', function (){
    $data = new class  extends Data
    {
        #[Present]
        public array $array;
    };

    dd($data::getValidationRules(['array' => []]), $data::getValidationRules([]));
});

array:1 [
  "array" => array:2 [
    0 => "array"
    1 => "present"
  ]
] // /Users/ruben/Spatie/laravel-data/tests/DataTest.php:138
array:1 [
  "array" => array:2 [
    0 => "array"
    1 => "present"
  ]
] // /Users/ruben/Spatie/laravel-data/tests/DataTest.php:138

[spatie-bot]

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.