Getbeans / Beans

Beans WordPress Theme Framework. The default branch is set to development, please switch to the master branch for production.
https://www.getbeans.io
Other
392 stars 61 forks source link

Use latest PHP_CodeSniffer #144

Closed GaryJones closed 6 years ago

GaryJones commented 6 years ago

Just because Beans is made to work (for consumers) at PHP 5.2, doesn't mean that development tools need that very low restriction.

PHPCS 3.* requires PHP 5.4, so it's more than fine to expect any contributor to Beans to have that to be able to run the PHPCS checks.

PHPCS 3. is a rewrite that uses namespaces etc. but also fixes many bugs and has many improvements. Your other PHPCS-related dependencies all work with PHPCS 3..

You should be explicit about requiring it the composer.json (it's not there at all, which is why it only installs the 2.9 branch), and optionally specify a php version of 5.4 (or higher) in the require-dev section.

FWIW, Genesis has PHP 5.6 or 7.* as the require-dev version, and we explicitly require PHPCS 3.2:

screenshot 2018-02-28 08 16 52

You can still set PHPCompatibility to check for PHP 5.2 compatibility for the main code like you already specify that PHP 5.6 (or higher) is needed for the tests/ directory.

hellofromtonya commented 6 years ago

I was concerned about catching PHP 5.2 incompatible constructs in the Beans code. Using your example of defining the minimum PHP versions in the require-dev configuration, I ran a few tests.

In the init.php file, I changed this line of code:

define( 'BEANS_API_PATH', wp_normalize_path( trailingslashit( dirname( __FILE__ ) ) ) );

to this:

define( 'BEANS_API_PATH', wp_normalize_path( trailingslashit( __DIR__ ) ) );

Then I ran the sniffer: composer phpcs-src lib/api/init.php and got these results:

FILE: /app/public/wp-content/themes/tm-beans/lib/api/init.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 30 | ERROR | __DIR__ magic constant is not present in PHP version 5.2 or
    |       | earlier (PHPCompatibility.PHP.NewKeywords.t_dirFound)
----------------------------------------------------------------------

That is the error we would expect since __DIR__ is not available in PHP 5.2. That shows the new composer.json configuration does work.

  "require": {
    "php": "^5.2|^7",
    "composer/installers": "^1.4",
    "roave/security-advisories": "dev-master"
  },
  "require-dev": {
    "php": "^5.6|^7",
    "brain/monkey": "^2.0",
    "dealerdirect/phpcodesniffer-composer-installer": "^0.4.3",
    "mikey179/vfsStream": "^1.6",
    "phpunit/phpunit": "~4.8 || ~5.7.9",
    "sirbrillig/phpcs-variable-analysis": "^2.0",
    "wimg/php-compatibility": "^8.0",
    "wp-coding-standards/wpcs": "^0.14.0",
    "xwp/wp-dev-lib": "^1.0.1"
  },

Thank you, @GaryJones, for pointing this out. We'll get a PR that includes the above composer.json changes and swaps out the deprecated phpcs ignore syntax.