-
Notifications
You must be signed in to change notification settings - Fork 3
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 field-level and resource-level validation support #7
base: master
Are you sure you want to change the base?
Conversation
Hey @tabacitu, let me know what you think 🙂 |
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.
Awesome! Haven't done a proper code review. Ran through this from a developer's perspective though, and only one potential problem stands out to me, see below.
/** | ||
* Set the validation rules for the field. | ||
*/ | ||
public function creationRules(string | array | Rule $rules): self | ||
{ | ||
$this->creationRules = $rules instanceof Rule || is_string($rules) ? func_get_args() : $rules; | ||
|
||
return $this; | ||
} | ||
|
||
/** | ||
* Determine the validation rules for the field in the time of creation. | ||
*/ | ||
protected function getCreationRules(): array | ||
{ | ||
return array_merge_recursive($this->rules, $this->creationRules); | ||
} |
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.
This is where validation + declarative syntax becomes a bit tricky, so bear with me please 😀 I'm not sure about using creationRules()
and updateRules()
. Why? Because people also have other operations that use Backpack fields. And this month (or next month the latest), we'll share tutorials and add a feature to DevTools to easily create operations that use Backpack forms (eg. Moderate, Reply, Ship, etc.). If creationRules()
and updateRules()
are hard-coded... it means there's no way devs can use this Resource syntax on custom operations, which... is not the end of the world, but... it's a shame.
Then again, the alternative isn't great either.
In Backpack v3.x we were pointing to the operation where those rules apply:
$this->crud->setFormRequest(ArticleRequest::class, 'create');
$this->crud->setFormRequest(ArticleRequest::class, 'update');
That was because we thought it would be easy to just define everything in one place (in Backpack's case, in the setup()
method). But after working on complex projects... it turns out for medium-complexity and high-complexity projects, you probably want to define each operation individually. So we came up with setupListOperation()
, setupCreateOperation()
and setupUpdateOperation()
.
Ok so that's the history 😅 What can we do here?
If this is indeed a problem worth solving (debatable), I see two options:
(A) magic methods that follow a convention, starting from the operation name (eg. if you do updateRules()
or createRules()
or replyRules()
even though that method doesn't exist, it will work; downside: magic, no IDE auto-completion;
(B) extra parameter for the rules()
method, eg.
->rules('required|min:5', 'create');
->rules('required|min:5', ['create', 'update']);
->rules('required|min:5'); // if missing it can default to both create and update
Related Issues
Original Issue: #3
Context and Purposes
In order to further simplify the usage of a CrudResource, the validations rules for the fields should reside in the CrudResource itself and/or to the fields themselves.
Challenges and Solutions
Field-level Validation Rules
In this update, we have added both the above support. Here's an example of using field level validation rules:
As shown in the example, there are three methods to use:
Resource-level Validation Rules
In addition to the field-level validation, the validation rules/messages can also be added to the CrudResource itself. Example: