Update PHPCS to v3.4

[mikeselander]

PHPCS 3.3 included a plethora of useful bugfixes and improvements and will improve the Linter sniffing for several clients.

Reference: https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.3.0 for a more exhaustive list of bugfixes.

Resolves #70
Resolves #110

[mikeselander]

@rmccue I'm seeing a quite odd error coming through in the PHPUnit tests that is not coming through when running the CS suite in the CLI.

The fixtures/pass/use-order.php is suddenly getting caught as having quite a few false whitespace errors of the following kind in the unit tests:

[43]=>
  array(1) {
    [2]=>
    array(1) {
      [0]=>
      array(5) {
        ["message"]=>
        string(60) "Line indented incorrectly; expected at least 1 tabs, found 0"
        ["source"]=>
        string(40) "Generic.WhiteSpace.ScopeIndent.Incorrect"
        ["listener"]=>
        string(68) "PHP_CodeSniffer\Standards\Generic\Sniffs\WhiteSpace\ScopeIndentSniff"
        ["severity"]=>
        int(5)
        ["fixable"]=>
        bool(true)
      }
    }
  }

This is indicating that there is no tab character on that line (and every other indented line in that file) but there absolutely is a tab character there. This error does not run in the CLI.

I cannot find any files issues in the PHPCS repo and I'm a little stumped on what could be happening here. Any ideas?

[rmccue]

@mikeselander You may need to entirely wipe both composer.lock and vendor to replicate; I've found that the composer.lock can mess you up sometimes there.

[mikeselander]

@rmccue I've tried that to no avail and also this is happening on Travis as well as locally :(

[mikeselander]

Where I've gotten so far on debugging this today:

1. After updating the PHPCS standards via Composer and updating (including nuking /vendor and composer.lock), the CLI returns appropriate errors via running vendor/bin/phpcs tests/fixtures/ --standard=HM/ --no-cache
2. However, when running the test suite (via vendor/bin/phpunit), a whole new slew of Whitespace errors like I identified above pop up on both of the use-order.php files (both fail and pass).
3. These whitespace errors oddly only show up on the use-order.php files and not in inc/namespace.php or plugin.php files. However, if I copy namespace.php into the same directory level as use-order.php and re-name the file, I get the whitespace errors. This leads me to believe that there is another, potentially related issue with our linting of the namespace.php and plugin.php files.
4. I haven't been able to identify anyone else experiencing the same issue that we are and cannot find any regressions in the ScopeIndentation rule between 3.1.0 and 3.3.2

Halp

[nathanielks]

PHPCS 3.4 was released 24 days ago, I wonder if we could jump to that?

[mikeselander]

I went ahead and bumped one more version, good idea @nathanielks 👍

[joehoyle]

Not sure if this will be an issue, but just to note I've seen some peeps using php7.0 for travis/circle setups, so we should expect our next version to stop working in those situations.

[mikeselander]

We could set that back to 7.0 if that is a wide-spread practice. I'm a little concerned about our engineers testing in WP 7.0 when all of our major hosting partners are using 7.2+. I know of only one project where we don't have access to 7.0+ and they don't appear to be using the standards.

[joehoyle]

@mikeselander IMO merge at will!

[wvega]

Hi everyone! I think I found an explanation for the errors triggered when running the test suite:

The first time that Ruleset processes the standard during the execution of the tests, the tab-width arg defined in WordPress-Core/ruleset.xml is read and applied to the configuration using Config::setCommandLineValues() and Config::processLongArgument(), so that tabWidth is set to 4 instead of 0 (the default in Config).

However, when the second and following tests are executed, Config::processLongArgument() ignores the arg, returning before changing the value. This occurs because processLongArgument() is using the static property $overridenDefaults to keep track of which values have already been overwritten, and that property is not being cleared before (or at the end of) every test.

Since the value of tabWidth is reset every time a new instance of Config is created, the second and following tests are executed using tabWidth = 0.

The errors are triggered because having tabWidth = 0 prevents the Tokenizer from using the replaceTabsInToken() method. As a result, all tab characters in use-order.php or inc/namespace.php (whichever is tested last) are left untouched and read as a WHITESPACE token of length = 1 instead of length = 4 as required by ScopeIndentation.

A quick way to test the above is adding $this->config->tabWidth = 4; to setUp() in FixtureTests.php. The tests should now pass without problems.

I tried using @backupStaticAttributes enabled as a solution but it triggers a Fatal error.

[mikeselander]

@wvega thank you so much for digging into this and for the detailed explanation on what's going on. Your recommendation does, indeed, get the tests to pass so that we can move this PR forward. I owe you a drink or two 🍻

[nathanielks]

GREAT forensics, @wvega! Well done!

[mikeselander]

Note: the following are rule additions made in PHPCS 3.3 & 3.4. There doesn't look like anything controversial to me, but wanted to identify for when we make notes about this next release.

3.4

• Added new Generic.CodeAnalysis.EmptyPHPStatement sniff
  • Warns when it finds empty PHP open/close tag combinations or superfluous semicolons
• Added new Generic.Formatting.SpaceBeforeCast sniff
  • Ensures there is exactly 1 space before a type cast, unless the cast statement is indented or multi-line
• Added new Generic.VersionControl.GitMergeConflict sniff
  • Detects merge conflict artifacts left in files
• Added Generic.WhiteSpace.IncrementDecrementSpacing sniff
  • Ensures there is no space between the operator and the variable it applies to
• Added PSR12.Functions.NullableTypeDeclaration sniff
  • Ensures there is no space after the question mark in a nullable type declaration

3.3

• Added new Generic.PHP.LowerCaseType sniff
  -Ensures PHP types used for type hints, return types, and type casting are lowercase
• Added new Generic.WhiteSpace.ArbitraryParenthesesSpacing sniff
  • Generates an error for whitespace inside parenthesis that don't belong to a function call/declaration or control structure
  • Generates a warning for any empty parenthesis found
  • Allows the required spacing to be set using the spacing sniff property (default is 0)
  • Allows newlines to be used by setting the ignoreNewlines sniff property (default is false)
• Added new PSR12.Classes.ClassInstantiation sniff
  • Ensures parenthesis are used when instantiating a new class
• Added new PSR12.Keywords.ShortFormTypeKeywords sniff
  • Ensures the short form of PHP types is used when type casting
• Added new PSR12.Namespaces.CompundNamespaceDepth sniff
  • Ensures compound namespace use statements have a max depth of 2 levels
    The max depth can be changed by setting the 'maxDepth' sniff property in a ruleset.xml file
• Added new PSR12.Operators.OperatorSpacing sniff
  -Ensures operators are preceded and followed by at least 1 space