[FormRequest] validated() method causes the validation to be called again.

[DmitryNiko]

• Laravel Version: 5.7.5
• PHP Version: 7.2.9
• Database Driver & Version: MySQL 5.7.23

Description:

The problem is getting the retrieve the validated input data with method validated(). When it is called, it re-initiates the request validation. It seems like 941c8c7 commit breaks this.

Steps To Reproduce:

1. Controller:

use App\Http\Requests\ClientStoreRequest;

class ClientController extends Controller
{
    public function store(ClientStoreRequest $request)
    {
        $this->clientService->createClient($request->validated());

        return redirect()->route('app.clients')->with('status', 'Client was successfully created!');
    }
}

2. FormRequest:

use Illuminate\Foundation\Http\FormRequest;
use Illuminate\Validation\Rule;

class ClientStoreRequest extends FormRequest
{
    public function rules()
    {
        return [
	    'full_name' => 'required|string|max:50',
	    'phone' => [
	        'required',
	        'max:50',
	        'regex:/^(\+[0-9]+)$/',
	        Rule::unique('clients')->where(function($query){
	            return $query->where('account_id', auth()->user()->account_id);
	        })
	    ],
	    'email' => 'nullable|email|max:50',
	    'info' => 'max:200'
        ];
    }
}

3. Debugbar:
   Since the rules have a check in the database, we can see a duplicated query to the database, which means that the method rules() was called twice.
   

[staudenmeir]

Please take a look at this @ttsuru. After #25128, calling validated() unnecessarily re-validates the data.

Can we store the validated data in ValidatesWhenResolvedTrait::validateResolved() and then just return it when validated() is called?

[ttsuru]

I'm sorry.
I think there are several ways to fix it.

1. Validator::validate() called fails() change to invalid()

2. Add validated() method to Validator

3. ValidatesWhenResolvedTrait::validateResolved() implements a method similar to Validate::validate()

[ttsuru]

1 is breaking change method to Validator::validate(). validate() validate only once.
2 requires to add contracts Illuminate\Contracts\Validation\Validator::validated()
3 requires to add contracts Illuminate\Contracts\Validation\Validator::getRules()

[gofish543]

@ankur91 Sorry about the duplication

I want to add that the scope of this issue is not limited to a few database queries being done twice. While annoying and inefficient, it isn't a breaking fault.

However, with regards to validation that uses One Time Password (OTP) keys or passwords, this issue becomes more of a breaking fault rather than an inconvenience. (Example, Google Recaptcha)

[mohammedmanssour]

Hi,
@ttsuru I think you're working to resolve this, right? If yes, where are you at it?
for me, adding the Illuminate\Contracts\Validation\Validator::validated() is the best choice we have. but I don't seem to understand why It's not a recommended option?

if you're not working on this one, please let me know, because I'd like to get this as my first contribution to Laravel 😃

[DmitryNiko]

@mohammedmanssour For me, this error is still a big problem in my app, which does not allow me upgrade to the latest version of Laravel. I also hope that this will be fixed soon.

[ttsuru]

pull request #26417.
Because no one was response, I hesitated pull request.

[driesvints]

Will be fixed once 5.8 is released.

[ghost]

