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

Outsourcing validation rules to validationRules() method in FormRequest@createDefaultValidator() method breaks validation #50083

Closed
lakkes-ra opened this issue Feb 14, 2024 · 5 comments · Fixed by #50084
Labels

Comments

@lakkes-ra
Copy link

lakkes-ra commented Feb 14, 2024

Laravel Version

10.43

PHP Version

8.2.14

Database Driver & Version

Docker MySQL image mysql:8.0

Description

References #49860

This change breaks my custom FormRequest rules where I use the merge() function.

class CategoryAvailabilityRequest extends FormRequest
{
    /**
     * Determine if the user is authorized to make this request.
     */
    public function authorize(): bool
    {
        return true;
    }

    /**
     * Get the validation rules that apply to the request.
     *
     * @return array<string, \Illuminate\Contracts\Validation\ValidationRule|array<mixed>|string>
     */
    public function rules(): array
    {
        $dateRange = explode(' - ', $this->date_range);
        $this->merge([
            'start_date' => $dateRange[0],
            'end_date' => $dateRange[1],
        ]);

        return [
            'start_date' => ['required'],
            'end_date' => ['required', 'after_or_equal:start_date'],
        ];
    }
}

With Update 10.43 it returns validation errors "start date must be filled" and "end date must be filled". When I revert the changes made in FormRequest@createDefaultValidator(ValidationFactory $factory) in #49860, it works again.

It is kinda weird, since the problem only seems to be coming from outsourcing $rules = method_exists($this, 'rules') ? $this->container->call([$this, 'rules']) : []; into a method. It all works fine, if I do it inside the createDefaultValidator() method

Steps To Reproduce

Create a custom FormRequest and use merge inside the rules() function like written above.

@driesvints
Copy link
Member

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!

@lakkes-ra
Copy link
Author

Hi @driesvints, thanks for the fast response!

I followed your instructions reproduced the issue in this repository: https://github.com/lakkes-ra/bug-report.
This is the exact commit: lakkes-ra/bug-report@1c8fe06

@rojtjo
Copy link
Contributor

rojtjo commented Feb 14, 2024

This is happening because the PR changed the order in which the rules and data are retrieved.

While this is a breaking change, I don't think the merging of data is supposed to happen in the rules method. You should be using the prepareForValidation method on your form request for that.

See: https://laravel.com/docs/10.x/validation#preparing-input-for-validation

@driesvints
Copy link
Member

@rojtjo thanks for noting that! I do think this is a breaking change. I've sent in an easy fix for this here: #50084

@lakkes-ra
Copy link
Author

@rojtjo that works, thanks for pointing out & thanks to @driesvints for fixing!

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

Successfully merging a pull request may close this issue.

3 participants