Framework: Illuminate\Validation\Validator::validated() may return data even if validation failed

[LastDragon-ru]

• Laravel Version: 5.8.35
• PHP Version: 7.2
• Database Driver & Version: Mysql 5.7

Description & Steps To Reproduce:

In our app validation logic is complex and Request mock much simpler than testing controller, but there is another bug in Validator::validated() - this method must check that messages empty, but it check that invalid() properties are empty... So vaidated() return data when it shouldn't.

    public function testValidated() {
        $input   = [];
        $request = new class extends FormRequest {
            public function rules() {
                return [
                    'property' => 'nullable',
                ];
            }

            protected function withValidator(Validator $validator) {
                $validator->after(function (Validator $validator) {
                    $validator->errors()->add('property', 'Always failed.');
                });
            }
        };

        $request->setContainer($this->app);
        $request->setRedirector($this->app->make(Redirector::class));
        $request->initialize($input);

        // Create validator (see https://github.com/laravel/framework/pull/30157)
        try {
            $request->validateResolved();
            $this->assertTrue(false); // never happens
        } catch (ValidationException $exception) {
            // ignore
        }

        // Validation failed and 'validated()' must throw exception...
        $this->expectException(ValidationException::class);

        $request->validated();
    }

[LastDragon-ru]

If someone wants to fix this 😄

class MyRequest extends FormRequest {
    public function validated() {
        // Fixes 
        // - https://github.com/laravel/framework/issues/30158
        // - https://github.com/laravel/framework/pull/30157
        $validator = $this->getValidatorInstance();

        if ($validator->errors()->isNotEmpty()) {
            throw new ValidationException($validator);
        }

        return parent::validated();
    }
    // </editor-fold>
}

[Arkanius]

It's a little wierd to me when someone tries to mock a formRequest. IMO you should use integration tests instead of mocking a formRequest

[LastDragon-ru]

Unit test is a method that instantiates a small portion of our application and verifies its behavior independently from other parts

Do you think that testing a full application is better than testing the small isolated part? I guess - no. So I don't see any reason why mocking FromRequest is a bad practice.

[X-Coder264]

I don't have time to investigate whether this is a bug or if it's expected behavior, but I'd like to share my thoughts on the testing stuff.

Do you think that testing a full application is better than testing the small isolated part? I guess - no.

Define "better". It all depends on the context and perspective.

From a client perspective the value that you get out of testing (the validation in) the whole application flow via a functional test is much greater than this kind of unit test. Testing small stuff in isolation sounds great on paper, but it should be done properly not to couple the test too much to the implementation and it should be done only where it makes sense (and IMHO in the case of form requests in Laravel it doesn't).

So I don't see any reason why mocking FromRequest is a bad practice.

From a dev perspective, by writing an unit test such as the one you pasted above, you get a test that is as coupled as it possibly could be to the implementation (and what is even worse it is totally coupled to the inner workings of form requests in Laravel). Just take a look at all the stuff you had to do to even setup the test in the first place (you basically replicated half of the Laravel internal stuff that Laravel would usually do for you), The smallest change in the Laravel internals in this part of the code could break your test which is obviously something that you don't want (and if this is the case what does it say about the "isolation" of this unit that's being tested?).

Any attempt to refactor this code is very likely to break such a test while moving the validation logic to some other class/removing the form request class means that the whole test can just be thrown away as nothing from this test could be reused for the new implementation. This makes such a test very non refactor friendly / a maintenance burden.