@driesvints @ttsuru I did a fresh Laravel install from master branch ( 5.8.* ) to test PR ( #26419 ), the issue still exist. Am I messing something?

[driesvints]

@ttsuru can you confirm that this is actually fixed?

[ghost]

This problem is a big issue for a project I am working on..
For now, to solve this.. I am using this method ( which was in laravel 5.6.* ) and removed in later releases:

    /**
     * Get the validated data from the request.
     *
     * @return array
     */
    public function validated()
    {
        $rules = $this->container->call([$this, 'rules']);
        return $this->only(collect($rules)->keys()->map(function ($rule) {
            return explode('.', $rule)[0];
        })->unique()->toArray());
    }

[ttsuru]

@driesvints
I add test case and I found other problem.
I fixed this in #26486

[driesvints]

Please see my comment on said PR.

[gofish543]

Can this be merged into 5.7?
This is a critical fault introduced within the 5.7 released and it sounds like I am not the only one who is forced to use 5.6 until it is resolved.

[driesvints]

The current solution can't be merged in 5.7 because it contains breaking changes.

[LastDragon-ru]

The current solution cant be merged in 5.7 because it contains breaking changes.

It is not a "breaking changes" it is a "Changed realization" like #26314 😄

[robjbrain]

Adding my voice to this completely breaking form requests, as others have said if you validate via a 3rd party api a double validation won't work. As well as any other expensive validation from database queries or custom methods.

If you are really serious about waiting till 5.8 (3 months?) the you should update the docs to warn people not to use the example if they're using expensive api calls, databases queries or custom methods.

[rodrigopedra]

By reading the comments in the closed PR ( #26417 ) for the 5.7 branch I understand the only breaking change is the added validated method to the Validator interface.

Can't we apply the fix to 5.7 the fix without changing the interface? This way we would not break any app relying on the validator interface.

We have cases of methods that were not in the Contracts and were used throughout the framework. One case is the UrlGenerator@previous method that will be added to the UrlGenerator interface in 5.8 but is used in a lot of places in previous versions.

I don't think anyone relies on double validation for their own app. So the fix would in fact be the expected behavior.

Hitting the database twice when using DB based validations, or calling an API twice when using a custom rule is enough reason to fix this behavior in the current release.

If the solution is to wait, ok, I guess I don't have a choice. But I think we could consider to fix this issue in 5.7 as well.

[driesvints]

We're probably not going to fix this in the current 5.7 release. I realize that the current implementation isn't ideal but it's also not a critical bug. This will be fixed in the 5.8 release.

[robjbrain]

The easiest temporary fix for anyone having this issue would be to extend the FormRequest class and intercept the validated() method e.g:

namespace App\Http\Requests;

use \Illuminate\Validation\ValidationException;
use Illuminate\Foundation\Http\FormRequest as LaravelFormRequest;

class FormRequest extends LaravelFormRequest
{
    /**
     * The validated data
     *
     * @var array
     */
    protected $validatedData;

    /**
     * Return the validated data.
     *
     * @return array
     *
     * @throws \Illuminate\Validation\ValidationException
     */
    public function validated()
    {
        if ($this->fails()) {
            throw new ValidationException($this);
        }

        return $this->validatedData ?: $this->validatedData = parent::validated();
    }
}

It's not how it should be fixed within Laravel obviously, but it's the easiest fix and should be compatible with any changes made between now and 5.8.

[taylorotwell]

I think we should fix this in the 5.7 release and not change the contract.

[driesvints]

I resent the original PR without the contract for 5.7 ^

[henriquerdd]

Is this really fixed? I'm using laravel 5.8 and during a http test it happened to me.
I need to do some api calls on my validation (2 to be precise), so i've mocked my api clients:

$response = \Mockery::mock(\Psr\Http\Message\ResponseInterface::class);

$response->shouldReceive('getStatusCode')->andReturn(200);

$this->mock(ProductsApiClient::class, function ($mock) use ($response) {
    $mock->shouldReceive('head')->once()->andReturn($response);
});

$this->mock(CollectionsApiClient::class, function ($mock) use ($response) {
    $mock->shouldReceive('head')->once()->andReturn($response);
});

The validation logic is as follow:

public function rules()
{
    return [
        'productUuid' => ['required', 'uuid', function ($attribute, $value, $fail) {

            if (!$this->productExists($value)) {
                return $fail("Product '$value' not found");
            }
        }],
        'collectionUuid' => ['required', 'uuid', function ($attribute, $value, $fail) {

            if (!$this->collectionExists($value)) {
                return $fail("Collection '$value' not found");
            }
        }]
    ];
}

private function productExists($productUuid)
{
    $response = $this->productsApiClient->head("/products/$productUuid");
    return $response->getStatusCode() == 200;
}

private function collectionExists($collectionUuid)
{
    $response = $this->collectionsApiClient->head("/collections/$collectionUuid");
    return $response->getStatusCode() == 200;
}

The test itself:

$response = $this->json('POST', '/something', $body);

$response
    ->assertStatus(201)
    ...

But when i run the tests, phpunit screams:

1. Tests\Feature\SomeTest::testCreationWithValidInput
   Mockery\Exception\InvalidCountException: Method head() from Mockery_3_App_ProductsApiClient should be called
   exactly 1 times but called 2 times.