-
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.4] Use composition to create TestResponses instead of inheritance #18089
[5.4] Use composition to create TestResponses instead of inheritance #18089
Conversation
Fixes #18049 by allowing a TestResponse to be created from any type of response, instead of being coupled to responses that support `getContent()`.
@adamwathan is this change likely to be the cause of my tests now failing? https://twitter.com/stephendball/status/837055367565701120 I am using https://github.com/robclancy/presenter so the controllers return \Robbo\Presenter\View\View which extend View |
Hey @REBELinBLUE I hope not! Is there any chance you could push up some sort of public repo that reproduces your issue? Happy to take a look and see if I can help track down the problem. |
No worries, didn't think it would be from looking at the diff of the changes. It was just the most obvious thing from the change log so I thought I'd maybe misunderstood it. I'll just continue to play around. Thanks |
I'm thinking it may be related because |
https://github.com/REBELinBLUE/laravel-bug yeah exactly the same on a base install just adding assertViewHas to the example test. I'll start a fresh issue |
Thanks for that @REBELinBLUE ! PR is up: |
Thanks 😄
…-----------------
Stephen Ball
[email protected]
On 1 Mar 2017, 23:25 +0000, Adam Wathan ***@***.***>, wrote:
Thanks for that @REBELinBLUE (https://github.com/REBELinBLUE) ! PR is up:
#18182 (#18182)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub (#18089 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/ACC2pM3Ex8wHi9HbafbngLhbZJYMNRSSks5rhf5hgaJpZM4MKXA3).
|
Hi, Just to give you a use case for which this update breaks the tests: when using third party packages such as Swagger Assertions, which type-hints response objects and expects them to either extend Symfony's response object or at least implement PSR-7's response interface. Cheers |
Fixes #18049 by allowing a TestResponse to be created from any type of response, instead of being coupled to responses that support
getContent()
.The real underlying cause for that issue is that Symfony's
BinaryFileResponse
extends their base response, even though it's not actually substitutable. Inheritance is stupid 👍🏻This does introduce one potential problem (although it seems unlikely...)
Since TestResponse no longer extends Response, it won't pass any
Response
type hints. I find it extremely unlikely that this will affect anyone, because it seems very odd to get the$response
variable back as a result in a test, and then pass it into something that's type hinting the base response, but this change would break that code if it exists anywhere.I've only implemented
__get
and__call
here because again it seems odd that anyone would try to mutate a property the returned response, but can add that if there's some reasonable justification for it.I've left the
$baseResponse
property public, so it would be trivial for end users to grab that directly and manipulate it themselves if needed for whatever reason.