On the other hand a functional test gives you a much higher confidence that your app works as expected and because it treats the whole app as a black box you are free to refactor the validation logic in any way you see fit and if the behavior stays the same your tests will pass without having to adjust anything in them (it's a win-win). That's why I think that the time spent to investigate and replicate Laravel form request internal stuff could've been better spent on writing a proper functional test.

[LastDragon-ru]

this is a bug

They both look like regressions:

• #26604 - previously used Validator::validate() (without d) throw exception if messages is not empty
• https://github.com/laravel/framework/commit/479d16d77a72b90e69526844b15ac13891dd0fb3#diff-93a08199642b328e7c151fef15179216 - definetelly a @taylorotwell's bug ($this->validator set only in getValidatorInstance())

Define "better". It all depends on the context and perspective.

Yep, and in my case much better test FormRequest without controller otherwise I should create a lot of models (in db) and/or mock a lot of objects. So, in theory, you are right about function tests but in practice, if you want test difficult form you will get tons of mocks/factories in each test and thousand lines of codes. This is also not so good.

The second problem with laravel functional testing out the box - it is not ideal, eg:

• there is no assertExactJsonStructure, only assertJsonStructure that allows additional properties in the response - if someone removes "Model::$hidden" you will never know about it because all tests with assertJsonStructure will pass. ...
• "Response status code [500] is not a successful status code. Failed asserting that false is true." - very very informative.

There is no ideal way, unfortunately :(

From a dev perspective, by writing an unit test such as the one you pasted above, you get a test that is as coupled as it possibly could be to the implementation (and what is even worse it is totally coupled to the inner workings of form requests in Laravel)

If you mean initial comment - the test inside it only for this issue (and probably should be part of laravel).

For aplication:

$model   = factory(Model)->make();
$request = Mockery::mock(FormRequest);

$request->shouldReceive('getModel`)->andReturn($model);

$this->assertEquals([... rules ...], $request->rules());

you basically replicated half of the Laravel internal stuff that Laravel would usually do for you

This just means that validation and request (and binding too) are ugly... Eg, why laravel do this?

$request->setContainer($this->app)->setRedirector($this->app->make(Redirector::class));

[driesvints]

Hey there,

Unfortunately we don't support this version anymore. Please check out our support policy on which versions we are currently supporting. Can you please try to upgrade to the latest version and see if your problem persists? We'll help you out and re-open this issue if so.

Thanks!

[LastDragon-ru]

Can you please try to upgrade to the latest version and see if your problem persists?

@driesvints, Yep. As I see all related code almost the same, and test failed in fresh 6.0.2 => so bug(s) exists.

test.php

<?php

namespace Tests\Unit;

use Illuminate\Foundation\Http\FormRequest;
use Illuminate\Routing\Redirector;
use Illuminate\Validation\ValidationException;
use Illuminate\Validation\Validator;
use Tests\TestCase;

class ExampleTest extends TestCase {
    public function testValidated() {
        $input   = [];
        $request = new class extends FormRequest {
            public function rules() {
                return [
                    'property' => 'nullable',
                ];
            }

            protected function withValidator(Validator $validator) {
                $validator->after(function (Validator $validator) {
                    $validator->errors()->add('property', 'Always failed.');
                });
            }
        };

        $request->setContainer($this->app);
        $request->setRedirector($this->app->make(Redirector::class));
        $request->initialize($input);

        // Create validator (see https://github.com/laravel/framework/pull/30157)
        try {
            $request->validateResolved();
            $this->assertTrue(false); // never happens
        } catch (ValidationException $exception) {
            // ignore
        }

        // Validation failed and 'validated()' must throw exception...
        $this->expectException(ValidationException::class);

        $request->validated();
    }
}

[driesvints]

Okay I'll re-open this but atm don't have time to check into this.

[lk77]

Hello,

i don't see a case where message will be empty and errors will not be empty.
it's something that can happen outside of a test ? in a real scenario ?

[LastDragon-ru]

it's something that can happen outside of a test ?

probably no

[oprypkhantc]

if someone removes "Model::$hidden" you will never know about it because all tests with assertJsonStructure will pass. ...

You can use $this->assertExactJson() if you want to. Either way, such things should NEVER rely on a property of the model. Before returning any data, you should use a transformer (fractal one, json resource - whichever fits your needs), in which you should always specify a list of keys to return so that you never face any issues like this.

"Response status code [500] is not a successful status code. Failed asserting that false is true." - very very informative.

$this->withoutExceptionHandling()

I'll agree with @X-Coder264 - feature tests are a much better way of testing this. You can mock the logic if you want to speed up your tests.

[LastDragon-ru]

You can use $this->assertExactJson() if you want to.

It requires more setup, eg you must set Carbon::setTestNow() and etc. So assertExactJsonStructure much more useful in practice (you can find implementation in this or ideas repo...). I really don't understand why it is not included by default.

$this->withoutExceptionHandling()

Not always useful.

You can mock the logic if you want to speed up your tests.

Validation also a logic, isn't it? And requests/responses too. Why I can't test/mock them?

feature tests are a much better way of testing this

Only if logic is simple, when not you will have 100500 tests in one file - unreadable/unsupported/etc... Another case: imagine that you have FormRequest which used for several routes/methods - will you create the same validation tests for all of them? :)

IMHO much better split the huge test into small parts and do something like (concept, but actually seems it is possible).

$this
    ->actingAs($this->getUser())
    ->postJson("/test/action", [])
    ->assertController(function ($controller) {
        $this->assertInstanceOf(CustomController::class, $controller);

        $controller
            ->shouldHaveReceived('actionMethod')
            ->withArgs(function ($args) {
                $this->assertInstanceOf(CustomRequest::class, $args[0]);
                $this->assertInstanceOf(Model::class, $args[1]);

                return true;
            });
    })
    ->assertExactJsonStructure(/* ... */);

[oprypkhantc]

@LastDragon-ru
I'm pretty sure you can make a PR with assertExactJsonStructure and it would get accepted and use a macro for now.

Not always useful.

Well, it's exactly what you want. A way to debug your tests. Not ideal, but it works and it's not bad.

Why I can't test/mock them?

What I meant was you could write complete feature tests and mock (or swap a dependency) anything that's running slow, like external services/apis. This way you can test everything, decouple your tests from the code while still being able to run them quick.

Another case: imagine that you have FormRequest which used for several routes/methods - will you create the same validation tests for all of them? :)

It's rarely a problem. Only happens if you have a related entity updated alongside main entity in one request. If you encounter this, then you can extract your validation to a separate validator and test that validator using unit tests, yes. But once again, it's a rare occasion and it's not the same as trying to force FormRequest to do that.

IMHO much better split the huge test into small parts and do something like

It is if your logic repeats multiple times. Otherwise, it's just a way to make the support harder and tests test less.

[X-Coder264]

Validation also a logic, isn't it? And requests/responses too. Why I can't test/mock them?

Request/response are value objects, not services, they are not supposed to be mocked.

Another case: imagine that you have FormRequest which used for several routes/methods - will you create the same validation tests for all of them? :)

