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] Better PSR2 compatibility #3863

Merged
merged 3 commits into from
Sep 27, 2016

Conversation

dkarlovi
Copy link
Contributor

@dkarlovi dkarlovi commented Sep 24, 2016

PR checklist

  • Read the contribution guildelines.
  • Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

These changes fix the emitted PHP compatibility with the declared PHP-FIG's PSR2 coding style. This can be verified by using the standard tool for the task in the PHP ecosystem, PHP-CS-Fixer, for which the configuration has also been included with the .php_cs file.

To verify compatibility, run as follows:

# run swagger-codegen as usual

# install dev dependencies, which now includes PHP-CS-Fixer
composer install

# run PHP-CS-Fixer which should NOT alter any files at all
vendor/bin/php-cs-fixer fix --dry-run --diff -vvv

Notes

  • as the emitted code required PHP 5.4 in composer.json, array notation has been switched to new short array syntax introduced in PHP 5.4 to better match new best practices, this has also been enforced in .php_cs
  • additional custom fixers (see PHP-CS-Fixer's Usage) have been enabled to make output more strict (and thus more predictable)

@dkarlovi
Copy link
Contributor Author

Please not, I did run bin/php-petstore.sh, but did not commit its output. Don't know if it's required, please advise. Tnx.

@cbornet
Copy link
Contributor

cbornet commented Sep 24, 2016

Yes, you should commit it so that your PR is tested by CI

@dkarlovi
Copy link
Contributor Author

@cbornet thanks, done.

@dkarlovi dkarlovi changed the title Feature/php cs fixer compat [PHP] Better PSR2 compatibility Sep 24, 2016
@wing328 wing328 added this to the v2.2.2 milestone Sep 26, 2016
@wing328
Copy link
Contributor

wing328 commented Sep 26, 2016

@dkarlovi thanks for the PR.

For array() replaced by [], shall we keep array() to make it compatible with PHP version before 5.4? This is not to say we officially support 5.3 or older version but just that people can still use it (without any support from us) in case they're still using 5.3 or older version in their environment.

Ref: http://php.net/manual/en/language.types.array.php

As of PHP 5.4 you can also use the short array syntax, which replaces array() with [].

cc @arnested

@dkarlovi
Copy link
Contributor Author

@wing328 I'm fine either way, but, the way I see it, if the declared requirement in composer.json is 5.4, you can expect it will not work out of the box with 5.3.

What we could do is add a blurb on how to convert it to classic array syntax by using PHP-CS-Fixer's long_array_syntax fixer. This way you don't officially support PHP 5.3, but users can work around that (as they do right now anyway).

@arnested
Copy link
Contributor

arnested commented Sep 26, 2016

This looks really fine to me. Nice job, @dkarlovi.

I'm still in the habit of using array().

But I think we should just move on and go all in on our 5.4 dependency and embrace []. It's time to move on and we probably haven't many users stuck on 5.3 but depending on updated versions of codegen.

@wing328
Copy link
Contributor

wing328 commented Sep 26, 2016

OK. Let's move forward with this :)

@wing328 wing328 merged commit 70fa2fb into swagger-api:master Sep 27, 2016
@wing328
Copy link
Contributor

wing328 commented Sep 27, 2016

@dkarlovi btw, I was not able to run the following:

vendor/bin/php-cs-fixer fix --dry-run --diff -vvv

vendor/bin/phpcs is available instead. Maybe it's because I'm using the latest version (2.7.0)

@dkarlovi
Copy link
Contributor Author

Thanks, @arnested.

@wing328 after you run composer install, you must have it as it has been added as a dev dependency. Then you'll have the command.

Thanks for the merge.

@dkarlovi dkarlovi deleted the feature/php-cs-fixer-compat branch September 27, 2016 07:02
@wing328
Copy link
Contributor

wing328 commented Sep 27, 2016

@dkarlovi I did but no luck:

SwaggerClient-php|master⚡ ⇒ vendor/bin/phpcs --version
PHP_CodeSniffer version 2.7.0 (stable) by Squiz (http://www.squiz.net)

@dkarlovi
Copy link
Contributor Author

@wing328 have you re-generated the composer.json file? Also, if you have composer.lock, Composer will warn you it's out of date, but still do as it says (and not install php-cs-fixer). Please remove it (or run composer update to update it).

phpcs is another tool altogether and not related here.

@wing328
Copy link
Contributor

wing328 commented Sep 27, 2016

@dkarlovi ok, will give it another try with a new PHP SDK. If you don't hear from him, you can assume I don't encounter any issue :)

@dkarlovi
Copy link
Contributor Author

@wing328 if you still have an issue, I'll verify that it's not in my code and file a new PR to fix it if it is. :)

@arnested
Copy link
Contributor

composer.lock was why it didn't install php-cs-fixer for me at first. I deleted it and ran composer install again.

acramatte added a commit to comerge/swagger-codegen that referenced this pull request Oct 4, 2016
* upstream/master: (79 commits)
  add undertow
  Add a new cli command to output version information (2nd attempt) swagger-api#3892 (swagger-api#3899)
  fix python flask controller without tag (default_controller)
  [aspnet5] Fix basePath application to operations (swagger-api#3911)
  Bugfix/issue 3723 (swagger-api#3726)
  Cgardens nested object regex (swagger-api#3879)
  [Cpprest] Fixing issue swagger-api#3773 (swagger-api#3876)
  escape callback parameter for java(okhttp) and python
  fix warning in html generator
  [PHP] fix PHPUnit invocation, add basic phpunit.xml.dist (swagger-api#3864)
  [Java] Remove duplicated model description in Spring, JAX-RS models (swagger-api#3887)
  [PHP] Better PSR2 compatibility (swagger-api#3863)
  Mention security script in pull request template
  [Swift] Use thread safe manager dictionary
  Replace ^M with new line (\r) in mustache template (swagger-api#3865)
  [swfit] fix url param with base name
  [JaxRS]Show correct default value on CLI option description (swagger-api#3862)
  add title, description to HTML output (swagger-api#3860)
  fix trailing comma in go api client
  fix typescript-fetch base path by removing ending slash
  ...
@dkarlovi dkarlovi mentioned this pull request Apr 3, 2017
1 task
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.

4 participants