json encode (and decode) failing silently

[linaori]

I was facing an issue where I encountered "HTTP/1.1 500 Internal Server Error" errors on the PostNL side. Turns out that due to wrong character encoding, json_encode was failing silently, causing an empty body to be sent.

I did a quick check and JSON_THROW_ON_ERROR isn't used so far. I don't think there should be any internal error handling for json errors, I think the handling of this is the responsibility of the Application using the library. My previous issue #58 was in fact caused by this as well.

There seems to be no explicit error handling for status 500. The response object (when Unable to generate label has:

reasonPhrase = 'Internal Server Error'
statusCode => 500
$response->getBody()->getContents() => ''

The logic in the LabelingService:

if (200 === $response->getStatusCode()) {
    throw new ResponseException('Invalid API response', null, null, $response);
}

throw new NotFoundException('Unable to generate label');

Removing the status code check and then just always throwing this would be sufficient if there's no specific edge cases you want to cover here

throw new ResponseException('Invalid API response', null, null, $response);

I can make PR with these changes if you want

[firstred]

Thanks for raising this issue! This was unfortunately not possible with the v1.x.x range of this library, because it had to support older versions of PHP.
I'll try to make the suggested changes in v2.0.0.

[firstred]

Hmmz, reading this issue again, I just realized my earlier reply makes no sense. Apologies for that!
There's absolutely no reason not to make this change in v1.x.x. It's not a matter of compatibility, except for the JSON_THROW_ON_ERROR flag, since not all PHP versions suported by v1.x.x have this flag.

Version 2.0.0 currently ignores the status code as well, and that works fine in production:

postnl-api-php/src/Service/ResponseProcessor/Rest/LabellingServiceRestResponseProcessor.php

Lines 67 to 77 in 30034d9

	public function processGenerateLabelResponse(ResponseInterface $response): GenerateLabelResponse
	{
	$this->validateResponse(response: $response);
	$responseContent = static::getResponseText(response: $response);
	$body = json_decode(json: $responseContent);
	if (isset($body->ResponseShipments)) {
	return GenerateLabelResponse::jsonDeserialize(json: (object) ['GenerateLabelResponse' => $body]);
	}
	throw new ApiException(message: '`GenerateLabelResponse` does not contain `ResponseShipments`');
	}

postnl-api-php/src/Service/ResponseProcessor/Rest/AbstractRestResponseProcessor.php

Lines 55 to 136 in 30034d9

	protected function validateResponse(ResponseInterface $response): bool
	{
	try {
	$body = json_decode(json: (string) $response->getBody(), flags: JSON_THROW_ON_ERROR);
	} catch (JsonException $e) {
	throw new ResponseException(message: 'Invalid response from server', previous: $e, response: $response);
	}
	if (!empty($body->fault->faultstring) && 'Invalid ApiKey' === $body->fault->faultstring) {
	throw new InvalidConfigurationException();
	}
	if (isset($body->Envelope->Body->Fault->Reason->Text)) {
	$vars = get_object_vars(object: $body->Envelope->Body->Fault->Reason->Text);
	throw new CifDownException(message: $vars[''] ?? 'Unknown');
	}
	if (!empty($body->Errors->Error)) {
	$exceptionData = [];
	foreach ($body->Errors->Error as $error) {
	if (isset($error->ErrorMsg)) {
	$exceptionData[] = [
	'description' => $error->ErrorMsg ?? '',
	'message' => $error->ErrorMsg ?? '',
	'code' => isset($error->ErrorNumber) ? (int) $error->ErrorNumber : 0,
	];
	} else {
	$exceptionData[] = [
	'description' => isset($error->Description) ? (string) $error->Description : '',
	'message' => null,
	'code' => isset($error->ErrorNumber) ? (int) $error->ErrorNumber : 0,
	];
	}
	}
	throw new CifException(message: $exceptionData);
	} elseif (!empty($body->Errors)) {
	$exceptionData = [];
	foreach ($body->Errors as $error) {
	if (isset($error->ErrorMsg)) {
	$exceptionData[] = [
	'description' => $error->ErrorMsg ?? '',
	'message' => $error->ErrorMsg ?? '',
	'code' => isset($error->ErrorNumber) ? (int) $error->ErrorNumber : 0,
	];
	} else {
	$exceptionData[] = [
	'description' => isset($error->Description) ? (string) $error->Description : '',
	'message' => isset($error->Error) ? (string) $error->Error : '',
	'code' => isset($error->Code) ? (int) $error->Code : 0,
	];
	}
	}
	throw new CifException(message: $exceptionData);
	} elseif (!empty($body->Array->Item->ErrorMsg)) {
	// {"Array":{"Item":{"ErrorMsg":"Unknown option GetDeliveryDate.Options='DayTime' specified","ErrorNumber":26}}}
	$exceptionData = [
	[
	'description' => isset($body->Array->Item->ErrorMsg) ? (string) $body->Array->Item->ErrorMsg : '',
	'message' => isset($body->Array->Item->ErrorMsg) ? (string) $body->Array->Item->ErrorMsg : '',
	'code' => 0,
	],
	];
	throw new CifException(message: $exceptionData);
	} elseif (isset($body->ResponseShipments)
	&& is_array(value: $body->ResponseShipments)
	&& isset($body->ResponseShipments[0]->Errors)
	&& is_array(value: $body->ResponseShipments[0]->Errors)
	&& !empty($body->ResponseShipments[0]->Errors)
	) {
	$error = $body->ResponseShipments[0]->Errors[0];
	$exceptionData = [
	[
	'message' => isset($error->message) ? (string) $error->message : '',
	'description' => isset($error->description) ? (string) $error->description : '',
	'code' => isset($error->code) ? (int) $error->code : 0,
	],
	];
	throw new CifException(message: $exceptionData);
	}
	return true;
	}

I don't think I have documented it properly, but these were supposed to be the meaning of every exception:

• ResponseException: Something wrong with the server response
• ApiException: Something wrong with the way the API returns an object (likely an unsupported feature or structure)
• CifDownException: CIF API broken
• CifException: Exception thrown by the CIF API
• NotFoundException: requested object cannot be found (!= cannot be generated)

The NotFoundException is really out of place here, tbh. I'll try to make your suggested changes in 1.4.0. Thanks again!

[firstred]

JSON_THROW_ON_ERROR flag is set in 2.0.0. It will be the previous throwable of the ReponseException.

[firstred]

A polyfill has been used to make this work from 1.4.0 on as well.