Small tweaks for php generator, PHPStan level 3, Psalm level 7 #7616

dkarlovi · 2020-10-07T07:58:25Z

Summary

This is a followup to #7376, #7528, trying to get the generated code quality up to a standard where strong static analysis tools like PHPStan, Psalm can be used to verify the generated output in CI.

This reaches level 3 for PHPStan (0 is most lax, 8 is most strict), level 7 for Psalm (8 is most lax, 1 is most strict).

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@jebentier (2017/07), @dkarlovi (2017/07), @mandrean (2017/08), @jfastnacht (2017/09), @ackintosh (2017/09) heart, @ybelenko (2018/07), @renepardon (2018/12)

dkarlovi · 2020-10-07T08:06:57Z

modules/openapi-generator/src/main/resources/php/ObjectSerializer.mustache

@@ -307,6 +314,7 @@ class ObjectSerializer
            }
        }

+        /** @psalm-suppress ParadoxicalCondition */


This one is interesting, Psalm found it's nonsensical since it includes object, which is already handled a few lines above, meaning this branch can never be reached with that specific "class".

This introduces the first tool-specific annotation, but I believe it's not a big issue since Doctrine and PHPUnit already use them natively.

See example what it found here: https://psalm.dev/r/1035490c06

dkarlovi · 2020-10-07T08:09:47Z

modules/openapi-generator/src/main/resources/php/ObjectSerializer.mustache

@@ -159,8 +165,9 @@ class ObjectSerializer
     */
    public static function toHeaderValue($value)
    {
-        if (method_exists($value, 'toHeaderValue')) {
-            return $value->toHeaderValue();
+        $callable = [$value, 'toHeaderValue'];


Using is_callable is safer than using method_exists, while also being more correct.

dkarlovi · 2020-10-07T08:10:53Z

@ybelenko this one might be more easy to digest. 😄

wing328 · 2020-10-10T02:19:20Z

Travis CI reports the following errors:

Time: 1.47 seconds, Memory: 10.00 MB
There were 4 errors:
1) OpenAPI\Client\DateTimeSerializerTest::testDateTimeSanitazion
Undefined property: stdClass::$dateTime
/home/travis/build/OpenAPITools/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/tests/DateTimeSerializerTest.php:20
2) OpenAPI\Client\DateTimeSerializerTest::testDateSanitazion
Undefined property: stdClass::$date
/home/travis/build/OpenAPITools/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/tests/DateTimeSerializerTest.php:38
3) OpenAPI\Client\PetApiTest::testGetPetById
Undefined offset: 0
/home/travis/build/OpenAPITools/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/tests/PetApiTest.php:65
4) OpenAPI\Client\PetApiTest::testGetPetByIdWithHttpInfo
Error: Call to a member function getId() on null
/home/travis/build/OpenAPITools/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/tests/PetApiTest.php:110

Please take a look when you've time.

For the CircleCI failure, it has been resolved in the master so please merge the latest master into this branch.

dkarlovi · 2020-10-10T08:16:33Z

@wing328 oh, great news we can rely on tests again. Will take a look ASAP.

dkarlovi · 2020-10-20T15:53:03Z

The issue was fixed, I'm still getting

POST http://petstore.swagger.io/v2/pet` resulted in a `405 Method Not Allowed

but I guess this is expected since it's failing consistently to master.

wing328 · 2020-10-20T15:57:55Z

but I guess this is expected since it's failing consistently to master.

Just checked the readme and all CI tests passed. (the CI tests against a local Petstore server instead of using the public one)

dkarlovi · 2020-10-20T19:18:30Z

@wing328 OK, do we merge here?

ybelenko · 2020-10-20T19:26:30Z

samples/client/petstore/php/OpenAPIClient-php/lib/ObjectSerializer.php

-                        && !in_array($value, $openAPIType::getAllowableEnumValues(), true)) {
-                        $imploded = implode("', '", $openAPIType::getAllowableEnumValues());
-                        throw new \InvalidArgumentException("Invalid value for enum '$openAPIType', must be one of: '$imploded'");
+                    if ($value !== null && !in_array($openAPIType, ['DateTime', 'bool', 'boolean', 'byte', 'double', 'float', 'int', 'integer', 'mixed', 'number', 'object', 'string', 'void'], true)) {


@dkarlovi So... instead of one big condition you decided to use three nested conditions? It's weird that PHPStan doesn't complain about that. It usually says to me "consider refactoring" when I have the third nested condition. 😕

@ybelenko the nested ifs are equivalent to the original single if, the difference is the safer types, in this case, $callable is really callable, using it instead of method_exists, see my other comment with links.

IMO the serializer should in the future likely be extracted from the package anyway since it's static and has tests which are outside of the generated package (hinting at its static nature). Refactoring it further until it's clean enough to split apart / replace is a good direction here.

dkarlovi · 2020-10-21T07:33:52Z

samples/client/petstore/php/OpenAPIClient-php/lib/ObjectSerializer.php

+                    if ($value !== null && !in_array($openAPIType, ['DateTime', 'bool', 'boolean', 'byte', 'double', 'float', 'int', 'integer', 'mixed', 'number', 'object', 'string', 'void'], true)) {
+                        $callable = [$openAPIType, 'getAllowableEnumValues'];
+                        if (is_callable($callable)) {
+                            /** array $callable */


Oh no, copy paste. 🙈

Should be

/** array $allowedEnumTypes */

Oh no, copy paste. 🙈

Should be

/** array $allowedEnumTypes */

Could you fix it in next PR?

ybelenko

Didn't check locally, lgtm.

dkarlovi · 2020-10-21T07:40:14Z

OK, great, I have some tweaks for the Markdown files next, used markdownlint to reorganize them a bit.

dkarlovi commented Oct 7, 2020

View reviewed changes

wing328 added Client: PHP Enhancement: Code format labels Oct 10, 2020

wing328 added this to the 5.0.0 milestone Oct 10, 2020

Small tweaks for php generator, PHPStan level 3

48ed5c8

ybelenko reviewed Oct 20, 2020

View reviewed changes

dkarlovi commented Oct 21, 2020

View reviewed changes

ybelenko approved these changes Oct 21, 2020

View reviewed changes

ybelenko merged commit 240c046 into OpenAPITools:master Oct 21, 2020

dkarlovi deleted the fix/psalm-level-3 branch October 21, 2020 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small tweaks for php generator, PHPStan level 3, Psalm level 7 #7616

Small tweaks for php generator, PHPStan level 3, Psalm level 7 #7616

dkarlovi commented Oct 7, 2020

dkarlovi Oct 7, 2020

dkarlovi Oct 7, 2020

dkarlovi Oct 7, 2020

dkarlovi commented Oct 7, 2020

wing328 commented Oct 10, 2020

dkarlovi commented Oct 10, 2020

dkarlovi commented Oct 20, 2020

wing328 commented Oct 20, 2020

dkarlovi commented Oct 20, 2020

ybelenko Oct 20, 2020

dkarlovi Oct 21, 2020 •

edited

Loading

dkarlovi Oct 21, 2020

ybelenko Oct 22, 2020 •

edited

Loading

ybelenko left a comment

dkarlovi commented Oct 21, 2020

Small tweaks for php generator, PHPStan level 3, Psalm level 7 #7616

Small tweaks for php generator, PHPStan level 3, Psalm level 7 #7616

Conversation

dkarlovi commented Oct 7, 2020

Summary

PR checklist

dkarlovi Oct 7, 2020

Choose a reason for hiding this comment

dkarlovi Oct 7, 2020

Choose a reason for hiding this comment

dkarlovi Oct 7, 2020

Choose a reason for hiding this comment

dkarlovi commented Oct 7, 2020

wing328 commented Oct 10, 2020

dkarlovi commented Oct 10, 2020

dkarlovi commented Oct 20, 2020

wing328 commented Oct 20, 2020

dkarlovi commented Oct 20, 2020

ybelenko Oct 20, 2020

Choose a reason for hiding this comment

dkarlovi Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

dkarlovi Oct 21, 2020

Choose a reason for hiding this comment

ybelenko Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

ybelenko left a comment

Choose a reason for hiding this comment

dkarlovi commented Oct 21, 2020

dkarlovi Oct 21, 2020 •

edited

Loading

ybelenko Oct 22, 2020 •

edited

Loading