[5.8] Fix numeric keys in resources

[staudenmeir]

Re-submit of #24339.

#23414 fixed an issue with MergeValue and an associative array. However, this left the $numericKeys parameter in removeMissingValues() unused and thereby broke associative arrays with numeric keys:

$test = new Test(['array' => [1 => 'foo', 2 => 'bar']]);
return new TestResource($test);

// expected
{"data":{"array":{"1":"foo","2":"bar"}}}

// actual
{"data":{"array":["foo","bar"]}}

This PR reverts #23414 and instead fixes the issue in filter(): $numericKeys has to be determined on the data of MergeValue, not on the original $data.

We also have to remove the tests from #25269.

The underlying code seems to have changed between now and then.

The merge() method has been renamed to mergeData().

Fixes #23595 and #24302.

[taylorotwell]

So, can we summarize breaking changes? What would need to be documented in the upgrade guide?

[staudenmeir]

It reverts the accidental breaking change
from #23414 and restores the original behavior: Numeric keys in associative arrays are preserved in the result.

[rodrigopedra]

Considering that #23414 changed the way resources treated associative arrays, one note about changes might be something like this:

Before 5.8, all associative arrays which the first element had a numeric index would be outputted as a JSON array, regardless if it contained non-numeric index.

// Before: 
$router->get('test', function (  ) {
    $data = [ 'data'=> [ '1' => 10, '2' => 20, 'total' => 30 ] ];

    return new \App\Http\Resources\MyResource($data);
});

// outputs: {"data":[10,20,30]}

Now associative arrays that contain a non-numeric index will be outputted as a JSON object.

// After: 

$router->get('test', function (  ) {
    $data = [ 'data'=> [ '1' => 10, '2' => 20, 'total' => 30 ] ];

    return new \App\Http\Resources\MyResource($data);
});

// outputs: {"data":{"1":10,"2":20,"total":30}};

Sample resource

<?php

namespace App\Http\Resources;

use Illuminate\Http\Resources\Json\JsonResource;

class MyResource extends JsonResource
{
    public function toArray( $request )
    {
        return [
            'data' => $this->resource[ 'data' ],
        ];
    }
}

EDIT: applied the patch and live tested the examples

[rodrigopedra]

Just for the records, these are additional cases with this PR applied:

1. Arrays only with numeric keys, but not starting at zero:

$router->get( 'test', function () {
    $data = [ 'data' => [ '1' => 10, '2' => 20, '3' => 30 ] ];

    return new \App\Http\Resources\MyResource( $data );
} );

// output: {"data":{"1":10,"2":20,"3":30}}

2. Array with only numeric keys, but starting at 0

$router->get( 'test', function () {
    $data = [ 'data' => [ '0' => 1, '1' => 10, '2' => 20, '3' => 30 ] ];

    return new \App\Http\Resources\MyResource( $data );
} );

// output: {"data":[1,10,20,30]}

In my opinion, this should be the expected output.

[taylorotwell]

We will need to make special note that if people are filtering their collection and not resetting their keys, it could break their application, especially when looping through the items in their JavaScript. They could have previously been relying on this behavior of the keys being reset. Now, the returned value will be a JavaScript object instead of a JavaScript array. Correct?

[staudenmeir]

Yes.

[taylorotwell]

It's still fairly uncertain to me if this is a breaking change worth making. In my opinion a JSON API returning keyed JavaScript objects that you then somehow iterate through is... weird? I personally have never worked with such an API.

I also fear the amount of people running a collection ->filter() on their DB results and then passing to a resource and this breaking those applications.

[rodrigopedra]

Well I partially agree.

In the case when there are string keys such as the examples with the total key I think an object shoudl be returning. Assuming that if the first index is a number and treat everything else as an array is bad.

On the other hand, I think the usage when someone filtered a collection and expects an array regardless if the indexes starts with zero, is a valid concern.

We could take two approaches:

1. Document to always use ->value() before sending to a collection to a resource (bad dev experience, in my opinion)

2. Check if a value being serialized is a collection and then apply ->values() implicitly

The second approach would not be a breaking change, as of today it converts to an array anyways.

[staudenmeir]

It has been the default behavior before #23414 and I haven't seen a lot of people stumbling over this.

You can argue about whether numeric keys make sense in JSON responses. But by allowing them, you at least give people a choice. You don't have to use them.

[SjorsO]

I agree with @staudenmeir that there should be a way to allow users to preserve keys. I and at least one colleague have run into problems with this in the past.

Adding a property protected $preserveKeys = false; to the base Resource could also be a solution. That way it isn't a breaking change, and you can override and enable it only when you need it.

[taylorotwell]

Yeah, I just think we need to be mindful throughout all these discussions we are about 10 people max having these discussions. Laravel has 100k+ Twitter followers alone. So we have to keep in mind the bigger picture.

