laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.51k stars 11.02k forks source link

Numeric keys in resources & $preserveKeys = true #30052

Closed Ir00man closed 4 years ago

Ir00man commented 5 years ago

Description:

As I understand that's not a new bug and was discussed a lot and even "was fixed" but still if we have JsonResource and trying to return some Collection (via JsonResource::collection method) keyed by some numeric field - we will get a plain array not an object as a output.

I mean that even if we use public $preserveKeys = true; Btw, I can't find any case where $preserveKeys has an effect, in all my tests $preserveKeys has no effect at all.

How to deal with that? How to have an object {} with keys outputted instead of array []?

27325

Steps To Reproduce:

/app/Http/Resources/Test.php file:

namespace App\Http\Resources;

use Illuminate\Http\Resources\Json\JsonResource;

class Test extends JsonResource
{
    public $preserveKeys = true;
    /**
     * Transform the resource into an array.
     *
     * @param  \Illuminate\Http\Request  $request
     * @return array
     */
    public function toArray($request)
    {
        //dd($this);
        return $this->resource;
    }
}

/routes/web.php file:

use Illuminate\Http\Request;
use Illuminate\Support\Collection;

Route::get('test', function () {
    $data = Collection::make([
        [
            'id' => 1,
            'name' => 'name1',
        ],
        [
            'id' => 2,
            'name' => 'name2',
        ],
    ])->keyBy('id');

    //dd($data->toArray());
    return \App\Http\Resources\Test::collection($data);
});

Now you will get in browser:

[{"id":1,"name":"name1"},{"id":2,"name":"name2"}]

But if you uncomment //dd($data->toArray()); you will see:

array:2 [▼
  1 => array:2 [▼
    "id" => 1
    "name" => "name1"
  ]
  2 => array:2 [▼
    "id" => 2
    "name" => "name2"
  ]
]
staudenmeir commented 5 years ago

You are using withoutWrapping(), right? With a wrapper, $preserveKeys works correctly. Not sure if it should/can also work without one.

Ir00man commented 5 years ago

Oh yeah, I'm using Resource::withoutWrapping(); And I've just tested that it's working good with wrapping (which is default) But I don't see any reason why it shouldn't work when the wrapping is turned off. Any ideas?

staudenmeir commented 5 years ago

It's caused by this line:

https://github.com/laravel/framework/blob/1bbe5528568555d597582fdbec73e31f8a818dbc/src/Illuminate/Http/Resources/Json/ResourceResponse.php#L71

Ir00man commented 5 years ago

Yes, you are right, this code

$w = array_merge_recursive([1 => [1 => 1, 2 => 2]]);
var_dump($w);

outputs this

array(1) {
  [0]=>
  array(2) {
    [1]=>
    int(1)
    [2]=>
    int(2)
  }
}

As for me it's weird and undocumented behavior of array_merge_recursive function...

staudenmeir commented 5 years ago

The behavior is documented for array_merge():

Values in the input arrays with numeric keys will be renumbered with incrementing keys starting from zero in the result array.

Ir00man commented 5 years ago

Sure it is. But look at the second level. All keys are preserved (but they are numeric as well).

Ir00man commented 5 years ago

Does somebody know what's an expected behavior for this input data:

$data = [1 => [1 => 11, 2 => 12]];
$with = [1 => [2, 3]];
$additional = ['some' => 1];

What should be an output with withoutWrapping() applied?

driesvints commented 5 years ago

@staudenmeir so is this a bug or not?

staudenmeir commented 5 years ago

@driesvints I think it is a bug, but I'm not sure if there's a good way to fix it.

Ir00man commented 5 years ago

Seems that nobody except me using API responses without extra wrapping and keyed by primary ID to cut off an extra transformation from client-side. For sure, this transformation is simple on client-side, like:

let response = [{id:1, name:'bob'}, {id:2, name:'alice'}];
let responseKeyed = {};
response.map((v)=>{responseKeyed[v.id] = v});

But as for me it's better to have working $preserveKeys option

jk2 commented 5 years ago

I had a different use case but solved it by overwriting toResponse of JsonResource.

In Test.php:

public function toResponse($request)
{
    return $this->resource;
}

Then it won't call problematic wrap function in ResourceResponse.

Not sure if using array_replace_recursive instead of array_merge_recursive would fix the bug on wrap function.

straube commented 5 years ago

I ran a couple tests over here and it seems array_replace_recusrive not always gives the same result as array_merge_recursive. It does work for some cases but it seems simply changing the function would introduce new bugs.

Ir00man commented 5 years ago

@jk2 Thx for sharing. @straube Yeah, same for me. I just don't know what is expected to be outputted in case I mentioned above https://github.com/laravel/framework/issues/30052#issuecomment-533879458

taylorotwell commented 4 years ago

No plans on changing any behavior here.