FormRequest side effects after introducing Laravel Precognition

lucaspanik commented 1 year ago

Laravel Version

v10.27.0

PHP Version

8.2.11

Database Driver & Version

No response

Description

In version 9.32.0 and earlier, the validationData() method was called before the rules() method in the FormRequest, which provided the possibility of changing the data before being sent to rules().

https://github.com/laravel/framework/blob/aae3b59f82434176546c9dd10804adda16da5278/src/Illuminate/Foundation/Http/FormRequest.php#L110-L116

As can be seen in the print below.

With the acceptance of PR [9.x] Introduce Laravel Precognition #44339, a "side effect" was introduced in the sequences in which methods are called.

From version 9.33.0 and later, the rules() method is being called before validationData(), not giving the possibility of making changes to the data before being sent to rules();

https://github.com/laravel/framework/blob/13665b7e15dbcbecb3651acc19ba8818da6fa0a9/src/Illuminate/Foundation/Http/FormRequest.php#L110-L122

As can be seen in the print below.

I confirmed that the same code is in version 10 as well:

https://github.com/laravel/framework/blob/10.x/src/Illuminate/Foundation/Http/FormRequest.php#L119C96-L119C96 https://github.com/laravel/framework/blob/6bfd7550c94ef8666b3312cae577ddd30db03e03/src/Illuminate/Foundation/Http/FormRequest.php#L117-L133

Steps To Reproduce

Use the code below to notice that there is a change in the sequence in which the methods are executed:

// Route

Route::post('home', [HomeController::class, 'index']);

// Controller
class HomeController extends Controller
{
    public function index(HomeTestRequest $request)
    {
        dd('HomeController');        
    }
}

// Form Request
class HomeTestRequest extends FormRequest
{
    /**
     * Get data to be validated from the request.
     *
     * @return array
     */
    public function validationData()
    {
        dump('validationData');

        $this->merge([
            'foo' => 'bar',
        ]);

        return $this->all();
    }

    protected function rules()
    {
        dump('rules');

        dump($this->all());

        return [];
    }
}

lucaspanik commented 1 year ago

Possibility of change 1 (Arrow Functions):

   /**
     * Create the default validator instance.
     *
     * @param  \Illuminate\Contracts\Validation\Factory  $factory
     * @return \Illuminate\Contracts\Validation\Validator
     */
    protected function createDefaultValidator(ValidationFactory $factory)
    {
        $rules = fn () => method_exists($this, 'rules') ? $this->container->call([$this, 'rules']) : [];

        $validator = $factory->make(
            $this->validationData(), $rules(),
            $this->messages(), $this->attributes()
        )->stopOnFirstFailure($this->stopOnFirstFailure);

        if ($this->isPrecognitive()) {
            $validator->setRules(
                $this->filterPrecognitiveRules($validator->getRulesWithoutPlaceholders())
            );
        }

        return $validator;
    }

Possibility of change 2 (remove variable):

   /**
     * Create the default validator instance.
     *
     * @param  \Illuminate\Contracts\Validation\Factory  $factory
     * @return \Illuminate\Contracts\Validation\Validator
     */
    protected function createDefaultValidator(ValidationFactory $factory)
    {
        $validator = $factory->make(
            $this->validationData(), 
            method_exists($this, 'rules') ? $this->container->call([$this, 'rules']) : [],
            $this->messages(), 
            $this->attributes()
        )->stopOnFirstFailure($this->stopOnFirstFailure);

        if ($this->isPrecognitive()) {
            $validator->setRules(
                $this->filterPrecognitiveRules($validator->getRulesWithoutPlaceholders())
            );
        }

        return $validator;
    }

