Skip to content

[5.6] Allow to fetch specific key when using json helper - #22489

Merged
taylorotwell merged 2 commits into
laravel:masterfrom
jerguslejko:5.5
Dec 22, 2017
Merged

[5.6] Allow to fetch specific key when using json helper#22489
taylorotwell merged 2 commits into
laravel:masterfrom
jerguslejko:5.5

Conversation

@jerguslejko

@jerguslejko jerguslejko commented Dec 20, 2017

Copy link
Copy Markdown
Contributor

This PR allows to fetch a specific key off of the TestResponse class.

Example:

$title = $this->post('/posts', $data)
    ->json('data.title');

vs

$title = $this->post('/posts', $data)
    ->json()['data']['title'];

🌵

/**
* Validate and return the decoded response JSON.
*
* @param string $key

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be @param string|null $key if I'm not wrong

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh thanks!

/**
* Validate and return the decoded response JSON.
*
* @param string $key

@jkniest jkniest Dec 20, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: This should be:

@param string|null $key 

if I'm not wrong

@mfn

mfn commented Dec 20, 2017

Copy link
Copy Markdown
Contributor

If in the old version, data or title was missing or title, you would get an immediate error (which I consider good).

OTOH with data_get you will always receive a value (default value) back and your test breaks at a later point.

Not sure I would consider that an improvement?

Iff it didn't matter for I test I would probably write ->json()['data']['title'] ?? ''.

@sisve

sisve commented Dec 20, 2017

Copy link
Copy Markdown
Contributor

A quick reminder that even the TestResponse class is a documented public api. Changing a method signature in a public api is a breaking change. Have you considered targeting the 5.6 release?

@jkniest

jkniest commented Dec 20, 2017

Copy link
Copy Markdown

But this change, as I see it, is completley optional. You don't need to pass a key in it. So it would not "break" anything and it would still be useable like before the change.

@jkniest

jkniest commented Dec 20, 2017

Copy link
Copy Markdown

If the $key is null, which it is by default, the normal object will be returned like before.

https://github.com/illuminate/support/blob/master/helpers.php#L454

@sisve

sisve commented Dec 20, 2017

Copy link
Copy Markdown
Contributor

The breaking isn't for those that call the method, but for those that have overridden it. You will get php warnings stating that your model signature does not match the signature that you are overriding.

* Validate and return the decoded response JSON.
*
* @param string|null $key
* @return array

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return type is now wrong

* Validate and return the decoded response JSON.
*
* @param string|null $key
* @return array

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return type is now wrong

@sisve

sisve commented Dec 20, 2017

Copy link
Copy Markdown
Contributor

Could you change the targeted branch to master to target the 5.6 release since this has breaking changes (changes to method signatures)?

@jerguslejko

Copy link
Copy Markdown
Contributor Author

Honestly, I do not consider this PR to be a breaking change. I'd like it to be merged into 5.5 so it becomes available right away.

@devcircus

Copy link
Copy Markdown
Contributor

I hate these back-and-forths on breaks, however, whether something is a BC break does not change if you don't agree. It either is or isn't. Technically, if the class isn't declared final, adding a parameter ( even with a default value) is a BC break. If the method is being overwritten (likely), then that app will break. Period.

Should target master, as we shouldn't intentionally break people's apps unless it is an important bug fix(this is not), but that's definitely up to Taylor.

@taylorotwell
taylorotwell changed the base branch from 5.5 to master December 22, 2017 15:03
@taylorotwell
taylorotwell merged commit bf1427f into laravel:master Dec 22, 2017
@GrahamCampbell GrahamCampbell changed the title [5.5] Allow to fetch specific key when using json helper [5.6] Allow to fetch specific key when using json helper Dec 22, 2017
@jerguslejko
jerguslejko deleted the 5.5 branch December 22, 2017 16:42
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.

7 participants