oauth2-client: `AbstractProvider::getParsedResponse` should always return array or throw exception

My main concern still remains: The UnexpectedValueException provides no way to see the contents of the response for debugging purposes. All you can see is that a request failed, and any content the provider might have returned in a non-JSON format (e.g. an HTML error page) is swallowed by the library. I’d like to see this addressed, personally.

As this issue is still open, I’d like to raise some concern and share my thought. This is how it is currently implemented

public function getParsedResponse(RequestInterface $request)
{
    try {
        $response = $this->getResponse($request);
    } catch (BadResponseException $e) {
        $response = $e->getResponse();
    }

    $parsed = $this->parseResponse($response);

    $this->checkResponse($response, $parsed);

    return $parsed;
}

I assume the intended task of parseResponse() is to correctly parse the response body and return it. Currently, parseResponse is able to parse urlencoded or valid json body. In general, the result of a json parsing does not need to be an array. It could even be null, empty string, integer, etc. So the value of $parsed can be of different type, which I don’t really care much. It is up to the calling function to correctly handle that value.

/**
 * Parses the response according to its content-type header.
 *
 * @throws UnexpectedValueException
 * @param  ResponseInterface $response
 * @return array
 */
protected function parseResponse(ResponseInterface $response)
{
    $content = (string) $response->getBody();
    $type = $this->getContentType($response);

    if (strpos($type, 'urlencoded') !== false) {
        parse_str($content, $parsed);
        return $parsed;
    }

    // Attempt to parse the string as JSON regardless of content type,
    // since some providers use non-standard content types. Only throw an
    // exception if the JSON could not be parsed when it was expected to.
    try {
        return $this->parseJson($content);
    } catch (UnexpectedValueException $e) {
        if (strpos($type, 'json') !== false) {
            throw $e;
        }

        if ($response->getStatusCode() == 500) {
            throw new UnexpectedValueException(
                'An OAuth server error was encountered that did not contain a JSON body',
                0,
                $e
            );
        }

        return $content;
    }
}

The @return array type hint is surely wrong.

The problem with current implementation of parseResponse() is that it throws an exception which does not help much in the calling function if the response body cannot be parsed as JSON. This includes body having empty string as value. Thus, I’d suggest to throw a new exception type called ResponseParsingException which encapsulates the response and its body, so that the calling function can reacts accordingly by retrieving the response and/or the body from the catched exception. To ensure BC we can define a flag in AbstractProvider, e.g.

protected $mayThrowResponseParsingException = false;

which can be used to control the behavior of parseResponse().

 /**
 * Parses the response according to its content-type header.
 *
 * @throws UnexpectedValueException
 * @throws ResponseParsingException if the flag $mayThrowResponseParsingException is true and
 *                                  response body cannot be parsed.
 * @param  ResponseInterface $response
 * @return array|string
 */
protected function parseResponse(ResponseInterface $response)
{
    $content = (string) $response->getBody();
    $type = $this->getContentType($response);

    if (strpos($type, 'urlencoded') !== false) {
        parse_str($content, $parsed);
        return $parsed;
    }

    // Attempt to parse the string as JSON regardless of content type,
    // since some providers use non-standard content types. Only throw an
    // exception if the JSON could not be parsed when it was expected to.
    try {
        return $this->parseJson($content);
    } catch (UnexpectedValueException $e) {
        if (strpos($type, 'json') !== false) {
            throw $this->mayThrowResponseParsingException
                ? new ResponseParsingException($response, $content, $e->getMessage(), $e->getCode())
                : $e;
        }

        // for any other content types
        if ($this->mayThrowResponseParsingException) {
            // let the calling function decide what to do with the response and its body
            throw new ResponseParsingException($response, $content, '', 0);
        } else {
            if ($response->getStatusCode() == 500) {
                throw new UnexpectedValueException(
                    'An OAuth server error was encountered that did not contain a JSON body',
                    0,
                    $e
                );
            }

            return $content;
        }
    }
}

` What do you think? I know that the method checkResponse() can be (mis)used to re-parse the response later. But this is most probably not what this method is supposed to do. I assume that checkResponse should be used to check for error messages in the successfully parsed body returned by a well-behaved identity provider.

Same here, too. Version 2.3.0 on PHP 7.2. The OAuth Server on the other end sends a error string, which goes into prepareAccessTokenResponse without hesitation, which in turn just fails with the exception the thread owner stated in his very first sentence.

I’m open to additional PR(s) to improve the behavior.