laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.32k stars 10.95k forks source link

Invokeable Rules don't work for Rule::forEach() under nested array property validation. #44756

Closed nearueng closed 1 year ago

nearueng commented 1 year ago

Description:

Validation: Having an invokeable Rule object as part of the Rule::forEach closure in a nested array throws an error on the latest version of Laravel.

The error is: trim(): Argument #1 ($string) must be of type string, Illuminate\Validation\InvokableValidationRule given

Stack trace:

TypeError : trim(): Argument #1 ($string) must be of type string, Illuminate\Validation\InvokableValidationRule given
 /var/www/html/vendor/laravel/framework/src/Illuminate/Validation/ValidationRuleParser.php:250
 /var/www/html/vendor/laravel/framework/src/Illuminate/Validation/ValidationRuleParser.php:232
 /var/www/html/vendor/laravel/framework/src/Illuminate/Validation/Validator.php:1030
 /var/www/html/vendor/laravel/framework/src/Illuminate/Validation/Validator.php:1011
 /var/www/html/vendor/laravel/framework/src/Illuminate/Validation/Validator.php:752
 /var/www/html/vendor/laravel/framework/src/Illuminate/Validation/Validator.php:709
 /var/www/html/vendor/laravel/framework/src/Illuminate/Validation/Validator.php:601
 /var/www/html/vendor/laravel/framework/src/Illuminate/Validation/Validator.php:417
 /var/www/html/vendor/laravel/framework/src/Illuminate/Validation/Validator.php:448
 /var/www/html/vendor/laravel/framework/src/Illuminate/Validation/Validator.php:490
 /var/www/html/vendor/livewire/livewire/src/ComponentConcerns/ValidatesInput.php:206

Steps To Reproduce:

            'additional_users.*.email' => [
                'bail',
                'required',
                'email',
                Rule::forEach(function ($value, $attribute) {
                    return [
                        new EnsureUserEmailDoesNotAlreadyExistInAdditionalUsersColumn($this->booking, $value),
                    ];
                }),
            ],

EnsureUserEmailDoesNotAlreadyExistInAdditionalUsersColumn is a new Invokeable rule and thus causes an error.

dyriavin commented 1 year ago

@nearueng I guess it will be nice to attach implementation of your Invokable rule.

nearueng commented 1 year ago

@dyriavin In this instant, I just converted it to an inline rule for now to keep it working, but I'll put the original implementation below. Interestingly enough, even a regular rule was causing an error here.

class EnsureUserEmailDoesNotAlreadyExistInAdditionalUsersColumn implements InvokableRule
{
    public function __construct(
        protected Booking $booking,
        protected ?string $email
    ) {
    }

    /**
     * Run the validation rule.
     *
     * @param  string  $attribute
     * @param  mixed  $value
     * @param  \Closure(string): \Illuminate\Translation\PotentiallyTranslatedString  $fail
     * @return void
     */
    public function __invoke($attribute, $value, $fail)
    {
        $emailAlreadyExists = collect($this->booking->additional_users)->pluck('email')->contains($this->email);

        if ($emailAlreadyExists) {
            $fail('This email already exists on this booking.');
        }
    }
}
driesvints commented 1 year ago

Heya, thanks for reporting.

We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as one separate commit on the main/master branch and share the repository here? Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Please do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.

Thanks!

driesvints commented 1 year ago

I'll re-open this once a repo is provided.

abbluiz commented 1 year ago

I'm having the exact same problem.

abbluiz commented 1 year ago

I have reproduced the error in Laravel v9.39.0, v9.38.0 and v9.36.4.

Had to downgrade to v9.19.0 which was the version I was using before upgrading, and then it worked.

@driesvints Apparently to reproduce you just have to use an InvokableRule as one of the rules used in a Rule::forEach(), not sure why you're asking for a repository.

aihowes commented 1 year ago

Are you sure this issue relates to the invokable rule and not how you're using Rule::forEach?

I've just been burned by the change in this PR that breaks how you/I'm using Rule::forEach.

If you change your validation to the following in the latest Laravel version, does it work?:

'additional_users.*.email' => Rule::forEach(function ($value, $attribute) {
    return [
        'bail',
        'required',
        'email',
        new EnsureUserEmailDoesNotAlreadyExistInAdditionalUsersColumn($this->booking, $value),
    ];
}),
abbluiz commented 1 year ago

@aihowes Now I'm not sure, but here's how I'm using it.

public function rules(): array
{
    return [
        'access_roles.*.role_id' => [
            'bail',
            'present',
            'nullable',
            'integer',
            'exists:roles,id',
            'distinct',
            Rule::forEach(fn ($value, $attribute) => [
                new RoleMustBeOfTheSameModule(
                    moduleIdArrayKey: Str::replace('role_id', 'module_id', $attribute)
                ),
            ]),
        ],
    ];
}

This works as expected in Laravel v9.19.0, but not in the latest versions. I will take a look in this PR you mentioned.

abbluiz commented 1 year ago

Now I get it. We have to include everything inside the Rule::forEach() callback:

public function rules(): array
{
    return [
        'access_roles.*.role_id' => [
            Rule::forEach(fn ($value, $attribute) => [
                'bail',
                'present',
                'nullable',
                'integer',
                'exists:roles,id',
                'distinct',
                new RoleMustBeOfTheSameModule(
                    moduleIdArrayKey: Str::replace('role_id', 'module_id', $attribute)
                ),
            ]),
        ],
    ];
}

Now it works, but it also breaks the way errors are returned in responses.

aihowes commented 1 year ago

Remove the array outside of the Rule::forEach:

public function rules(): array
{
    return [
        'access_roles.*.role_id' => Rule::forEach(fn ($value, $attribute) => [
            'bail',
            'present',
            'nullable',
            'integer',
            'exists:roles,id',
            'distinct',
            new RoleMustBeOfTheSameModule(
                moduleIdArrayKey: Str::replace('role_id', 'module_id', $attribute)
            ),
        ]),
    ];
}
nearueng commented 1 year ago

@aihowes thanks! Just came back to this and your method looks good!