Consider aligning PHPCS ruleset with WPGraphQL.

[justlevine]

Consider aligning PHPCS ruleset with WPGraphQL.

[justlevine]

To further this goal across the ecosystem (and a deeper explanation on why a shared ruleset is important), I released this WPGraphQL Coding Standards, which should make it significantly easier to evaluate the impact and value on this codebase.

To test:

1. run composer require --dev axepress/wp-graphql-cs locally to install the sniffs.
2. Copy the example phpcs.xml.dist.example file over to the root directory and rename it to phpcs.xml.
3. Replace the included/excluded directories in phpcs.xml with those from phpcs.xml.dist
4. Run vendor/bin/phpcs and review the results (or even better run vendor/bin/phpcbf, and then diff the branch).

[theodesp]

@justlevine Thanks. We do maintain a phpcs plugin

https://github.com/wpengine/wpengine-coding-standards

do you think we can include this ruleset there?

[justlevine]

@theodesp looking at the ruleset, the sniffs WPEngine-Strict has cherry-picked are already included in WPGraphQL-Minimum, as are all the ones in the phpcs.xml.dist of this repo.

The only conflict I see is the wp-core preference for long array() syntax - WPGraphQL core (and the ruleset) enforces the short [] syntax.

The next level up, WPGraphQL-Strict includes functional sniffs about PHP best practices, so that also should be fine.

The one that gives me pause is the optional WPGraphQL-Extra that handles formatting, alignment, and some extra doc-block requirements, as it's opinionated and might go against what the Faust team uses internally (although unenforced by the WPEngine ruleset). I did my best to use this repo as a guide for that (e.g. the no-space before a method's : ReturnType goes against my personal preference and what WPGraphQL core currently does), considering that afaik, WPEngine has the largest number of paid contributors to the ecosystem. I'm sure the team will have feedback, and I'm looking forward to incorporating it 😁

[theodesp]

Closing this ticket since #165 is merged.

[josephfusco]

Hey @justlevine has #111 satisfied this issue? Thank you!

[justlevine]

@josephfusco when I reviewed it pre-merge, I noted that PR was actually justification in support of this issue, and as far as I can tell, the merged version didnt make any changes to the PHPCS ruleset.

A stricter (and ecosystem-shared) PHPCS ruleset would have prevented/autofixed many of those issues from leaking into the codebase. Further, it would catch additional smells missed by that PR (to be fair, last time I reviewed was pre-merge) , and most importantly it would keep future changes to the codebase clean so a housekeeping PR like #111 would be mostly unnecessary.

[justlevine]

@josephfusco take a look at the individual commits in #126 . Beyond switching to modern array syntax, it caught (and in many cases fixed), typing smells, memory consumption from nonstatic callables.

To get a feel for its future usefulness, you can cherry pick the first commit onto a branch from before my and/or @mindctrl 's series of housekeeping PRs, and see how many would have been autofixed or at least caught before merge.