Framework: [>=5.7.14] Breaking change when asserting json structure.

[austinkregel]

• Laravel Version: >= 5.7.14
• PHP Version: 7.1.3
• Database Driver & Version: n/a

Description:

As of this commit: https://github.com/laravel/framework/commit/cfab97fbed78c1eddb9a570edcf69122ae197029, there seems to now be a bug where the second value passed to assertArrayHasKey() is not an array.

It appears that the commit is a breaking change for php versions less than 7.2.

More info.

During my team's investigation of this bug we were able to conclude that it does go away with a php upgrade, but that solution seems out of place for a minor release of Laravel. We also were able to conclude that as of the writing of this issue it only affects the two latest versions of laravel 5.7.14 and 5.7.15.

This issue seems to stem from the commit stated above (which was included in the .14 release, but not the .13 release)

When we deep dive further into why this might be happening and put a debugger inside the framework around this line from the 5.7.15 tag we saw that the data key from the paginator now had two items in it, one being a class and one being an array. The only other difference being that the keys were 10 and "10". So when the framework tries to assertArrayHasKey we error out because the item that shows up first (the "10") is an object, and is obviously not an array.

Steps To Reproduce:

1. Install a brand new Laravel project where the version of the framework is running 5.7.14 or greater, and a PHP version < 7.2
2. Create an entry in your routes/api.php file as follows. The goal of this entry is to mock the second page of a request based on the Laravel Paginator.

Route::get('test-failure', function () {
   $page = 2;
   $total = 1;
   $perPage = 15;

    $response = new \Illuminate\Pagination\LengthAwarePaginator([
       '16' => new \App\User(['name' => 'John Smith'])
    ], $total, $perPage, $page);
    return response()->json($response);
});

1. In the existing /tests/Feature/ExampleTest.php file, add the following test. It's goal is to just assert that we're getting the structure we'd expect from the paginator for our API..

    public function testThing()
    {
        $response = $this->json('get', '/api/test-failure');

        $response->assertJsonStructure([
            'data' => [
                '*' => [
                    'name'
                ]
            ]
        ]);
    }

If all the conditions described above are met when you run PHPUnit you should get something similar to this exception:
```
1) Tests\Feature\ExampleTest::testThing
PHPUnit\Framework\Exception: Argument #2 (No Value) of PHPUnit\Framework\Assert::assertArrayHasKey() must be a array or ArrayAccess

./vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestResponse.php:600
./vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestResponse.php:593
./vendor/laravel/framework/src/Illuminate/Foundation/Testing/TestResponse.php:598
./tests/Feature/ExampleTest.php:19

ERRORS!
Tests: 2, Assertions: 3, Errors: 1.

[nathanheffley]

In PHP 7.2 the reason this doesn't cause issues is because the new array with a key of 10 overwrites the original object with a key of "10" which looks like the intention of the new code that caused this breaking change.

[nathanheffley]

Here's another example of this bug that demonstrates how this happens even if you use features like the collection's forPage function:

Route::get('test-failure', function () {
    $data = collect([
        new \App\User(['name' => 'John Smith']),
        new \App\User(['name' => 'Jane Doe']),
        new \App\User(['name' => 'John Doe']),
        new \App\User(['name' => 'Jane Smith']),
    ])->forPage(2, 2);

    $response = new \Illuminate\Pagination\LengthAwarePaginator($data, 4, 2, 2);
    return response()->json($response);
});

If you replace the route in @austinkregel's example with this route, and use the same test, you will see the exact same issue when you assert the JSON structure on the second page. Even more strangely, if you change my route to use forPage(1, 2) the test will pass.

Tl;dr: using assertJsonStructure on the first page of a paginated json response is successful, using assertJsonStructure on the second page of a paginated json response is unseccessful. This was tested using PHP 7.1.15.

[driesvints]

@NathanHeffley I did some research on the pagination part and this is normal. When you're on page 2 the keys for the sliced data array will start with a non-zero number and thus json_encode will encode it to an object instead of an array. This is standard behavior: https://www.sitepoint.com/community/t/json-encode-sometimes-does-or-does-not-add-keys-for-array-elements/116226/2

We should ask ourselves if this is wanted and if we don't want to add an array_values call around the $this->items->toArray() call in the toArray method of the LengthAwarePaginator. Might send a PR in for that. Tests still pass if I do.

As for the bug with the assertJsonStructure there's indeed a discrepancy beween PHP 7.1 and >=7.2. I'm working on a PR with some tests to help prevent breakages in the future.

[driesvints]

Sent in a PR which now covers this scenario: https://github.com/laravel/framework/pull/26741

[austinkregel]

@driesvints and @NathanHeffley you guys are awesome!!