Make content type check less strict

[judgej]

The payload response from Starling will be parsed only if the Content-Type header is exactly application/json see:

https://gi:hub.com/consilience/starling-payments-objects/blob/master/src/HydratableTrait.php#L261

Today the content type gained a trailing semi-colon application/json; and all parsing failed. So this header value needs to be split at the first semi-colon and everything following it should be discarded or ignored. Bear in mind there may or may not be a semi-colon, and there may or may not be anything following that semi-colon.

Starling are fixing this to restore the old content type value, but this package needs to be prepared for its return, possibly with a charset name following it.

[karfrank]

I was curious how you fixed this bug and found a few things that don't make sense to me. This might be due to my extremely limited PHP knowledge, so apologies if I got this wrong.

If I understand correctly, your updated code does the following:

1. Get everything before the first ; from the Content-Type header
2. Check whether it is application/json
3. If yes, try to parse response body and write the result to the return value
4. If not, create an empty return value
5. Get the http status code
6. If the http status indicates an unsuccessful request, log an error
7. Return the return value

The problem we saw in #562 was that the application tried to write the empty return from this function into the database.

The bugfix enables us to handle response headers with a charset, but any response with a successful http status and a non application/json Content-Type, e.g. text/html, will be discarded and return an empty result without error because the if ($statusClass !== 2) isn't true for those responses. That's at least my interpretation of the code and confirmed by what we saw in production.

Wouldn't it be better to process the response in the following order:

1. Get the http status code
2. If the http status indicates an unsuccessful request, log/return an error
3. If the http status indicates a successful request
4. Check if the Content-Type header contains application/json somewhere
5. If not, log/return an error
6. If yes, try to parse the body
7. If unsuccessful, log/return an error
8. If successful, return the result

I think another issue is the missing validation of the parsed json. What if it is valid json but doesn't contain the expected data? For example {}. I think the application should do more to validate the data and ensure that it contains all required fields, that those fields are populated and that the values are of the correct length before it tries to write it to the database.

[judgej]

Yes, all good refactoring points that we can certainly tackle.

In the wider context, this package only deals with the content and structure of the requests and responses. The HTTP client is outside of this, so any non-2XX responses should (or could) already be known and the application should be prepared to handle it appropriately.

Returning the empty objects could probably, in hindsight, be better served by throwning an exception. If this package is being asked to squeeze a HTTP response into an object, and it can't, then that is arguably an exception.