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

FormRequest side effects after introducing Laravel Precognition #48685

Closed
lucaspanik opened this issue Oct 10, 2023 · 6 comments
Closed

FormRequest side effects after introducing Laravel Precognition #48685

lucaspanik opened this issue Oct 10, 2023 · 6 comments
Assignees

Comments

@lucaspanik
Copy link

lucaspanik commented Oct 10, 2023

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().

protected function createDefaultValidator(ValidationFactory $factory)
{
return $factory->make(
$this->validationData(), $this->container->call([$this, 'rules']),
$this->messages(), $this->attributes()
)->stopOnFirstFailure($this->stopOnFirstFailure);
}

As can be seen in the print below.
image


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();

protected function createDefaultValidator(ValidationFactory $factory)
{
$rules = $this->container->call([$this, 'rules']);
if ($this->isPrecognitive()) {
$rules = $this->filterPrecognitiveRules($rules);
}
return $factory->make(
$this->validationData(), $rules,
$this->messages(), $this->attributes()
)->stopOnFirstFailure($this->stopOnFirstFailure);
}

As can be seen in the print below.
image


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

protected function createDefaultValidator(ValidationFactory $factory)
{
$rules = 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;
}

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
Copy link
Author

lucaspanik commented Oct 10, 2023

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
Copy link
Member

@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
Copy link
Author

lucaspanik commented Oct 11, 2023

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

/**
* Get data to be validated from the request.
*
* @return array
*/
public function validationData()
{
return $this->all();
}

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
Copy link
Contributor

NickSdot commented Oct 11, 2023

@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
Copy link
Author

Hi @NickSdot, thanks for your interaction.

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

Jun 17, 2016 - validationData()
#13914

Nov 7, 2016 - prepareForValidation()
36b0f50

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
Copy link
Member

timacdonald commented Oct 11, 2023

@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

Screenshot 2023-10-12 at 10 28 48 am

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

No branches or pull requests

4 participants