-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[PHP] Enhance Symfony generator #12532
[PHP] Enhance Symfony generator #12532
Conversation
11f65a3
to
41eded1
Compare
@dmetzner thanks for the PR. Can you please merge the latest master into your branch as well since we've added some tests for PHP Symfony server in the CI? |
41eded1
to
7c107ab
Compare
@dmetzner thanks again for the PR but I got the following errors when running the tests locally:
Can you please take a look when you've time? |
- Add missing typehints - Add missing use statements - Simplify model ctor - Change fallthrough Exception to Throwable - prevent storing result of void methods - fix validate method - add default null values to model properties - simplify API interface return values - fix/rework default value implementation - fix optional params deprecation warnings - .. For more details check out the PR: OpenAPITools#12532
7c107ab
to
9d44dcf
Compare
- Skip risky tests - Fix type hint error - Fix class exists tests - Fix broken doc block - Enhance annotations - Update phpunit.xml.dist - Fix test config resource location
4067e67
to
4df930b
Compare
@wing328 Thanks for pointing that out. There has been a typo in a type hint.
|
Thanks for the quick fix.
|
Sorry for necro-posting, but... When client skip this property, we get fatal error (because of getter return null, not boolean)... What can we do with it? Maybe default values need to be moved to getter function? |
This merge request provides enhancements to the Symfony PHP generator all over the place by reducing possible syntax errors, stricter types, and "reworking" the default values feature. For more details check out the what has changed section.
These enhancements were added on the fly during the process of upgrading the Catrobat API to the new v6 generator. Therefore, this merge request contains quite a lot of changes for a single merge request. In case the merge request should be split into multiple smaller ones, please just say so :)
What has changed & and why?
Added missing type hints all over the place to:
Stricter types allow to write a more secure code and detect possible issues with static analysis.
For example, the Catrobat API now passes all phpstan checks up to level 4 without the necessity to manually fix some issues.
Added missing use statements.
This was done to improve static analysis and prevent warnings.
Simplified model constructor
Switched from ìsset` to the ternary operator
Reason: Simpler code is better code -> fewer warnings in most IDEs.
Switched API controller
$fallthrough
Exception to ThrowableSince more and more type hints have been introduced, the possibility to trigger type errors has also been introduced.
This switch allows the runtime to catch such type errors & and send an error response without crashing.
void
method result is no longer stored in a$result
variableImproved controller API handling. In case a method returns nothing, nothing can't be stored in the $result variable. Hence, the result variable has no purpose and is just code smell.
changed the validate method
By changing the logic to build the error string in the validate method, the method no longer has to force a string cast on the object, which could produce an unexpected result and hence is also a warning in most static analyzers.
enhancing the
deserializeString
methodto support bool values, without the necessity to transform a boolean string to a boolean.
This allows setting default request values already to a boolean value. The same logic was already implemented for integers.
Added default null values to the model properties.
This allows accessing an uninitialized model's property without triggering a Throwable. Basically, resulting in the same logic which was already implemented in the model's constructor. However, this fixes the issue when no constructor is used to instantiate the model, for example, when a request has not the property. Since the object creation in the deserialization process is not using the constructor the property remained uninitialized and crashed during access in the getter. In addition, it was not possible to catch this Throwable with methods such as
isset
orempty
, because those methods can only be used directly on the property and not a getter. Hence another fix to this issue could have been to get rid of those getters and setters and make the property public.Simplified the return types of the API interfaces.
This fixes various bugs introduced with v6 and return type declarations.
204 API will remain
: void
; Everything else is now typed as: array|object|null
It's not perfect, however, an easy first fix for the following bugs:
: array|\array
which is invalidThis was not possible when the type was fixed to e.g.:
: array|SuccessResponseModel
E.g: 422 ErrorResponseModel + 200 SuccesResponseModel
This was not possible when the type was fixed to e.g:
: array|SuccessResponseModel
Note: In case the
: array|object|null
return type declaration is needed instead for a 204 status code, for example for a 422 error Response which has content, the API specification can be "tricked" by defining content for 204Default values are now actually working.
The current implementation of default values for request parameters has no useful effects, however, introduces deprecations warnings and requires manual work within the projects implementing the API to fix those issues.
Php8.0: Deprecation warning
optional before required param
- Example:function getFoo(string $type = "useless", int &$responseCode, array $responseHeaders)
Now we have an optional parameter($type) before a required parameter($responseCode) which is bad. Hence, the optional parameter is no longer optional, and therefore the default value can't be used for something other than documentation.
By removing the default values from the method signature we can get rid of this deprecation warning. However, the defined default values in the specification would still have zero purposes, other than documentation. Therefore, default values have been added to the, in my opinion, correct spot. From now on the second parameter in the requests get-method is used, which does exactly what is needed for the default values to work as expected. Example:
$limit = $request->query->get('limit', 20)
Now the $limit is either set by the request or by the default value, and all method parameters contain the expected value.PR checklist
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
(6.0.1) (patch release),6.1.x
(breaking changes with fallbacks),7.0.x
(breaking changes without fallbacks)@jebentier (2017/07), @dkarlovi (2017/07), @mandrean (2017/08), @jfastnacht (2017/09), @ackintosh (2017/09) ❤️, @ybelenko (2018/07), @renepardon (2018/12), @BenjaminHae, @wing328