[5.4] ConvertEmptyStringsToNull middleware breaks confirmation validator with blank values

[chriswayoub]

• Laravel Version: 5.4.7
• PHP Version: 7.0.5
• Database Driver & Version: N/A

Description:

The ConvertEmptyStringsToNull middleware replaces blank strings in the input with null, which breaks the validation when comparing the input with the confirmation value.

Steps To Reproduce:

Create a test case with the following in a new Laravel project:

<?php

namespace Tests\Feature;

use Illuminate\Http\Request;
use Illuminate\Validation\ValidationException;
use Tests\TestCase;

class ConfirmValidatorTest extends TestCase {
    public function setUp() {
        parent::setUp();

        // set up route that validates a password confirmation
        \Route::post('/confirm-password', function (Request $request) {
            $validator = \Validator::make($request->all(), [
                'password' => 'confirmed'
            ]);

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

            return response('');
        });
    }

    public function testSamePasswordConfirmationValidates() {
        $response = $this->postJson('/confirm-password', [
            'password' => 'secret',
            'password_confirmation' => 'secret'
        ]);
        $response->assertStatus(200);
    }

    public function testWrongPasswordConfirmationDoesNotValidate() {
        $response = $this->postJson('/confirm-password', [
            'password' => 'secret',
            'password_confirmation' => 'wrong secret'
        ]);
        $response->assertStatus(422);
    }

    public function testBlankPasswordConfirmationDoesNotValidate() {
        $response = $this->postJson('/confirm-password', [
            'password' => 'secret',
            'password_confirmation' => ''
        ]);
        $response->assertStatus(422);
    }

    public function testBlankPasswordWithBlankConfirmationValidates() {
        $response = $this->postJson('/confirm-password', [
            'password' => '',
            'password_confirmation' => ''
        ]);
        $response->assertStatus(200);
    }
}

The testBlankPasswordWithBlankConfirmationValidates test fails:

PHPUnit 5.7.9 by Sebastian Bergmann and contributors.

...F.                                                               5 / 5 (100%)

Time: 146 ms, Memory: 10.00MB

There was 1 failure:

1) Tests\Feature\ConfirmValidatorTest::testBlankPasswordWithBlankConfirmationValidates
Expected status code 200 but received 422.
Failed asserting that false is true.

confirm-validator-test/vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestResponse.php:51
confirm-validator-test/tests/Feature/ConfirmValidatorTest.php:56

FAILURES!
Tests: 5, Assertions: 5, Failures: 1.

Solution:

In my case, I just removed the ConvertEmptyStringsToNull middleware since that's not what I want to happen anyway. However, since that middleware is enabled on the default install, it should probably work out of the box. The reason for failure is in the ValidatesAttributes class:

    protected function validateSame($attribute, $value, $parameters)
    {
        $this->requireParameterCount(1, $parameters, 'same');

        $other = Arr::get($this->data, $parameters[0]);

        return isset($other) && $value === $other;
    }

$other and $value are both null in this case, but the isset($other) returns false causing the validation to fail regardless. My first thought is to just remove the isset check:

return $value === $other;

However, I'm not sure why that isset check was in there in the first place or what effect changing it will have on any other use cases. I've attached a PR in case that's the direction you want to go, but I think no matter what this is giving unexpected results since the middleware is mangling the input values in the first place. It looks like maybe the isset check is just a carry-over from very early on to avoid undefined array access warnings possibly. Here is an excerpt from what looks like the original version back on 1/10/13:

return isset($this->data[$other]) and $value == $this->data[$other];

[kamui545]

Take a look at http://themsaid.com/laravel-convert-empty-strings-to-null-20170130/

[chriswayoub]

Adding nullable to the rules causes it to validate, but I don't think it addresses the underlying issue that two blank strings are not considered the "same" when the ConvertEmptyStringsToNull middleware is active. Plus, I don't think it should be necessary to have to account for that in your validation rules if the field really shouldn't be nullable in the first place (meaning the value is missing), but a blank string is allowed.

[themsaid]

@chriswayoub I agree that the isset check can be removed and it'd solve an issue when the two values are explicitly null even when the middleware isn't used.