[PHP] Fix deserialize ApiException as a Model

[ymilin]

PR checklist

• Read the contribution guidelines.
• Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
• Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
• Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

When defining default responses (ApiException) as a Schema Object in Swagger declaration files, the current implementation cannot deserialize the Model properly into ApiException::$responseObject.

Added a check for $returnType and json_decode appropriately (same logic as the try part of same function for success responses)

fix #7544

[ymilin]

@jebentier @dkarlovi @mandrean @jfastnacht @ackintosh

[ackintosh]

Thanks for the PR! I'll check it later. 😉

[ackintosh]

Each case section has own response type like below. ( \Swagger\Client\Model\Pet, \Swagger\Client\Model\ErrorModel in example)

        } catch (ApiException $e) {
            switch ($e->getCode()) {
                case 200:
                    $data = ObjectSerializer::deserialize(
                        $e->getResponseBody(),
                        '\Swagger\Client\Model\Pet', // <---
                        $e->getResponseHeaders()
                    );
                    $e->setResponseObject($data);
                    break;
                default:
                    $data = ObjectSerializer::deserialize(
                        $content,
                        '\Swagger\Client\Model\ErrorModel', // <---
                        $e->getResponseHeaders()
                    );
                    $e->setResponseObject($data);
                    break;
            }

So I think it is needed to put in each case section like below. 💡

        } catch (ApiException $e) {
            switch ($e->getCode()) {
                case 200:
+                  $content = $e->getResponseBody();
+                  if ('\Swagger\Client\Model\Pet' !== 'string') {
+                      $content = json_decode($content);
+                  }
                    $data = ObjectSerializer::deserialize(
                        $content,
                        '\Swagger\Client\Model\Pet', // <---
                        $e->getResponseHeaders()
                    );
                    $e->setResponseObject($data);
                    break;
                default:
+                  $content = $e->getResponseBody();
+                  if ('\Swagger\Client\Model\ErrorModel' !== 'string') {
+                      $content = json_decode($content);
+                  }
                    $data = ObjectSerializer::deserialize(
                        $content,
                        '\Swagger\Client\Model\ErrorModel', // <---
                        $e->getResponseHeaders()
                    );
                    $e->setResponseObject($data);
                    break;
            }

[ymilin]

Oh, I see... Thank you very much for the review :-)

I'll be able to try and submit a new patch on Monday I think.

Also, my apologies I forgot to move the commits to a git branch before submitting, let me know if you want me to change that (would I need to submit a new PR?)

[ackintosh]

No need to submit a new PR. 👍 Please add a new patch to this PR.

[ackintosh]

Thanks for adding the patch. It looks good to me!

[wing328]

@ymilin what about other primitive types such as integer, boolean, etc? Do we need to check those as well?

[ymilin]

@wing328 Thanks for the review 😄

I went ahead and took a better look at the ObjectSerializer::deserialize function. This PR seems to effectively break other primitive type support (especially with booleans as they get decoded as (int) 1 and null for true and false respectively).

Now it feels to me that the json_decode part should be handled in deserialize as the function has knowledge of both the data and its type.

The same approach is already used with "success" responses and is quite concerning for the same reasons:

https://github.com/swagger-api/swagger-codegen/blob/432b3589861d4a35e2d4a7a1f6a55b889645886f/modules/swagger-codegen/src/main/resources/php/api.mustache#L159-167

I'll spend some time and test with all the types from the OpenAPI spec and validate all this.

[vladimir-shebastyuk]

@wing328 I was trying to resolve the same problem and found out that the approach that was used here is correct, because php json_dencode function properly handle raw types, e.g.:

echo "json_decode('123')\n";
var_dump(json_decode('123'));

echo "json_decode('123.45')\n";
var_dump(json_decode('123.45'));

echo "json_decode('true')\n";
var_dump(json_decode('true'));

echo "json_decode('false')\n";
var_dump(json_decode('false'));

will return:

json_decode('123')
int(123)
json_decode('123.45')
double(123.45)
json_decode('true')
bool(true)
json_decode('false')
bool(false)

So each primitive type handled by json_decode properly (I think the same idea was behind handling success response with the same check only for string primitive type). I also generate client and test it with real API and got correct answers.

I believe it's safe to apply theses changes:

$content = $e->getResponseBody();
if ('{{dataType}}' !== 'string') {
	$content = json_decode($content);
}
$data = ObjectSerializer::deserialize(
	$content,
	'{{dataType}}',
	$e->getResponseHeaders()
);

Is it possible to accept this PR or it is necessary to do some extra actions? E.g. update code or create a new PR?

[ymilin]

Hi, this PR was ported and merged to the OpenAPITools/openapi-generator fork of this project.

Link to the PR over there : OpenAPITools/openapi-generator#757