Why is the default response of a Mock client an empty 200 response?
ruudk opened this issue · 8 comments
I think this leads to very weird situations where you register a Mock client in your test environment. Then suddenly, pages are breaking because "null cannot be foreached". Looking at the code I see that the client responded with 200 OK but the response is null. Only then realising that the client is a Mock client.
Wouldn't it make more sense if the Client would always return an 501 Not Implemented or throws a NoMockSpecifiedException?
If you have not run $mockClient->setDefaultResponse(new Response(500)); then your default response will be a 200 with empty headers and body. (See https://github.com/php-http/mock-client/blob/master/src/Client.php#L87)
But $mockClient->sendRequest($request), should never return null. Im not sure why you get a "null cannot be foreached".
I got null cannot be foreached because my code did a check on status code 200, then proceed with json decode the body, then assume it's an array.
Then I found out it was a mock that didn't catch anything and wondered what the reasoning behind this was?
i think i would always check if json decode worked, even if you have status 200. the server might be somehow bugged. or, use something like https://github.com/thePanz/json-exception to have an exception when parsing failed to not have to bother with checking return values of the method.
as for the mock: depending on what you do, 200 with empty body might be fine. returning some sort of error status or throwing an exception would be a BC break for people who relied on it. what would you expect the mock client to do?
It is a BC break, but I find it weird behavior. Can't we solve this in a new major? Default should throw "no mock specified".
Good idea about the json decode verification
i created the 2.0 milestone and added it. can you formulate the argument why it should throw an exception instead of "silently work"? imho it depends on the use case if "default ok" is good or is hiding a problem.
My argument is: When will an empty 200 body be something that is expected from an API? IMHO it just masks the problem. You have no idea that some services are calling endpoints and don't verify the response correctly. It's the same as with PHPUnit Mock's versus Mockery Mock's. With PHPUnit's Mocks it will just return null on any existing method you call but haven't specified. While Mockery tells you "you didn't supply a mock for this method, so I don't know what to do". This hints you to the fact that you forgot something.
i agree with that. do you want to create a pull request for version 2 to change this behaviour? i think we don't have a ton of things coming up that would need a new major version. then again, a major version for this library is not a problem so lets just do it?
I will see if I can find some time. Added to my todo list.