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

Build/PHPCS: update/improve the PHPCS configuration #284

Merged
merged 4 commits into from
Sep 5, 2018
Merged

Build/PHPCS: update/improve the PHPCS configuration #284

merged 4 commits into from
Sep 5, 2018

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Sep 5, 2018

Commit summary:

Build/PHPCS: rename ruleset

If the PHPCS ruleset is named .phpcs.xml, phpcs.xml, .phpcs.xml.dist or phpcs.xml.dist, it will automatically be picked up by PHPCS and you don't need to pass the ruleset name anymore.

Additionally:
Using a .dist file for a repo ruleset allows for individual developers to overrule the ruleset with a custom version (without the .dist file extension).
This makes testing of new additions/changes to the ruleset easier.
The master ruleset can be imported into a custom ruleset by using <rule ref="./.phpcs.xml.dist"/>.

Since PHPCS 3.1.0, it is also possible to use dot-prefixed files for the PHPCS config, allowing these files to be sorted with other configuration related files.

The loading order of the ruleset files in PHPCS, as of version 3.1.1., is:

  1. .phpcs.xml
  2. phpcs.xml
  3. .phpcs.xml.dist
  4. phpcs.xml.dist

References:

Composer/PHPCS: use a Composer plugin to sort out the PHPCS installed standards

For PHP_CodeSniffer to recognize external standards, the installed_paths command needs to be run.
The DealerDirect Composer PHPCS plugin can sort this out automatically for you, no matter which or how many external standards would be in use.

This plugin makes sure that the PHPCompatibility standard which is being installed via Composer can actually be used.

The relevant command will automatically be run after each composer install/update.

For convenience, I've added an additional script to the composer.json file to allow for calling the relevant command on demand.

References:

Build/PHPCS: actually use PHPCompatibility

  • Switches the PHPCompatibility dependency over to use the repo in the PHPCompatibility GitHub organisation rather than the one in wimg's personal account.
  • Adds the standard to the custom ruleset to activate it.

References:

PHPCS: move command-line arguments into the ruleset

  • Move the . to indicate the files to scan to the ruleset.
  • Move the -s to the ruleset and combine it with the showProgress/-p command line parameter.
  • Clean up the file paths PHPCS shows by adding the basepath directive.
    This will strip the paths down to the relative paths from the project root directory instead of showing the full file paths.
  • Adjust the relevant scripts in composer.json to match and make them work cross-platform.

General note:
While when used from within a custom ruleset, the impact is the same, <config> directives are originally intended to change the PHPCS configuration for all runs, while <arg> directives only apply to the current run.

@joshcanhelp joshcanhelp added this to the v5-Next milestone Sep 5, 2018
Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

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

Thank you @jrfnl! Really appreciate the complete PR description and references. 2 tiny whitespace changes here.

.phpcs.xml.dist Outdated Show resolved Hide resolved
.phpcs.xml.dist Outdated Show resolved Hide resolved
If the PHPCS ruleset is named `.phpcs.xml`, `phpcs.xml`, `.phpcs.xml.dist` or `phpcs.xml.dist`, it will automatically be picked up by PHPCS and you don't need to pass the ruleset name anymore.

Additionally:
Using a `.dist` file for a repo ruleset allows for individual developers to overrule the ruleset with a custom version (without the `.dist` file extension).
This makes testing of new additions/changes to the ruleset easier.
The master ruleset can be imported into a custom ruleset by using `<rule ref="./.phpcs.xml.dist"/>`.

Since PHPCS 3.1.0, it is also possible to use dot-prefixed files for the PHPCS config, allowing these files to be sorted with other configuration related files.

The loading order of the ruleset files in PHPCS, as of version 3.1.1., is:
1. `.phpcs.xml`
2. `phpcs.xml`
3. `.phpcs.xml.dist`
4. `phpcs.xml.dist`

References:
* https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#using-a-default-configuration-file
* https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.1.0
* https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.1.1 (change in the file loading order)
… standards

For PHP_CodeSniffer to recognize external standards, the `installed_paths` command needs to be run.
The DealerDirect Composer PHPCS plugin can sort this out automatically for you, no matter which or how many external standards would be in use.

This plugin makes sure that the `PHPCompatibility` standard which is being installed via Composer can actually be used.

The relevant command will automatically be run after each `composer install/update`.

For convenience, I've added an additional script to the `composer.json` file to allow for calling the relevant command on demand.

References:
* https://github.com/squizlabs/PHP_CodeSniffer/wiki/Configuration-Options#setting-the-installed-standard-paths
* https://github.com/DealerDirect/phpcodesniffer-composer-installer
* Switches the PHPCompatibility dependency over to use the repo in the `PHPCompatibility` GitHub organisation rather than the one in `wimg`'s personal account.
* Adds the standard to the custom ruleset to activate it.
* Move the `.` to indicate the files to scan to the ruleset.
* Move the `-s` to the ruleset and combine it with the `showProgress`/`-p` command line parameter.
* Clean up the file paths PHPCS shows by adding the `basepath` directive.
    This will strip the paths down to the relative paths from the project root directory instead of showing the full file paths.
* Adjust the relevant scripts in `composer.json` to match and make them work cross-platform.

**General note**:
While when used from within a custom ruleset, the impact is the same, `<config>` directives are originally intended to change the PHPCS configuration for all runs, while `<arg>` directives only apply to the current run.
@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 5, 2018

@joshcanhelp Hi Josh, thanks for the quick review and sorry about that spacing mishap. Thanks for catching it. I work in too many different standards, so that's something which quickly goes awry.

I've fixed up that particular commit now.

While I was at it, I realized I hadn't added the "user" phpcs.xml filenames to the .gitignore file, so I've also fixed up the first commit and added that.

@joshcanhelp
Copy link
Contributor

@jrfnl - Thanks again! Everything looks great and I'll merge it now.

I used similar standards on another PHP lib we have here:

https://github.com/auth0/wp-auth0

Would be happy to merge same/similar changes there. Otherwise, I'll add that to our backlog and use your notes here to do it myself.

Really appreciate everything you're doing in this space!

@joshcanhelp joshcanhelp merged commit 0040919 into auth0:master Sep 5, 2018
@jrfnl jrfnl deleted the feature/fix-phpcompatibility-usage branch September 5, 2018 16:30
@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 5, 2018

Would be happy to merge same/similar changes there.

I'll see what I can do, but I'm not making any promises. I'm currently working through the packagist list of packages which have the wimg version of PHPCompatibility as a dependency to move those over to use the version in the PHPCompatibility GH organisation and, for now, only focusing on the biggest packages at that.

I don't see your other package listed there. Is that correct ?

@joshcanhelp
Copy link
Contributor

It looks like I already updated to the PHPCompatibility package:

https://github.com/auth0/wp-auth0/blob/master/composer.json#L10

I can make the rest of the changes here without too much trouble.

Not listed on packagist at the moment as it's a standalone WP plugin.

@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 5, 2018

I can make the rest of the changes here without too much trouble.

@joshcanhelp 👍

While you're at it:

@github-actions
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants