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.

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

[11.x] Added ModelExists rule #52505

Draft
wants to merge 3 commits into
base: 11.x
Choose a base branch
from

Conversation

pascalbaljet
Copy link
Contributor

@pascalbaljet pascalbaljet commented Aug 16, 2024

There is an exists rule to check the existence of a record in the database. You can already pass an Eloquent Model class as the table argument in the Rule::exists() method, but that just extracts the table and connection from the Model. You can apply additional constraints by chaining the where() method, but under the hood, it uses the Illuminate\Database\Query\Builder, not the Illuminate\Database\Eloquent\Builder, so you can't use scopes, for instance.

What we have found in our application is that we are repeating code by writing exists rules and reimplementing the constraints for which we actually already have scopes defined.

This new Rule::modelExists() rule lets you use scopes and other Eloquent Builder features:

Rule::modelExists(User::class)->active()->admin();

By default, it checks the model key (using whereKey()), but you can change that with the optional second argument:

Rule::modelExists(User::class, 'email');

Besides the Model class string, you can also pass a Builder or Model instance:

Rule::modelExists(new User);
Rule::modelExists(User::query()->active());

@Sairahcaz
Copy link

Would be super awesome if that one gets merged!

@onlime
Copy link
Contributor

onlime commented Aug 16, 2024

yes, that would be super awesome, indeed!

@pascalbaljet has developed this in my project and we're already using it in a lot of FormRequests, greatly streamlining our codebase. Was looking for this for a long time, as it never felt clean/Laravel-ish to use the In rule as workaround.
I am super happy about this clean and simple implementation. 👏

@Sairahcaz
Copy link

The topic has also been discussed a little bit here:
#49831

@newtonjob
Copy link

Would really be nice to have!

Shouldn't this ideally go together with the sister ModelUnique rule though?

Also, I'd have loved to see this use the current Rule::exists() signature and intelligently return the proper underlying rule class, based on whether a model class string was passed or not.

@onlime
Copy link
Contributor

onlime commented Aug 17, 2024

@newtonjob

Also, I'd have loved to see this use the current Rule::exists() signature and intelligently return the proper underlying rule class, based on whether a model class string was passed or not.

yes, that would be nice, but it would be a BC break, as Rule::exists() already accepted a model class string and resolved it to the table name (DatabaseRule trait's constructor). So if Rule::exist() would suddenly return the new ModelExists, this would probably not make it into L11.

@pascalbaljet
Copy link
Contributor Author

pascalbaljet commented Aug 17, 2024

Would really be nice to have!

Shouldn't this ideally go together with the sister ModelUnique rule though?

If this PR gets merged, I could do a follow-up PR for a unique equivalent.

Also, I'd have loved to see this use the current Rule::exists() signature and intelligently return the proper underlying rule class, based on whether a model class string was passed or not.

You can already pass a class string to Rule::exists(), which extracts the connection and table from the Model. In some cases, it preserves the class string (for example, when the table is prefixed). Of course I first thought about extending the existing rule, but under the hood, it uses the DatabasePresenceVerifier, which then uses the Illuminate\Database\Query\Builder. It is definitely possible to overcome all of this, but I found it cleaner to create a new rule specifically for Eloquent Models and the Eloquent Builder.

@taylorotwell
Copy link
Member

taylorotwell commented Aug 19, 2024

I don't mind this as a concept, but Rule::modelExists is just not the Laravel way. 😔 In my opinion we should adjust Rule::exists in Laravel 12.x.

There could be a way to kinda hack it into Laravel 11.x by using named arguments like Rule::exists(model: Foo::class).

@taylorotwell taylorotwell marked this pull request as draft August 19, 2024 02:51
@taylorotwell
Copy link
Member

Drafting this for now - mark as ready for review if you make any adjustments noted above.

@Neol3108
Copy link
Contributor

Should this not also be added here?

return in_array($rule, ['Unique', 'Exists']) ? ! $this->messages->has($attribute) : true;

* @param Builder<TModel>|TModel|class-string<TModel> $model
* @return \Illuminate\Validation\Rules\ModelExists<Builder<TModel>>
*/
public static function modelExists(Builder|Model|string $model, ?string $column = null): ModelExists
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about relations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses the Builder contract (Illuminate\Contracts\Database\Eloquent\Builder) which is implemented by all relation classes.

@siarheipashkevich
Copy link
Contributor

siarheipashkevich commented Aug 19, 2024

What do you think for adding "optimization" for array validation to avoid many requests to the DB?

For example: 'users.*' => Rule::modelExists($account->users())->many()

when we are calling ->many() method from validation rule then we will get an array of the keys and save the result to the variable, and when validation will running we will check the stored result with the given value...

@siarheipashkevich
Copy link
Contributor

I've implemented the same validation rule in my project, but will happy if it will be exists in the Laravel:

<?php

namespace App\Validation\Rules;

use Closure;
use Illuminate\Contracts\Database\Query\Builder;
use Illuminate\Contracts\Validation\ValidationRule;
use Illuminate\Translation\PotentiallyTranslatedString;

class ModelExists implements ValidationRule
{
    protected bool $many = false;

    protected array $modelIds = [];

    public function __construct(protected Builder $modelQuery, protected $withTrashed = false) {}

    public function many(): self
    {
        $this->many = true;

        $this->modelIds = $this->resolveModelQuery()->pluck('id')->all();

        return $this;
    }

    /**
     * @param Closure(string): PotentiallyTranslatedString $fail
     */
    public function validate(string $attribute, mixed $value, Closure $fail): void
    {
        if (!$this->passed($value)) {
            $fail('validation.exists')->translate();
        }
    }

    protected function passed(mixed $value): bool
    {
        return $this->many
            ? in_array($value, $this->modelIds)
            : $this->resolveModelQuery()->whereKey($value)->exists();
    }

    protected function resolveModelQuery(): Builder
    {
        return $this->modelQuery->when($this->withTrashed, fn (Builder $query) => $query->withTrashed())
            ->clone();
    }
}

: $this->query->whereKey($value);

if (! $this->query->exists()) {
$fail('validation.exists')->translate(['attribute' => $attribute]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove ['attribute' => $attribute] and leave only ->translate()

@pascalbaljet
Copy link
Contributor Author

I don't mind this as a concept, but Rule::modelExists is just not the Laravel way. 😔 In my opinion we should adjust Rule::exists in Laravel 12.x.

There could be a way to kinda hack it into Laravel 11.x by using named arguments like Rule::exists(model: Foo::class).

I'll refactor it to use named arguments first, and if you're not happy with it, I can always rework it for Laravel 12 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants