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

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

About this issue

Original URL
State: closed
Created 5 years ago
Comments: 19 (18 by maintainers)

Most upvoted comments

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.

X-Coder264 on Oct 2, 2019