-
-
Notifications
You must be signed in to change notification settings - Fork 207
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
Add the ability to optionally merge automatically inferred rules with manual rules #848
base: main
Are you sure you want to change the base?
Changes from all commits
975a21c
a08ee9a
bb53227
52e3e81
fda7091
d12665d
d96851e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
<?php | ||
|
||
namespace Spatie\LaravelData\Attributes; | ||
|
||
use Attribute; | ||
|
||
#[Attribute(Attribute::TARGET_CLASS)] | ||
class MergeRules | ||
{ | ||
// | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
use Illuminate\Support\Arr; | ||
use Illuminate\Support\Str; | ||
use Illuminate\Validation\Rule; | ||
use Spatie\LaravelData\Attributes\MergeRules; | ||
use Spatie\LaravelData\Attributes\Validation\ArrayType; | ||
use Spatie\LaravelData\Attributes\Validation\Present; | ||
use Spatie\LaravelData\Support\DataClass; | ||
|
@@ -67,7 +68,7 @@ public function execute( | |
$dataRules->add($propertyPath, $rules); | ||
} | ||
|
||
$this->resolveOverwrittenRules( | ||
$this->resolveRules( | ||
$dataClass, | ||
$fullPayload, | ||
$path, | ||
|
@@ -223,7 +224,7 @@ protected function resolveToplevelRules( | |
} | ||
|
||
|
||
protected function resolveOverwrittenRules( | ||
protected function resolveRules( | ||
DataClass $class, | ||
array $fullPayload, | ||
ValidationPath $path, | ||
|
@@ -240,9 +241,19 @@ protected function resolveOverwrittenRules( | |
$path | ||
); | ||
|
||
$overwrittenRules = app()->call([$class->name, 'rules'], ['context' => $validationContext]); | ||
$manualRules = app()->call([$class->name, 'rules'], ['context' => $validationContext]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's keep this as overwrittenRules. |
||
|
||
foreach ($overwrittenRules as $key => $rules) { | ||
if ($this->shouldMergeRules($class)) { | ||
$manualRules = collect($manualRules)->map( | ||
fn (string|array $rules) => is_array($rules) ? $rules : explode('|', $rules) | ||
)->all(); | ||
Comment on lines
+247
to
+249
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use the Also: when we add an attribute not to validate the property it will still be validated we don't want that. I think a better solution would be to implement the logic within the loop where addRules isn't called since it replaces them but rather rules are merged over there (or not if the property shouldn"t be validated). |
||
|
||
$dataRules->rules = array_merge_recursive($dataRules->rules, $manualRules); | ||
|
||
return; | ||
} | ||
|
||
foreach ($manualRules as $key => $rules) { | ||
if (in_array($key, $withoutValidationProperties)) { | ||
continue; | ||
} | ||
|
@@ -278,4 +289,9 @@ protected function inferRulesForDataProperty( | |
$path | ||
); | ||
} | ||
|
||
protected function shouldMergeRules(DataClass $class): bool | ||
{ | ||
return $class->attributes->contains(fn (object $attribute) => $attribute::class === MergeRules::class); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for an extra function here, just inline the variable |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
<?php | ||
|
||
use Illuminate\Validation\ValidationException; | ||
use Spatie\LaravelData\Data; | ||
use Spatie\LaravelData\Tests\Fakes\DataWithMergedRuleset; | ||
use Spatie\LaravelData\Tests\Fakes\DataWithMergedStringRuleset; | ||
|
||
it('it will merge validation rules', function () { | ||
try { | ||
DataWithMergedRuleset::validate(['first_name' => str_repeat('a', 1)]); | ||
} catch (ValidationException $exception) { | ||
expect($exception->errors())->toMatchArray([ | ||
'first_name' => ['The first name field must be at least 2 characters.'] | ||
]); | ||
} | ||
|
||
try { | ||
DataWithMergedRuleset::validate(['first_name' => str_repeat('a', 11)]); | ||
} catch (ValidationException $exception) { | ||
expect($exception->errors())->toMatchArray([ | ||
'first_name' => ['The first name field must not be greater than 10 characters.'] | ||
]); | ||
} | ||
}); | ||
|
||
it('it will merge validation rules using string rules', function () { | ||
try { | ||
Data::validate(['first_name' => str_repeat('a', 1)]); | ||
} catch (ValidationException $exception) { | ||
expect($exception->errors())->toMatchArray([ | ||
'first_name' => ['The first name field must be at least 2 characters.'] | ||
]); | ||
} | ||
|
||
try { | ||
DataWithMergedStringRuleset::validate(['first_name' => str_repeat('a', 11)]); | ||
} catch (ValidationException $exception) { | ||
expect($exception->errors())->toMatchArray([ | ||
'first_name' => ['The first name field must not be greater than 10 characters.'] | ||
]); | ||
} | ||
|
||
try { | ||
DataWithMergedStringRuleset::validate(['first_name' => 'a123']); | ||
} catch (ValidationException $exception) { | ||
expect($exception->errors())->toMatchArray([ | ||
'first_name' => ['The first name field must only contain letters.'] | ||
]); | ||
} | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
<?php | ||
|
||
namespace Spatie\LaravelData\Tests\Fakes; | ||
|
||
use Spatie\LaravelData\Attributes\MergeRules; | ||
use Spatie\LaravelData\Attributes\Validation\Max; | ||
use Spatie\LaravelData\Data; | ||
|
||
#[MergeRules] | ||
class DataWithMergedRuleset extends Data | ||
{ | ||
public function __construct( | ||
#[Max(10)] | ||
public string $first_name, | ||
) { | ||
} | ||
|
||
public static function rules(): array | ||
{ | ||
return [ | ||
'first_name' => ['min:2'] | ||
]; | ||
Comment on lines
+18
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inline these classes within the tests, that makes the fakes directory a bit less heavy. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
<?php | ||
|
||
namespace Spatie\LaravelData\Tests\Fakes; | ||
|
||
use Spatie\LaravelData\Attributes\MergeRules; | ||
use Spatie\LaravelData\Attributes\Validation\Max; | ||
use Spatie\LaravelData\Data; | ||
|
||
#[MergeRules] | ||
class DataWithMergedStringRuleset extends Data | ||
{ | ||
public function __construct( | ||
#[Max(10)] | ||
public string $first_name, | ||
) { | ||
} | ||
|
||
public static function rules(): array | ||
{ | ||
return [ | ||
'first_name' => 'min:2|alpha' | ||
]; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us rename this to appendOverwrittenRules, resolveRules is too general.