Possibility of change 3 (create a variable for validationData):

   /**
     * Create the default validator instance.
     *
     * @param  \Illuminate\Contracts\Validation\Factory  $factory
     * @return \Illuminate\Contracts\Validation\Validator
     */
    protected function createDefaultValidator(ValidationFactory $factory)
    {
        $validationData = $this->validationData();
        $rules = method_exists($this, 'rules') ? $this->container->call([$this, 'rules']) : [];

        $validator = $factory->make(
            $validationData, $rules,
            $this->messages(), $this->attributes()
        )->stopOnFirstFailure($this->stopOnFirstFailure);

        if ($this->isPrecognitive()) {
            $validator->setRules(
                $this->filterPrecognitiveRules($validator->getRulesWithoutPlaceholders())
            );
        }

        return $validator;
    }

timacdonald commented 1 year ago

@lucaspanik although I can see the order of method execution has changed, I'm not really sure what we are trying to do here so I might need a bit more context so i can write a test for this.

not giving the possibility of making changes to the data before being sent to rules();

The data is not sent to the rules method. I think this is what I'm tripping up on.

These two methods are rather independent, so I can only assume you are somehow creating a dependency between them with stored state?

If you could you provide a code snippet that shows how this is functionally impacting your app I can dig in and try and get this sorted for you.

Thanks.

lucaspanik commented 1 year ago

Thank you @driesvints and @timacdonald for your time and review.

The need arose at a time when I needed to make modifications to the data that arrived via a Request, before they were validated by rules().

Searching the FormRequest.php code I found the method that said to obtain the data to be validated in the Request https://github.com/laravel/framework/blob/1cae8375e8bbcec8bd1d47102184d34a2ee77747/src/Illuminate/Foundation/Http/FormRequest.php#L135-L143

After evaluating the birth of the validationData() method in the PR and commit:

I understood that the validationData() method was born to change the Request data before rules() was reached, indirectly creating a "dependency" between them.

Contextualizing my needs in code

I receive a word/phrase (term) in Request that needs to be registered originally in English and after registration, the team carries out translations into other languages. This way I create a key for this term and a unique hash to avoid duplication in the DB.

Using validationData(), I can use the term received to create the key and key_hash (for example) before validating its existence and use the key_hash in the controller to register correctly.

class TranslationTermRequest extends FormRequest
{
    /**
     * Get data to be validated from the request.
     *
     * @return array
     */
    public function validationData()
    {
        $term = $this->get('term');
        $key = snakeCase(mb_strtolower($term));

        $this->merge([
            'id'            => $this->route()->parameter('id'),
            'key'           => $key,
            'original_term' => $term,
            'key_hash'      => hash('sha512', $key),
        ]);

        return $this->all();
    }

    /**
     * Get the validation rules that apply to the request.
     *
     * @return array
     */
    public function rules()
    {
        $id = $this->get('id');

        return [
            'key' => [
                "required",
                "string",
                "unique:translation_keys,key,{$id},translation_key_id",                
            ],

            'original_term' => [
                'required',
                'string',
            ],

            'key_hash' => [
                "required",
                "string",
                "unique:translation_keys,key_hash,{$id},translation_key_id",
            ],
        ];
    }
}

NickSdot commented 1 year ago

@lucaspanik isn't for your scenario prepareForValidation() the better choice?

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

Edit: it is. The side effect shouldn't matter with this anymore because Precognition is handled after.

https://github.com/laravel/framework/blob/10.x/src/Illuminate/Validation/ValidatesWhenResolvedTrait.php

lucaspanik commented 1 year ago

Hi @NickSdot, thanks for your interaction.

I actually used validationData() because it existed/was born before prepareForValidation()

Jun 17, 2016 - validationData() https://github.com/laravel/framework/pull/13914

Nov 7, 2016 - prepareForValidation() https://github.com/laravel/framework/commit/36b0f5009a07e3bff3d64f6a42435a3dec8d9fd7

But I evaluated my code and using prepareForValidation() it worked as expected.

and that brought me a question:

If the correct option would be to use prepareForValidation(), what purpose would the validationData() method be for?

timacdonald commented 1 year ago

@lucaspanik prepareForValidation is a hook to do any required "work" before validation where as validationData is intended to return the data to be validated.

Gonna close this as I agree that prepareForValidation should be used in this case and this ordering has been released for a while now.

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

laravel / framework