WPTT / WPThemeReview

PHP_CodeSniffer rules (sniffs) to enforce WordPress theme review coding conventions
MIT License
208 stars 38 forks source link

WPThemeReview autoload doesn't work with Composer project-based install #184

Closed ntwb closed 5 years ago

ntwb commented 5 years ago

Sorry for deleting all the issue template, but none of it was relevant to reporting a bug that is not a rule

Issue

A bug where using Composer to install the WPThemeReview package per install instructions fails:

Run using Composer script:

❯ composer run-script lint
> phpcs --standard=phpcs.xml.dist --report-summary --report-source
Uh oh... can't find WPCS.

If you use Composer, please run `composer install`.
Otherwise, make sure you set a `WPCS_DIR` environment variable
pointing to the WPCS directory.
Script phpcs --standard=phpcs.xml.dist --report-summary --report-source handling the lint event returned with error code 1

Running phpcs directly:

❯ ./vendor/bin/phpcs -p . --standard=WPThemeReview
Uh oh... can't find WPCS.

If you use Composer, please run `composer install`.
Otherwise, make sure you set a `WPCS_DIR` environment variable
pointing to the WPCS directory.

To Reproduce

My relevent composer.json file contents:

    "require-dev": {
        "dealerdirect/phpcodesniffer-composer-installer": "~0.4.4",
        "wptrt/wpthemereview": "dev-develop"
    },
    "scripts": {
        "format": "phpcbf --standard=phpcs.xml.dist --report-summary --report-source",
        "lint": "phpcs --standard=phpcs.xml.dist --report-summary --report-source"
    }

My phpcs.xml.dist file:

