Add PHPCompatibilityWP Standard.

[tfrommen]

For now, I set it up to test for PHP version 7.0 and higher.

Resolves #74

[rmccue]

Looks good to me.

@nathanielks anything we should know about the PHPCompatibility sniffs based on your experience with using them on our repos?

[nathanielks]

@rmccue sure! There will likely be a lot of false positives, so I'd actually recommend not adding this to hm-linter (unless you remove a bunch of rules). For reference, here's a report performed on our own repos: https://github.com/humanmade/hm-stack/files/2015027/reports.tar.gz

If you want to filter out everything but the errors, you can use jq:

jq '.files | to_entries | map(select(.value.errors != 0)) | from_entries' path/to/report.json

Errors relating to a now deprecated Mcrypt extension were the primary offender. Another issue is that the sniffs do static code analysis, so it won't detect feature detection flags that use newer, non-deprecated code. This means older code will still trigger the sniff which will just produce a ton of noise.

I see using PHPCompatibility manually as a great tool, but I don't think I'd recommend using it in an automated fashion like this. Project-specific configuration would be required (ignoring directories and certain sniffs).

[tfrommen]

Hm, I'm not quite following, @nathanielks. Didn't you just say a couple days ago that we should make more use of coding standards (tooling) in general? 😁

Please mind that this is not the old PHPCompatibility ruleset, but the brand-new WordPress-specific one PHPCompatibilityWP, which has been published just a couple months ago. This will not trigger any warnings for general PHP issues that are no issues in a WordPress context, for example, because there is a backfill in place.

In my opinion, it is (super) important to make sure that all of your codebase is compliant with a set PHP version. This will mean that you also get to see some/several false-positives, sure. But you can add exceptions where you need them. This might be some effort in the beginning, but after that, you only have to update a line or two every other month/release, and if you don't make use of too many third-party plugins, not even that.
Please see here for a project where I did just that.

Just like the rest of the PHP world, we decided to pick PHP_CodeSniffer as our PHP-specific linting tool. So I'd expect all new (and also not-so-old) projects to have a custom ruleset file—or be fine with having the Human Made coding standards run against their code. PHP_CodeSniffer is super configurable, so you can switch off certain error codes, certain sniffs, or certain categories—in general, or just for one or more specific file paths/patterns. And you can, of course, not only disable, but also customize every sniff (including severity, message etc.) to your liking.

Last but not least, I only chose 7.0- as default version range. I mean, I thought it makes sense, because this is what we are using the most, but you can override this in your local config in the repo no problem.

So, I'd like to see us make use of PHP-compatibility rules. Let's overcame any potential obstacles together. 😎

[rmccue]

Also, I would presume that all projects are excluding any non-first-party code from linting anyway, which should reduce a lot of the false positives; presumably you couldn't do that with running the tool separately.

I think we can move forward with this tentatively, but we should do some manual testing of projects as well.

[rmccue]

