Skip to content
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] Upgrade php-cs-fixer to 2.12, enables PHP >= 7.2 support #769

Merged
merged 3 commits into from
Sep 6, 2018

Conversation

mattmelling
Copy link
Contributor

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: master, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

The PHP client uses friendsofphp/php-cs-fixer v1.12 for PSR2 analysis. 1,12 doesn't supports PHP up to < 7.2.

PHP >= 7.2 support was added in php-cs-fixer v2 which made some changes to the API and default fixers/rules.

This PR upgrades php-cs-fixer to v2.12. The fixers/rules have been aligned for PSR2 as desired in the original PR on swagger-codegen (#3863). I've changed defaults for some of the rules to prevent lots of whitespace warnings from php-cs-fixer, these typically seem to be down to blank template values and are not raised when using the configuration in master.

Notes

php-cs-fixer added the idea of "risky" fixers in v2. Since the strict_param and strict_comparison fixers are considered risky, we must alter the command line to vendor/bin/php-cs-fixer fix --dry-run --diff -vvv --allow-risky yes even when using --dry-run.

/cc @jebentier, @dkarlovi, @mandrean, @jfastnacht, @ackintosh, @ybelenko

@mattmelling mattmelling changed the title [PHP Upgrade php-cs-fixer to 2.12, enables PHP >= 7.2 support [PHP] Upgrade php-cs-fixer to 2.12, enables PHP >= 7.2 support Aug 8, 2018
@@ -58,7 +58,7 @@ class ObjectSerializer
if ($value !== null
&& !in_array($openAPIType, [{{&primitives}}], true)
&& method_exists($openAPIType, 'getAllowableEnumValues')
&& !in_array($value, $openAPIType::getAllowableEnumValues())) {
&& !in_array($value, $openAPIType::getAllowableEnumValues(), true)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain, please, how this change is related to phpcs fixer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

php-cs-fixer raises a strict-param warning for in_array calls where the $strict parameter is false (which is the default). Passing true ensures we use strict comparison, i.e. value + type checking.

From my reading of the code I couldn't see any reason why we would need loose comparisons, especially since they can result in weird behaviour as documented here and here.

Copy link
Contributor

@ackintosh ackintosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is a breaking change with fallback, could you please file this PR against 3.3.x branch instead?

@wing328
Copy link
Member

wing328 commented Sep 2, 2018

@ackintosh we've set the current master to 3.3.0 (scheduled on coming Friday) after the 3.2.3 (patch) release.

@ackintosh
Copy link
Contributor

Oh, sorry 💦

LGTM ✨ I'll merge this PR later today if no further feedback.

@ackintosh
Copy link
Contributor

Upgrade Note

php-cs-fixer 2.12 requires PHP5.6. To use php client in PHP5.5, please set ~1.12 to composer.json as below:

    "require-dev": {
        "phpunit/phpunit": "^4.8",
        "squizlabs/php_codesniffer": "~2.6",
-       "friendsofphp/php-cs-fixer": "~2.12"
+       "friendsofphp/php-cs-fixer": "~1.12"
    },

@ackintosh ackintosh merged commit a8cbae4 into OpenAPITools:master Sep 6, 2018
@wing328 wing328 added this to the 3.3.0 milestone Sep 13, 2018
@wing328
Copy link
Member

wing328 commented Oct 2, 2018

@mattmelling thanks again for the PR, which is included in the v3.3.0 minor release: https://twitter.com/oas_generator/status/1046941449609068544

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
…PITools#769)

* upgrade php-cs-fixer to 2.12

* ran petstore-security-test for php

* updating openapi3 php petstore example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants