Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.7] Add tests for the way JsonResource handles numeric keys #25269

Merged
merged 1 commit into from
Aug 20, 2018
Merged

[5.7] Add tests for the way JsonResource handles numeric keys #25269

merged 1 commit into from
Aug 20, 2018

Conversation

SjorsO
Copy link
Contributor

@SjorsO SjorsO commented Aug 20, 2018

(#25226 retargeted to 5.7)

JsonResource seems to strip all array keys if any of them are numeric or a numeric string. I couldn't find any tests that assert this behavior, so this PR adds some.

Because of this behavior, doing this will give you an array of names, without any ids:

public function index()
{
    $data = User::pluck('name', 'id');

    return JsonResource::make($data);
}

I'm not sure if this is a bug or a feature, but having tests for behavior like this is important.

@staudenmeir
Copy link
Contributor

I and others would call this a bug that should be fixed (#24339, #23595, #24302). At least, the unused removeMissingValues() parameter should be fixed.

@taylorotwell taylorotwell merged commit fdd2ca9 into laravel:5.7 Aug 20, 2018
@taylorotwell
Copy link
Member

It's not a bug.

@deleugpn
Copy link
Contributor

I can see how this would be treated as not-a-bug because the Resource API layer is only interested in giving back a collection of objects or a single object and in either case the key is no longer in the Eloquent's power as it's a whole other software layer. I usually include 'id' => $this->resource->id in my Resource Objects, allowing frontend to pluck the same way we do with Collections. I guess I'm neutral on this one as I can see it working both ways.

@staudenmeir
Copy link
Contributor

staudenmeir commented Aug 20, 2018

I mainly consider it a bug because #23414 unintentionally changed the behavior in a minor release.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants