-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tweaks for php generator, based on issues reported by PHP static analysis tools (PHPStan) #7376
Conversation
@dkarlovi Can you attach Psalm report from you build(any php generator) right to that conversion? Just as a visual example of what should be fixed in next PRs. |
@@ -423,7 +426,7 @@ class Configuration | |||
/** | |||
* Returns an array of host settings | |||
* | |||
* @return an array of host settings | |||
* @return array an array of host settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type completely missing here.
* @return URL based on host settings | ||
* @param int $index index of the host settings | ||
* @param array|null $variables hash of variable and the corresponding value (optional) | ||
* @return string URL based on host settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type completely missing here.
@@ -51,20 +51,26 @@ class ObjectSerializer | |||
* @param string $type the OpenAPIToolsType of the data | |||
* @param string $format the format of the OpenAPITools type of the data | |||
* | |||
* @return string|object serialized form of $data | |||
* @return scalar|object|array|null serialized form of $data | |||
*/ | |||
public static function sanitizeForSerialization($data, $type = null, $format = null) | |||
{ | |||
if (is_scalar($data) || null === $data) { | |||
return $data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code simplified since it's returning, no need for elseif
.
*/ | ||
public function setHostIndex($host_index) | ||
public function setHostIndex($hostIndex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CS change
} | ||
|
||
/** | ||
* Get the host index | ||
* | ||
* @return Host index | ||
* @return int Host index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing type.
@@ -16,7 +16,7 @@ | |||
* Please update the test case below to test the endpoint. | |||
*/ | |||
|
|||
namespace {{invokerPackage}}; | |||
namespace {{invokerPackage}}\Test\Api; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests were placed in totally wrong namespace.
@@ -36,6 +36,6 @@ | |||
"psr-4": { "{{escapedInvokerPackage}}\\" : "{{srcBasePath}}/" } | |||
}, | |||
"autoload-dev": { | |||
"psr-4": { "{{escapedInvokerPackage}}\\" : "{{testBasePath}}/" } | |||
"psr-4": { "{{escapedInvokerPackage}}\\Test\\" : "{{testBasePath}}/" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests placed in the wrong namespace.
@@ -38,6 +38,11 @@ use \{{invokerPackage}}\ObjectSerializer; | |||
* @package {{invokerPackage}} | |||
* @author OpenAPI Generator team | |||
* @link https://openapi-generator.tech | |||
{{^isEnum}} | |||
* @implements \ArrayAccess<TKey, TValue> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generics.
|
@dkarlovi Cannot review until Tuesday Sep 22th, sorry. |
No rush, was just interested if there is something I should do to get it in since I have some time now. Whenever you have time is great, thanks! |
Can we fix it somehow? $ vendor/bin/phpunit tests
PHPUnit 9.3.11 by Sebastian Bergmann and contributors.
Warning: Your XML configuration validates against a deprecated schema.
Suggestion: Migrate your XML configuration using "--migrate-configuration"!
EEEEE.........ES.................................ESSSSSSSSSSSSSS. 65 / 73 ( 89%)
......E. 73 / 73 (100%)
Time: 00:10.930, Memory: 12.00 MB
There were 8 errors:
1) OpenAPI\Client\AsyncTest::testAsyncRequest
OpenAPI\Client\ApiException: [405] Client error: `POST http://petstore.swagger.io/v2/pet` resulted in a `405 Method Not Allowed` response:
<?xml version="1.0" encoding="UTF-8" standalone="yes"?><apiResponse><type>unknown</type></apiResponse>
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php:162
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php:135
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/tests/AsyncTest.php:40
2) OpenAPI\Client\AsyncTest::testAsyncRequestWithHttpInfo
OpenAPI\Client\ApiException: [405] Client error: `POST http://petstore.swagger.io/v2/pet` resulted in a `405 Method Not Allowed` response:
<?xml version="1.0" encoding="UTF-8" standalone="yes"?><apiResponse><type>unknown</type></apiResponse>
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php:162
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php:135
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/tests/AsyncTest.php:40
3) OpenAPI\Client\AsyncTest::testAsyncThrowingException
OpenAPI\Client\ApiException: [405] Client error: `POST http://petstore.swagger.io/v2/pet` resulted in a `405 Method Not Allowed` response:
<?xml version="1.0" encoding="UTF-8" standalone="yes"?><apiResponse><type>unknown</type></apiResponse>
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php:162
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php:135
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/tests/AsyncTest.php:40
4) OpenAPI\Client\AsyncTest::testAsyncApiExceptionWithoutWaitIsNotThrown
OpenAPI\Client\ApiException: [405] Client error: `POST http://petstore.swagger.io/v2/pet` resulted in a `405 Method Not Allowed` response:
<?xml version="1.0" encoding="UTF-8" standalone="yes"?><apiResponse><type>unknown</type></apiResponse>
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php:162
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php:135
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/tests/AsyncTest.php:40
5) OpenAPI\Client\AsyncTest::testAsyncHttpInfoThrowingException
OpenAPI\Client\ApiException: [405] Client error: `POST http://petstore.swagger.io/v2/pet` resulted in a `405 Method Not Allowed` response:
<?xml version="1.0" encoding="UTF-8" standalone="yes"?><apiResponse><type>unknown</type></apiResponse>
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php:162
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php:135
/Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/tests/AsyncTest.php:40
6) OpenAPI\Client\DebugTest::testEnableDebugOutput
OpenAPI\Client\ApiException: [405] Client error: `POST http://petstore.swagger.io/v2/pet` resulted in a `405 Method Not Allowed` response:
<?xml version="1.0" encoding="UTF-8" standalone="yes"?><apiResponse><type>unknown</type></apiResponse>
in /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php:162
Stack trace:
#0 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/tests/DebugTest.php(15): OpenAPI\Client\Api\PetApi->addPetWithHttpInfo(Object(OpenAPI\Client\Model\Pet))
#1 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/Framework/TestSuite.php(610): OpenAPI\Client\DebugTest::setUpBeforeClass()
#2 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/Framework/TestSuite.php(665): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#3 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(671): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#4 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/TextUI/Command.php(148): PHPUnit\TextUI\TestRunner->run(Object(PHPUnit\Framework\TestSuite), Array, Array, true)
#5 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/TextUI/Command.php(101): PHPUnit\TextUI\Command->run(Array, true)
#6 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/phpunit(61): PHPUnit\TextUI\Command::main()
#7 {main}
7) OpenAPI\Client\PetApiTest::testGetPetById
OpenAPI\Client\ApiException: [405] Client error: `POST http://petstore.swagger.io/v2/pet` resulted in a `405 Method Not Allowed` response:
<?xml version="1.0" encoding="UTF-8" standalone="yes"?><apiResponse><type>unknown</type></apiResponse>
in /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php:162
Stack trace:
#0 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/tests/PetApiTest.php(49): OpenAPI\Client\Api\PetApi->addPetWithHttpInfo(Object(OpenAPI\Client\Model\Pet))
#1 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/Framework/TestSuite.php(610): OpenAPI\Client\PetApiTest::setUpBeforeClass()
#2 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/Framework/TestSuite.php(665): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#3 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(671): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#4 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/TextUI/Command.php(148): PHPUnit\TextUI\TestRunner->run(Object(PHPUnit\Framework\TestSuite), Array, Array, true)
#5 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/TextUI/Command.php(101): PHPUnit\TextUI\Command->run(Array, true)
#6 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/phpunit(61): PHPUnit\TextUI\Command::main()
#7 {main}
8) OpenAPI\Client\StoreApiTest::testGetInventory
OpenAPI\Client\ApiException: [405] Client error: `POST http://petstore.swagger.io/v2/pet` resulted in a `405 Method Not Allowed` response:
<?xml version="1.0" encoding="UTF-8" standalone="yes"?><apiResponse><type>unknown</type></apiResponse>
in /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php:162
Stack trace:
#0 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/lib/Api/PetApi.php(135): OpenAPI\Client\Api\PetApi->addPetWithHttpInfo(Object(OpenAPI\Client\Model\Pet))
#1 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/tests/StoreApiTest.php(47): OpenAPI\Client\Api\PetApi->addPet(Object(OpenAPI\Client\Model\Pet))
#2 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/Framework/TestSuite.php(610): OpenAPI\Client\StoreApiTest::setUpBeforeClass()
#3 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/Framework/TestSuite.php(665): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#4 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(671): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#5 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/TextUI/Command.php(148): PHPUnit\TextUI\TestRunner->run(Object(PHPUnit\Framework\TestSuite), Array, Array, true)
#6 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/src/TextUI/Command.php(101): PHPUnit\TextUI\Command->run(Array, true)
#7 /Users/ybelenko/Sites/openapi-generator/samples/client/petstore/php/OpenAPIClient-php/vendor/phpunit/phpunit/phpunit(61): PHPUnit\TextUI\Command::main()
#8 {main}
ERRORS!
Tests: 73, Assertions: 140, Errors: 8, Skipped: 15. |
@ybelenko how did you get these errors? Running the same command as you, I get:
|
Oh right, I need to specify |
@ybelenko testing this with current master gives me exactly the same result. Is this broken in general? |
Yeah, that what I've been afraid of. Ok, let's fix it in another PR. |
Looks like this breaks the Travis CI build:
Ref: https://travis-ci.org/github/OpenAPITools/openapi-generator/builds/730663358 I'll comment it out for the time being. Please take a look when you guys have time. |
UPDATE: the failure seems to be intermittent. I'll take another look later today to see what's wrong. |
FYI. Filed #7525 to fix the issue. The results look good so far. |
@wing328 AFAIK, this PR should not influence that since it's mostly PHPDoc, we did find out the tests are failing in master prior to merging this also. |
@dkarlovi I agreed with you this PR didn't cause the failure. My guess is that it's due to changes in the PHP version or something else in the Travis build images. |
The PHP client generator could use quite a bit of love in general, I'm working on a specialization of this project and hoping to polish the generator quite a bit, basically doing #7284 here. |
Totally agreed with you there are rooms for improvements. Look forward to your PRs. |
@wing328 which generator would you recommend me to look at for good |
What do you mean by I believe |
@wing328 Sorry, I meant this
The use case is adding support to build glue code for one or more frameworks directly into the client, making the package work there automagically. Example
Generates:
so you can pull the package, it works as raw PHP code, but also as a Symfony or Laravel package by default. |
The library option usually refers to HTTP library. From what I recall, none of the generators support multiple You may want to add an option to take a list of supported frameworks/libraries for your use cases (e.g. |
@wing328 OK, I'll take a look, thanks! |
About
Purpose of this PR is to start fixing small and not so small issues found by PHPStan, after having applied CS Fixer 2.x.
hese templates are directly used in my specialization of OpenAPI generator, you can see a sample Reddit client. The purpose of the specialization is to iterate quickly, being able to collect many issues and fix them in bulk.
How to see the issues (required functional Docker):
Reported by Psalm:
You can also run PHPStan, but need to change
level
inphpstan.neon.dist
to:and then run:
With everything enabled (strict rules, all other checks), this version of PHPStan finds 109 errors for me.
PR checklist
./bin/generate-samples.sh
to 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.master
@jebentier (2017/07), @dkarlovi (2017/07), @mandrean (2017/08), @jfastnacht (2017/09), @ackintosh (2017/09) heart, @ybelenko (2018/07), @renepardon (2018/12)