Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add field-level and resource-level validation support #7

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add field-level and resource-level validation support #7

wants to merge 3 commits into from

Conversation

phpfour
Copy link
Member

@phpfour phpfour commented Jul 5, 2022

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:

public function fields(): array
{
    $id = $this->crud->getRequest()->input('id') ?? $this->crud->getRequest()->route('id');

    return [
        Text::make('Name')
            ->rules('required', 'min:5', 'max:255')
            ->sortable(),

        Email::make('Email')
            ->rules('required')
            ->creationRules('unique:users,email')
            ->updateRules('unique:users,email,'.$id),

        Password::make('Password')
            ->size(6)
            ->rules(['confirmed', 'min:8'])
            ->onlyOnForms(),

        Password::make('Confirm Password', 'password_confirmation')
            ->size(6)
            ->creationRules('required')
            ->onlyOnForms(),
    ];
}

As shown in the example, there are three methods to use:

  • rules: adds the validation rule for both create and update forms
  • creationRules: adds the validation rule only for create form
  • updateRules: adds the validation rule only for update form

Resource-level Validation Rules

In addition to the field-level validation, the validation rules/messages can also be added to the CrudResource itself. Example:

/** @inheritDoc */
public function validationRules(): array
{
    $id = $this->crud->getRequest()->get('id') ?? $this->crud->getRequest()->route('id');

    $rules = [
        'name' => 'required|min:5|max:255',
        'role' => 'required',
    ];

    $rules['email'] = $this->crud->getOperation() === 'create'
        ? 'required|unique:users,email'
        : 'required|unique:users,email,'.$id;

    $rules['password'] = $this->crud->getOperation() === 'create'
        ? 'required|confirmed'
        : 'confirmed';

    return $rules;
}

/** @inheritDoc */
public function validationMessages(): array
{
    return [
        'name.required' => 'You gotta give it a name, man.',
        'name.min' => 'You came up short. Try more than 2 characters.',
    ];
}

@phpfour phpfour self-assigned this Jul 5, 2022
@phpfour phpfour linked an issue Jul 5, 2022 that may be closed by this pull request
[Feature Request] Validation rules and messages inside CrudResource #3
Open
@phpfour phpfour requested a review from tabacitu July 6, 2022 11:16
@phpfour
Copy link
Member Author

phpfour commented Jul 6, 2022

Hey @tabacitu, let me know what you think 🙂

tabacitu
tabacitu requested changes Jul 19, 2022
View reviewed changes
Copy link
Collaborator

@tabacitu tabacitu left a comment •
edited

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.

src/Fields/Field.php
Comment on lines +454 to +470
/**
* 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);
}
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tabacitu tabacitu tabacitu requested changes

Assignees

@phpfour phpfour

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Validation rules and messages inside CrudResource
2 participants
@phpfour @tabacitu