I don't think such a case even exists, but if it does the answer is absolutely yes. If you do TDD you would first write the tests and only then the implementation, the fact that the same form request would be reused would then just be an implementation detail about which my test wouldn't know anything. The test only cares about what my client will get as the final response. This allows for much easier refactoring down the road (if for example the common form request is no longer enough for that route and you need to create a new form request with additional validation rules) and such test would serve as a great and very easily readable "documentation" about how the endpoint works.

I would highly recommend that you watch "Lies You've Been Told About Testing" - Adam Wathan - Laracon Online 2017 and then take a look at Test-Driven Laravel. This is some really amazing content about TDD (and testing in general) produced by Adam Wathan.

[LastDragon-ru]

I'm pretty sure you can make a PR with assertExactJsonStructure

Macro already used (not ideal too). Not sure about PR - it's required a lot of time and no guarantee that it will be accepted...

It's rarely a problem. Only happens if you have a related entity updated alongside main entity in one request.

Nope, you can do something like to avoid code duplication:

Few examples

class UpdateRequest extends FormRequest {
    public function rules() {
        return array_merge(parent::rules(), [
            'name' => '...'
            'field_01' => '...',
            // ...
            'field_25' => '...',
        ]);
    }
}

class CreateRequest extends UpdateRequest {
    public function rules() {
        return array_merge(parent::rules(), [
            'onetime' => '...'
        ]);
    }
}

or

class OtherRequest extends UpdateRequest {
    public function rules() {
        $rules = parent::rules();

        return [
            'field_08' => $rules['field_08'],
            'field_15' => $rules['field_15'],
        ];
    }
}

or (if you need extended validation based on model properties)

class UpdateRequest extends Request {
    public function rules() {
        $model = $this->getMyModel();
        $rules = [
            'name' => ['required', 'min:3', 'max:255', Rule::modelUnique($model)],
            'url'  => ['required', 'url'],
        ];

        if ($model instanceof MyModel) {
            if ($model->trashed()) {
                unset($rules['url']);
            }
        }

        return $rules;
    }

    protected function getMyModel() {
        return $this->route('model');
    }
}

class CreateRequest extends UpdateRequest {
    protected function getMyModel() {
        return MyModel::class;
    }
}

or (actual for SPA)

abstract class BaseUploadRequest extends Request {
    public final function rules() {
        return [
            'file' => array_merge(['required', 'file'], 'mimes:'.implode(',', $this->getAllowedMimes())),
        ];
    }

    protected abstract function getAllowedMimes(): array;
}

class PngImageRequest extends BaseUploadRequest {
    protected function getAllowedMimes(): array {
        return ['png'];
    }
}

class JpgRequest extends BaseUploadRequest {
    protected function getAllowedMimes(): array {
        return ['jpg'];
    }
}

class ZipRequest extends BaseUploadRequest {
    protected function getAllowedMimes(): array {
        return ['zip'];
    }
}

etc

In any case, there is no easy way to test it, unfortunately.

If you encounter this, then you can extract your validation to a separate validator and test that validator using unit tests, yes.

And in this case, you will see this issue 🎉

[LastDragon-ru]

@X-Coder264, please check my previous comment you will find a few examples inside. About tdd, there is also another way

• Create tests for Request
• Create Request
• Create tests for Controller::method and check that Request used (no need test Request again)
• Create Controller::method
• Create tests for Controller::method2 and check that Request used
• Create Controller::method2
• and so on

Pros:

• less & more readable code
• place, where error happens almost always known => withoutExceptionHandling() not needed
• tests require a much less time => we will run them more often

Cons:

• we should be a bit more careful while massive refactoring (but we can always add additional tests for critical places)

[themsaid]

Closing this since the PR was closed (https://github.com/laravel/framework/pull/30157).

Quoting Taylor:

Do not mock FormRequests. Just run an integration test.

[LastDragon-ru]

@themsaid this bug is not related to #30157 or maybe there is a global problem "very hard to mock the Request"... In any case, both fixed in https://github.com/laravel/framework/issues/30158#issuecomment-537489330

[tomswinkels]

Same problem here.

I have fix this to added the validated function to my request class.

    public function validated()
    {
        parent::setContainer(app());
        parent::getValidatorInstance();

        return parent::validated();
    }