[rodrigopedra]

And what about the solution @SjorsO proposed? I got it working and it keeps the current behavior, while allowing the "new" (or expected depending of POV) behavior to who needs it .

EDIT: to make it easier to test I set up a branch (from master) in a fork:

https://github.com/rodrigopedra/framework/tree/resource

<?php

namespace Illuminate\Http\Resources;

use Illuminate\Support\Arr;

trait ConditionallyLoadsAttributes
{
    /**
     * Flag if the array keys should be preserved
     *
     * @var  bool
     */
    protected $preserveKeys = false;

    /**
     * Filter the given data, removing any optional values.
     *
     * @param  array  $data
     * @return array
     */
    protected function filter($data)
    {
        $index = -1;

        foreach ($data as $key => $value) {
            $index++;

            if (is_array($value)) {
                $data[$key] = $this->filter($value);

                continue;
            }

            if (is_numeric($key) && $value instanceof MergeValue) {
                return $this->mergeData($data, $index, $this->filter($value->data));
            }

            if ($value instanceof self && is_null($value->resource)) {
                $data[$key] = null;
            }
        }

        return $this->removeMissingValues($data, $this->hasNumericKeys($data));
    }

    /**
     * Check if the given data should have its keys preserved
     *
     * @param  array  $data
     * @return bool
     */
    protected function hasNumericKeys($data) {
        if ($this->preserveKeys) {
            return array_values($data) === $data;
        }

        return ! empty($data) && is_numeric(array_keys($data)[0]);
    }

    /**
     * Merge the given data in at the given index.
     *
     * @param  array  $data
     * @param  int  $index
     * @param  array  $merge
     * @return array
     */
    protected function mergeData($data, $index, $merge)
    {
        if ($this->hasNumericKeys($merge)) {
            return $this->removeMissingValues(array_merge(
                array_merge(array_slice($data, 0, $index, true), $merge),
                $this->filter(array_values(array_slice($data, $index + 1, null, true)))
            ), true);
        }

        return $this->removeMissingValues(array_slice($data, 0, $index, true) +
                $merge +
                $this->filter(array_slice($data, $index + 1, null, true)));
    }

    /**
     * Remove the missing values from the filtered data.
     *
     * @param  array  $data
     * @param  bool  $numericKeys
     * @return array
     */
    protected function removeMissingValues($data, $numericKeys = false)
    {
        foreach ($data as $key => $value) {
            if (($value instanceof PotentiallyMissing && $value->isMissing()) ||
                ($value instanceof self &&
                $value->resource instanceof PotentiallyMissing &&
                $value->isMissing())) {
                unset($data[$key]);
            }
        }

        return $numericKeys ? array_values($data) : $data;
    }

    /**
     * Retrieve a value based on a given condition.
     *
     * @param  bool  $condition
     * @param  mixed  $value
     * @param  mixed  $default
     * @return \Illuminate\Http\Resources\MissingValue|mixed
     */
    protected function when($condition, $value, $default = null)
    {
        if ($condition) {
            return value($value);
        }

        return func_num_args() === 3 ? value($default) : new MissingValue;
    }

    /**
     * Merge a value into the array.
     *
     * @param  mixed  $value
     * @return \Illuminate\Http\Resources\MergeValue|mixed
     */
    protected function merge($value)
    {
        return $this->mergeWhen(true, $value);
    }

    /**
     * Merge a value based on a given condition.
     *
     * @param  bool  $condition
     * @param  mixed  $value
     * @return \Illuminate\Http\Resources\MergeValue|mixed
     */
    protected function mergeWhen($condition, $value)
    {
        return $condition ? new MergeValue(value($value)) : new MissingValue;
    }

    /**
     * Merge the given attributes.
     *
     * @param  array  $attributes
     * @return \Illuminate\Http\Resources\MergeValue
     */
    protected function attributes($attributes)
    {
        return new MergeValue(
            Arr::only($this->resource->toArray(), $attributes)
        );
    }

    /**
     * Retrieve a relationship if it has been loaded.
     *
     * @param  string  $relationship
     * @param  mixed  $value
     * @param  mixed  $default
     * @return \Illuminate\Http\Resources\MissingValue|mixed
     */
    protected function whenLoaded($relationship, $value = null, $default = null)
    {
        if (func_num_args() < 3) {
            $default = new MissingValue;
        }

        if (! $this->resource->relationLoaded($relationship)) {
            return value($default);
        }

        if (func_num_args() === 1) {
            return $this->resource->{$relationship};
        }

        if ($this->resource->{$relationship} === null) {
            return null;
        }

        return value($value);
    }

