Added check for json_decode result

[PlanetTheCloud]

Fix #39

Checked json_decode result if it's null, then return a null.

On line 63, I've added throw new \RuntimeException('Failed to decode JSON data: ' . $this->getData()); which can be uncommented if you prefer Exception to be thrown instead of returning null.

[Grendel7]

Thank you for your contribution!

I agree with your judgement that it's better to return null than to throw an exception. That said, with the current code, it's not possible to distinguish between the API returning null (which is normal) and the API returning invalid JSON (which is not normal).

Also, could you please:

• Make sure that your code follows the PSR-2 coding style guide?
• Add a test for this behavior?

[PlanetTheCloud]

Hi,

I've added a new commit that complies with PSR-2 coding style guide.
I've also added a test case but since I've never worked with PHPUnit, I can't be sure 100% that my implementation was correct. Please send me a feedback about the test I've implemented.

About the distinguishing between the API returning invalid JSON vs null, do you agree if the API returned an invalid JSON, an Exception shall be thrown?

[Grendel7]

I've added a new commit that complies with PSR-2 coding style guide.

Thank you, it's looking good now!

I've also added a test case but since I've never worked with PHPUnit, I can't be sure 100% that my implementation was correct. Please send me a feedback about the test I've implemented.

The test you've added looks good!

About the distinguishing between the API returning invalid JSON vs null, do you agree if the API returned an invalid JSON, an Exception shall be thrown?

If anything, it should not silently ignore the error. But to be honest, I've never seen this happen.

[PlanetTheCloud]

I have a suggestion but this may need a lot of work to do but what if we created an error handler function where it will be called when something went wrong and developers can handle the error accordingly, just like Laravel's handler.

I imagined it has a function like this:

public static function handle($e){

	// ... Code

	throw $e;
}

And on our source we can do something like this:

public function getServerResponse(){
	$serverResponse = $this->request->getBody();
	if($serverResponse === 'null'){
		Client::handle(new Vendor\Exception\ServerNullResponse);
	}
	return $serverResponse;
}

Then the developers that uses this can do:

public static function handle($e){

    if ($e instanceof ServerNullResponse){
        // ... Do some stuff
        return;
    }

    throw $e;
}

This is a bit flexible since the dev's can handle the errors easily and we can list the possible errors on the GitHub README.

What do you think about this?

ALTERNATIVELY we could just return false if server responded with broken JSON... if that happens somehow...

[PlanetTheCloud]

Hi, should we just return false if the server responded with broken JSON?

[Grendel7]

I really don't like the error handler solution for a few reasons.

For starters, with any other case where the API does not respond with the expected data type (e.g. most of the XML endpoints), you just get an XxxResponse object back with isSuccessful() returning false and getMessage() responding with the full body. For the error scenarios I've seen, this setup works well. Any other data parsing methods normally respond with some falsey zero value in the normally expected data type.

I don't think it's necessary to have checks to be able to gracefully handle every conceivable kind of issue, because it just makes the code unnecessarily complicated, hard to read and slow to execute.

On really spectacular failures, you'd typically see either the PHP code crash and go through the regular crash recovery system or a Guzzle exception bubbling up. Which is the "right" way to handle errors: the PHP error handler, or throwing exceptions.

Having the error handler exactly as you proposed them means that:

• People need to have static functions, which are not just a code smell but also mean that..
• ... they have to edit the library code to use it.
• ... they don't get access to their call stack to figure out which API call they were doing or why, meaning it's very hard to turn this into a graceful response for the end user.

Passing a callable to the client to handle error would be a slightly nicer way to handle it. But even then, you're just reproducing the behavior of PHP's set_error_handler, but now it has to be implemented for every third party library you use individually, rather than once for the entire application. It also increases the barrier to learn and use the library, which is not the point of a library. Remember that a good framework helps you to do things right (e.g. Laravel), but a library should be as generic and flexible as possible.

To summarize, if you need to deal with some catastrophic failure, exceptions are the way to go. But with the current implementation, in case there is invalid JSON, isSuccesful() returns false ✔️, getMessage() returns the response body ✔️ and getStatus() returns a falsey default ✔️.

[PlanetTheCloud]

Alright then, shall we merge?