-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
Conversation
@@ -500,9 +500,10 @@ public function assertJsonValidationErrors($keys) | |||
/** | |||
* Validate and return the decoded response JSON. | |||
* | |||
* @param string $key |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
If in the old version, OTOH with Not sure I would consider that an improvement? Iff it didn't matter for I test I would probably write |
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? |
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. |
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 |
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
Could you change the targeted branch to master to target the 5.6 release since this has breaking changes (changes to method signatures)? |
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. |
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. |
This PR allows to fetch a specific key off of the
TestResponse
class.Example:
vs
🌵