    /**
     * Execute a callback if the given pivot table has been loaded.
     *
     * @param  string  $table
     * @param  mixed  $value
     * @param  mixed  $default
     * @return \Illuminate\Http\Resources\MissingValue|mixed
     */
    protected function whenPivotLoaded($table, $value, $default = null)
    {
        return $this->whenPivotLoadedAs('pivot', ...func_get_args());
    }

    /**
     * Execute a callback if the given pivot table with a custom accessor has been loaded.
     *
     * @param  string  $accessor
     * @param  string  $table
     * @param  mixed  $value
     * @param  mixed  $default
     * @return \Illuminate\Http\Resources\MissingValue|mixed
     */
    protected function whenPivotLoadedAs($accessor, $table, $value, $default = null)
    {
        if (func_num_args() === 3) {
            $default = new MissingValue;
        }

        return $this->when(
            $this->resource->$accessor &&
            ($this->resource->$accessor instanceof $table ||
            $this->resource->$accessor->getTable() === $table),
            ...[$value, $default]
        );
    }

    /**
     * Transform the given value if it is present.
     *
     * @param  mixed  $value
     * @param  callable  $callback
     * @param  mixed  $default
     * @return mixed
     */
    protected function transform($value, callable $callback, $default = null)
    {
        return transform(
            $value, $callback, func_num_args() === 3 ? $default : new MissingValue
        );
    }
}

It keeps all the current tests and adds this one (based on @staudenmeir work)

    // tests/Integration/Http/ResrouceTest.php

    public function test_keys_are_preserved_if_flagged()
    {
        Route::get('/', function () {
            return new PostResourceWithPreservedKeys(new Post);
        });

        $response = $this->withoutExceptionHandling()->get(
            '/', ['Accept' => 'application/json']
        );

        $response->assertStatus(200);

        $response->assertJson([
            'data' => [
                'array' => [1 => 'foo', 2 => 'bar'],
            ],
        ]);
    }

Adding this resource fixture:

<?php

namespace Illuminate\Tests\Integration\Http\Fixtures;

class PostResourceWithPreservedKeys extends PostResource
{
    protected $preserveKeys = true;

    public function toArray($request)
    {
        return [
            'array' => [1 => 'foo', 2 => 'bar'],
        ];
    }
}

[taylorotwell]

I don't think PHP will allow you to override the value of a trait property with a different value?

[taylorotwell]

[rodrigopedra]

Weird, I ran locally, I am in Ubuntu using PHP 7.3.1.

PHPUnit 7.5.2 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.3.1-1+ubuntu18.04.1+deb.sury.org+1
Configuration: /home/rodrigo/code/open-source/framework/phpunit.xml.dist

...

OK, but incomplete, skipped, or risky tests!
Tests: 4005, Assertions: 9919, Skipped: 5.

(Basically the skipped ones are the ones testing the DynamoDB and one testing PDO ODBC which I have disabled locally)

But now that you mentioned it might be something introduced in this new PHP version.

Anyway I updated my branch to use a dynamic check on this property (using property_exists).

EDIT:

new commit is:

rodrigopedra@63ba76d

EDIT 2:

Link to compare to laravel/framework@master

master...rodrigopedra:resource

[rodrigopedra]

Regarding the property override, if you override in a child class it does not error:

<?php

trait SomeTrait
{
    public $foo = 'foo';
}

class BaseClass
{
    use SomeTrait;

    // public $foo = 'bar'; // raises error
}

class ChildClass extends BaseClass
{
    public $foo = 'bar'; // does not raise error
}

print_r( ( new ChildClass )->foo ); // output: bar

Which is inconsistent with the documentation, I think it might be a PHP bug (at least the PHPStorm linter flags it as a bug).

So as the MyResource extends JsonResource and this one uses the trait, this was why I didn't saw any warnings.

Anyways, as it is not the PHP documented behavior, I prefer to use the dynamic check.

EDIT:

As pointed out bu @SjorsO in the comment bellow, it is indeed an expected behavior. So I can revert on the explicit property if you want, but I think I prefer the property_exists approach, as it does not pollute any class extending a Resource

[SjorsO]

You can override trait properties if the trait is used by a parent class, you can't when the trait is used directly in the class.

[taylorotwell]

OK, well if we're able to have a preserveKeys flag I would be OK with that. I would prefer it be defaulted to false to preserve current behavior and people could set it to true in a base resource class all of their other resources inherit if that is the behavior they want.

[rodrigopedra]

Thanks @taylorotwell !

@staudenmeir can you update your PR? If you prefer I can send a new one.

[staudenmeir]

@rodrigopedra Please submit a new PR.

[rodrigopedra]

Submitted as PR #27384

@staudenmeir thanks for all the work towards this feature. If you could review it, I'd be glad to improve or fix any issues.