[5.6] Fix numeric keys in resources

[staudenmeir]

#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 response: {"data":{"array":{"1":"foo","2":"bar"}}}
Actual response:   {"data":{"array":["foo","bar"]}}

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

Fixes #23595 and fixes #24302.

[taylorotwell]

This is probably a breaking change to existing applications that are not anticipating the JSON response to contain numerically keyed things and are expected arrays instead of objects. I don't want to "fix" this mid-release for that reason. I'm not really sure returning numerically keyed resource responses is a great idea in the first place. That's odd to interact with in JS.

[staudenmeir]

I would argue that it's the expected behavior and #23414 was the breaking change.

Regardless of the behavior we choose, there shouldn't be an unused method parameter.

[Pluiesurlavitre]

Agreed, this was a breaking change in my application.

[rodrigopedra]

I faced a problem when trying to return and index keyed object using a resource, and analyzing this PR and PR #23414 it seems the fix proposed in this commit makes more sense as it fixes the problem where it happens.

I am sorry if I am sounding rude, English is not my native language, I saw that the merged PR (#23414) was submitted by @themsaid who is someone I admire and seems to be very careful and able.

As @taylorotwell pointed out, it is very odd to use integer keys on an object in JS, but I faced this issue while porting an project to 5.6.

It uses a client-side chart component written in AngularJS to plot a linear chart of monthly accumulated sales.

This component expects that each month series to be as follow (shortened for brevity):

[{"1":100.0,"2":115.0, ... ,"total":1500.0}, ...]

Where each day is represented by a numeric key. I know it is very odd and I would not write it like this, but it was written by a someone else from outside the company.

For now I am keeping the old code and not using a resource for the route which returns this data as we cannot rewrite this client-side library.

But it took me sometime to find out, it would be nice if we could have both behaviors.

[robclancy]

@taylorotwell I agree with having numeric keys in the response is not a good idea but the resource strips keys of any array nested anywhere, this can easily include custom json fields that use ids as keys. Basically it breaks any custom data formats nested into a response if you use numbers.

It should only change the keys for resources imo. Leaving arrays as they are. I got to hack the JSON to have an _ now I guess and now that I know of this it won't be an issue but if one of my devs runs into this they could waste hours.

EDIT: it only needs the first key to be a number and it replaces them all :/

[SethTompkins]

@taylorotwell this was a breaking change to our application, we practice the methods outlined here for normalizing our frontend state to allow for easy transaction and lookups with api resources: https://redux.js.org/recipes/structuringreducers/normalizingstateshape

The idea is that you can return lookup tables for your resources so that dealing with relational data is much simpler, referencing an entity like thing = things.byId[thingId] rather than having to filter through a whole dataset.

This completely breaks that when using API Resources, and looking at the method that causes the issue it appears to be a mixup in implementation.

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 ! empty($data) && is_numeric(array_keys($data)[0])
                    ? array_values($data) : $data;
}

removeMissingValues expects a $numericKeys argument to let the method know if it should coerce the array to be sequential, however it completely ignores that parameter and uses a naive check instead, only checking if the first key in the array passes the is_numeric test. This test is problematic since it will be true for any string that can be properly cast to a number.

That aside, the original test for numeric keys that passes in the $numericKeys argument is much more solid: $numericKeys = array_values($data) === $data;. If the method actually used the result of this test as its parameter list implies then our problem would be solved.

I understand that using strings for keys seems odd, but it is in fact a very useful and widely adopted approach to architecting frontend applications.

Here is an example official example from the vuex repo: https://github.com/vuejs/vuex/blob/master/examples/chat/vuex/store.js

Their chat data is normalized, described as threads and messages which can both be referenced by id. This structure would be impossible to get using API Resources from Laravel with this change. You would have to get nested data from the API then post-process it on the frontend.

[taylorotwell]

I'm still open for this to be fixed if necessary on 5.8. Would require a fresh PR to master branch and I can review it. The underlying code seems to have changed between now and then.