<?xml version="1.0"?>
<ruleset name="WordPress Theme Coding Standards">
    <!-- See https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-ruleset.xml -->
    <!-- See https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress-Core/ruleset.xml -->

    <!-- Set a description for this ruleset. -->
    <description>A custom set of code standard rules to check for WordPress themes.</description>

    <arg name="extensions" value="php"/>

    <!-- Strip the filepaths down to the relevant bit. -->
    <arg name="basepath" value="./"/>

    <!-- Check up to 20 files simultanously. -->
    <arg name="parallel" value="20"/>

    <!-- Show sniff codes in all reports -->
    <arg value="ps"/>

    <file>.</file>

    <!-- Directories and third party library exclusions -->
    <exclude-pattern>/node_modules/*</exclude-pattern>
    <exclude-pattern>/vendor/*</exclude-pattern>

    <!-- Include the WP Theme Review ruleset, with space for exclusions if necessary. -->
    <rule ref="WPThemeReview">

    </rule>

</ruleset>

Expected Behaviour

For WPThemeReiew to work without the requirement to have WPCS or WPCS_DIR constants defined, of a .pathtowpcs file.

The use of the dealerdirect/phpcodesniffer-composer-installer registers and installs WPCS rulesset correctly and as such this registration should be used and not the above constants or dotfile.

Actual Behaviour

Without manually pointing to the WPCS path WPThemeReview fails.

Environment

Composer version 1.7.2 2018-08-16 16:57:12

~/Developer/Code/WordPress/twentynineteen master*❯ phpcs -i
The installed coding standards are PEAR, Zend, PSR2, MySource, Squiz, PSR1, PSR12, WordPress-VIP, WordPress, WordPress-Extra, WordPress-Docs and WordPress-Core
~/Developer/Code/WordPress/twentynineteen master*❯ ./vendor/bin/phpcs -i
The installed coding standards are PEAR, Zend, PSR2, MySource, Squiz, PSR1, PSR12, PHPCompatibilityWP, WordPress-VIP, WordPress, WordPress-Extra, WordPress-Docs, WordPress-Core, WPThemeReview, PHPCompatibility, PHPCompatibilityParagonieRandomCompat and PHPCompatibilityParagonieSodiumCompat
❯ composer install
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Package operations: 7 installs, 0 updates, 0 removals
  - Installing squizlabs/php_codesniffer (3.3.2): Loading from cache
  - Installing dealerdirect/phpcodesniffer-composer-installer (v0.4.4): Loading from cache
  - Installing phpcompatibility/php-compatibility (9.0.0): Loading from cache
  - Installing phpcompatibility/phpcompatibility-paragonie (1.0.0): Loading from cache
  - Installing wp-coding-standards/wpcs (1.1.0): Loading from cache
  - Installing phpcompatibility/phpcompatibility-wp (2.0.0): Loading from cache
  - Installing wptrt/wpthemereview (dev-develop 50ba5f7): Cloning 50ba5f783d from cache
dealerdirect/phpcodesniffer-composer-installer suggests installing dealerdirect/qa-tools (All the PHP QA tools you'll need)
phpcompatibility/php-compatibility suggests installing roave/security-advisories (dev-master || Helps prevent installing dependencies with known security issues.)
phpcompatibility/phpcompatibility-paragonie suggests installing roave/security-advisories (dev-master || Helps prevent installing dependencies with known security issues.)
phpcompatibility/phpcompatibility-wp suggests installing roave/security-advisories (dev-master || Helps prevent installing dependencies with known security issues.)
Generating autoload files
PHP CodeSniffer Config installed_paths set to ../../phpcompatibility/php-compatibility/,../../phpcompatibility/phpcompatibility-paragonie/,../../wp-coding-standards/wpcs/,../../phpcompatibility/phpcompatibility-wp/,../../wptrt/wpthemereview/
jrfnl commented 5 years ago

Sorry for deleting all the issue template, but none of it was relevant to reporting a bug that is not a rule

No apology needed. Now the first version is close to release, we should probably add some additional issue templates.

For WPThemeReiew to work without the requirement to have WPCS or WPCS_DIR constants defined, of a .pathtowpcs file.

Absolutely. Thanks for finding and reporting this bug. PR #187 should fix this.

The use of the dealerdirect/phpcodesniffer-composer-installer registers and installs WPCS rulesset correctly and as such this registration should be used and not the above constants or dotfile.

Technically, this works slightly differently.

Yes, the Composer plugin registers the standards with PHPCS, however, PHPCS will only load any autoload files provided by a standard if the whole standard is used.

As WPTRTCS only uses select sniffs from WPCS, not any of the complete rulesets, WPTRTCS needs to load the WPCS autoload file, which is where the bug was.

ntwb commented 5 years ago

Absolutely. Thanks for finding and reporting this bug. PR #187 should fix this.

Thanks for the fix and quick turnaround @jrfnl 👍

Yes, the Composer plugin registers the standards with PHPCS, however, PHPCS will only load any autoload files provided by a standard if the whole standard is used.

Would including wp-coding-standards/wpcs in composer.json and adding <rule ref="WordPress-Core"/> and <rule ref="WordPress-Docs"/> in phpcs.xml.dist workaround that as they are then explicitally called?

jrfnl commented 5 years ago

Would including wp-coding-standards/wpcs in composer.json and adding and in phpcs.xml.dist workaround that as they are then explicitally called?

Changing the composer.json file would make no difference as WPCS will be installed anyway as it is a dependency of TRTCS.

Adding any of the WPCS rulesets to the phpcs.xml.dist would, as in that case, the ruleset is read out by PHPCS and PHPCS will load the autoload file based on that.

However, for TRTCS, that wouldn't be a solution as it shouldn't have an opinion on code style (which the WPCS rulesets do), so the PR I've pulled is a more permanent fix.

dingo-d commented 5 years ago

I had the same issue when I wanted to sniff the new twenty nineteen theme against this standard, but I thought that I did something wrong 😬

ntwb commented 5 years ago

@dingo-d, same, I discovered it whilst trying to add it to twenty nineteen 👍

jrfnl commented 5 years ago

I'm just glad it was discovered before the 0.1 release. I tested with quite a few different setups and at some point must not have realized I hadn't tested yet with project based, duh..