(Also, I'll merge this after I do some manual testing this week)

[nathanielks]

Didn't you just say a couple days ago that we should make more use of coding standards (tooling) in general?

I did, but an important note is applicable tooling, not more for the sake of more.

---

[nathanielks]

Hit the wrong button!

[nathanielks]

So I'd expect all new (and also not-so-old) projects to have a custom ruleset file

@rmccue I was of the impression HM Linter didn't support custom rulesets?

PHP_CodeSniffer is super configurable, so you can switch off

Which is part of the reason I'm not in favor of adding PHP version compatibility to the coding standards. As I understand it, you either turn the sniff off entirely, or turn it off for each line or section of code. This presents two problems: either we turn a sniff off entirely, preventing future updates from being checked, OR we litter our repositories with compat ignores. In both cases, we lose visibility into whether things are actually broken and the latter is annoying to maintain in the event of plugin updates and the like.

"But we can just ignore 3rd party code and only test our code?"

That is valid, but also undermines the purpose of adding this, as I understand it. We're trying to gain visibility into whether our application, dependencies and all, are compatible with the desired versions of PHP. Ignoring sniffs/files undermines this ability.

---

While I'm in the middle of this, let me clarify: I am not remotely against tests. At all. Ever. I am easily their biggest fan and proponent. We need more and better tests. Ok, moving on.

---

PHP version sniffs make sense to me at these times:

• When new 3rd party code is being added to a repository
• When the PHP version the code is running on is being changed.

We don't need to check that the code is valid on every PR because we already know it's running. This is something we only need to check when we're changing versions. We'll be notified things don't work for existing versions while we're developing via Fatals or Warnings. Running a linter for PHP Version compatibility on a regular basis is unnecessary overhead, imo.

Finally, the largest reason I'm hesitant to add tests is whether or not it can answer these questions:

• Do these tests inhibit the development feedback cycle by making testing feedback longer?
• How much extra noise do these tests produce?

I'm not a fan of adding tests for the sake of adding tests as it will hamper the feedback loop and potentially add unnecessary noise. So all that said: is it possible to keep visibility and reduce unnecessary noise?

[nathanielks]

A significant point of context as well: I'm thinking of this with the understanding it's making its way upstream to HM Linter, so it greatly affects that experience.

[rmccue]

@rmccue I was of the impression HM Linter didn't support custom rulesets?

It supports custom rulesets, just not custom external rulesets. If you have a phpcs.ruleset.xml in your project, it will use it, you just can't use any additional Composer dependencies.

That means that most projects will have ignores set up for any code they don't own.

Adding this sniff is purely to make sure the code that we are writing is compatible with PHP 7.0, and we'll continue pushing that forward in the future. Testing whether the whole project is compatible is a different kettle of fish, and not an issue we're trying to solve here.

[nathanielks]

👍

[mikeselander]

@nathanielks @rmccue @tfrommen it looks to me like we've come to a tentative agreement on including this ruleset, is it accurate to say that this is agreed upon to move forward?

fwiw, even if we don't require the rules in our ruleset I'd very much like to make the ruleset available via Composer so that Linter bot could run these.

Also, looks like v2.0.0 has been released since this PR was written and might be worth bumping to: https://github.com/PHPCompatibility/PHPCompatibilityWP/releases

[jrfnl]

In the mean time v 2.0.0 has come out.... 😕

[tfrommen]

Thanks for chiming in, Juliette! (We know! 🙈)

I just bumped the package to ^2.0.0, and ensured PHP compatibility checks for 7.1-, as PHP 7.1 is currently the minimum version we both advertise and require in various places such as composer.json files and documentation etc.

[jrfnl]

@tfrommen Thanks!

Just read through the complete thread and there are a few things I can add here.

FYI: This statement from above is not really correct:

Last but not least, I only chose 7.0- as default version range. I mean, I thought it makes sense, because this is what we are using the most, but you can override this in your local config in the repo no problem.

Overruling a PHPCS config setting set within a standard from within a custom ruleset does not work. See this open issue: squizlabs/PHP_CodeSniffer#2197

Since PHPCS 3.3.0 - see squizlabs/PHP_CodeSniffer#1821 - you can overrule a config setting from the command-line though, which in this case you would do with --runtime-set testVersion 5.6- to change the testVersion for a project to 5.6- (= must be compatible with PHP 5.6 and higher).
This kind of command-line addition can, of course, be added to a composer script like:

      "check-cs": [
        "@php ./vendor/squizlabs/php_codesniffer/bin/phpcs --runtime-set testVersion 5.4-"
      ],

We'll be notified things don't work for existing versions while we're developing via Fatals or Warnings. Running a linter for PHP Version compatibility on a regular basis is unnecessary overhead, imo.

This is incorrect. PHPCompatibility will find quite some things for which PHP will not throw any fatals or warning, but where you will see different behaviour between PHP versions. Often these things won't be caught by unit tests either unless a unit test was set up specifically to test for it.

Do these tests inhibit the development feedback cycle by making testing feedback longer?

Barely. If PHP_CodeSniffer is being run anyway, it's only the loading of the rules which will add a second or two, other than that, you will hardly notice it.
FYI: PHPCompatibility is set up to be highly efficient and will bow out as soon as it can for any given sniff. /cc @nathanielks

[mikeselander]

I'm investigating an issue with the test suite whereby it's loading the PHPCompatibility suite twice and triggering a failure after upgrading to v2.0.0. There is some discussion in PHPCompatibility/PHPCompatibility#793.

The failure is coming when we load in the PHPCS Ruleset class - https://github.com/humanmade/coding-standards/blob/master/tests/FixtureTests.php#L61. Even though we're specifically only loading in our HM ruleset, the PHPCompatibility ruleset is loading itself alongside it which triggers the below error. This might be intentional behavior; still digging in to find out.

Fatal error: Cannot declare class PHPCompatibility\Sniffs\FunctionUse\RequiredToOptionalFunctionParametersSniff, because the name is already in use in /Users/mikeselander/repos/coding-standards/vendor/phpcompatibility/php-compatibility/PHPCompatibility/Sniffs/FunctionUse/RequiredToOptionalFunctionParametersSniff.php on line 310

Both (independently of each other) removing the PHPCompatibilityWP ruleset and downgrading it to 1.0.0 fix the problem and the test suite runs fine.

Some various potential refs:

• 3.x: Autoloader failing to load arbitrary files when installed_paths only set via a custom ruleset squizlabs/PHP_CodeSniffer#1591
• What is the easiest way of testing custom coding standard squizlabs/PHP_CodeSniffer#596

I've run out of time for this for today but if anyone has some ideas, I'm all 👂s

[mikeselander]

It only took a year and a half, but the tests finally pass 🏅

[mikeselander]

🚢