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.6] Allow to fetch specific key when using json helper #22489

Merged
merged 2 commits into from
Dec 22, 2017
Merged

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

merged 2 commits into from
Dec 22, 2017

Conversation

jerguslejko
Copy link
Contributor

@jerguslejko jerguslejko commented Dec 20, 2017

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'];

🌵

@@ -500,9 +500,10 @@ public function assertJsonValidationErrors($keys)
/**
* Validate and return the decoded response JSON.
*
* @param string $key
Copy link

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
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
Copy link

@jkniest jkniest Dec 20, 2017

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
Copy link
Contributor

mfn commented Dec 20, 2017

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
Copy link
Contributor

sisve commented Dec 20, 2017

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
Copy link

jkniest commented Dec 20, 2017

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
Copy link

jkniest commented Dec 20, 2017

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
Copy link
Contributor

sisve commented Dec 20, 2017

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
Member

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

@@ -500,9 +500,10 @@ public function assertJsonValidationErrors($keys)
/**
* Validate and return the decoded response JSON.
*
* @param string|null $key
* @return array
Copy link
Member

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
Copy link
Contributor

sisve commented Dec 20, 2017

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
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
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