Created by: dmetzner
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 Throwable Since 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
variable Improved 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
method to 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:
- where sometimes the return value was typed as
: array|\array
which is invalid - where null has to be returned; e.g: in case of an error, where no response model can/should be built
This was not possible when the type was fixed to e.g.:
: array|SuccessResponseModel
- where the API can return different Models for different status codes:
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 204'204': content: application/json: schema: type: object '422': content: application/json: schema: type: object
- where sometimes the return value was typed as
-
Default 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
-
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. -
Run the following to build the project and update samples: ./mvnw clean package ./bin/generate-samples.sh ./bin/utils/export_docs_generators.sh
./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
(6.0.1) (patch release),6.1.x
(breaking changes with fallbacks),7.0.x
(breaking changes without fallbacks) -
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request. @jebentier (2017/07), @dkarlovi (2017/07), @mandrean (2017/08), @jfastnacht (2017/09), @ackintosh (2017/09) ❤ ️, @ybelenko (2018/07), @renepardon (2018/12), @BenjaminHae, @wing328