search

PHPCSStandards / composer-installer

Composer installer for PHP_CodeSniffer coding standards
https://packagist.org/packages/dealerdirect/phpcodesniffer-composer-installer
MIT License
549 stars 36 forks source link

Incorrect relative paths for WPCS #33

Closed GaryJones closed 7 years ago

GaryJones commented 7 years ago

Problem/Motivation

Possibly related to #32.

With the following in my composer:

"dealerdirect/phpcodesniffer-composer-installer": "^0.4",
"wimg/php-compatibility": "dev-feature/prevent-conflicts-with-other-standards",
"wp-coding-standards/wpcs": "dev-develop"

the installed_paths phpcs config value is wrong.

Composer resolves this as using PHPCS 3.0.2, and the relative branches of WPCS and PHPCompatibility, both of which support PHPCS 3.*.

Expected behaviour

I expect the paths to be correct. With 0.3 of this plugin. I get:

<?php
 $phpCodeSnifferConfig = array (
  'installed_paths' => '/Users/gary/Local Sites/incipio/app/public/wp-content/themes/incipio/vendor/wp-coding-standards/wpcs,/Users/gary/Local Sites/incipio/app/public/wp-content/themes/incipio/vendor/wimg/php-compatibility',
)
?>

phpcs -i shows all of the standards correctly available, and phpcs runs correctly against the phpcs.xml.dist in my project.

Actual behaviour

With 0.4 of the plugin, I get:

<?php
 $phpCodeSnifferConfig = array (
  'installed_paths' => '../../wp-coding-standards/wpcs/WordPress/,../../wp-coding-standards/wpcs/WordPress-Core/,../../wp-coding-standards/wpcs/WordPress-Docs/,../../wp-coding-standards/wpcs/WordPress-Extra/,../../wp-coding-standards/wpcs/WordPress-VIP/,../../wimg/php-compatibility/PHPCompatibility/',
)
?>

phpcs -i still does show the right standards are available, but, phpcs does not run correctly. Depending on the first called ruleset in phpcs.xml.dist, I get an error the form:

ERROR: Referenced sniff "WordPress" does not exist

This also happens if phpcs . --standard=WordPress is called.

Interestingly, calling phpcs . --standard=WordPress-Core does appear to run.

I'm not sure where the issue lies exactly, but @jrfnl says:

PHPCS 3.x is supposed to support standards as a top-level folder (as in how PHPCompatibility used to be layed out and how DealerDirect registered the paths now), but that looks to be buggy based on your findings.

I can run some tests with that and will report my finding to PHPCS, but basically as long as the installed_paths are registered in the PHPCS 2.x way (which is also still the recommendation for PHPCS 3.x), things should be fine.

jrfnl commented 7 years ago

What @GaryJones said.

Basically PHPCS expects the installed_paths which are registered to point to a standards directory like it has itself. This directory is expected to contain one or more subdirectories which each contain a ruleset.xml file and may contain sniffs in a Sniffs subdirectory underneath that.

As of PHPCS 3.x, PHPCS is supposed to support root-directory standards, i.e. passing a path to installed_paths which does not contain standards in subdirectories, but has the ruleset.xml in the root of the passed path and may contain sniffs - again - in a Sniffs subdirectory underneath that.

Based on Gary's findings that PHPCS 3 feature appears to be buggy.

As this plugin has registered the installed_paths for all the standards as if they were root-directory standards, that bug is playing up. I will investigate this and report that in a separate issue to PHPCS itself.

(Manually) Setting the installed_paths to the top-level directory, i.e. the one above the individual standards, shows that works fine and to be honest, that is the behaviour I would have expected from this plugin.

jrfnl commented 7 years ago

I've pulled a fix for the upstream bug as https://github.com/squizlabs/PHP_CodeSniffer/pull/1581

frenck commented 7 years ago

Hi @GaryJones & @jrfnl,

The issue indeed seems an error / unexpected behavior. I will look into this today and tomorrow trying to reproduce and fix what needs fixing.

../Frenck

jrfnl commented 7 years ago

@frenck Thanks! Much appreciated.

frenck commented 7 years ago

For v0.3.x we've used this when finding a path that contains a ruleset.xml file:

$standardsPath = dirname(dirname($ruleset))

In v0.4.0 we changed this behavior to support two things.

  1. PHPCS 3.0 allows pointing to the directory containing the ruleset.xml
  2. PHPCS 3.0 has support for having standards in the root of the repository.

The plugin detects if you are using PHPCS 2.x or 3.x and adjusts the behavior. For 2.x the plugin will use the directory above the directory containing the standard, for 3.x it will only use the directory containing the standard. There is one directory level difference here.

If I understand @jrfnl correctly, this is an issue with PHPCS itself, and a hotfix is made squizlabs/PHP_CodeSniffer#1581.

Therefore I'm wondering, how is this a problem with this plugin? It does not feel right to mitigate a bug in PHPCS...

The only option I see here is to revert partially to the PHPCS 2.x behavior, but this would/could/might cause problems when trying to support coding standards in the root of the repository. Nevertheless, this is again workaround-able, but it ain't pretty...

So actually, the actual behavior is my expected behavior... Since this is documented by PHPCS.

Any thoughts on this @jrfnl?

frenck commented 7 years ago

@GaryJones This issue is not related to #32.

32 is actually an issue with the repository that is used. It contains a ruleset.xml file instead of an phpcs.xml file, which causes troubles and is documented behavior in PHPCS. That issue is left open as an inspiration for improvements on our side (robustness) and not actually a bug.

jrfnl commented 7 years ago

@frenck Thank you for investigating.

I would expect the plugin to register paths using the cross-version compatible method of a "Standards" directory and only register standards as root-standard directories if:

There are two reasons for this:

frenck commented 7 years ago

@jrfnl I have created a working solution on my laptop at this moment. I've tested it extensively, including the case mentioned by @GaryJones.

I will do some documentation and some more (paranoia) testing and get back to you guys as soon as possible.

jrfnl commented 7 years ago

@frenck Thank you! You're awesome!

frenck commented 7 years ago

PR #34 should fix this issue, will do a minor release right after the merge.

frenck commented 7 years ago

@jrfnl @GaryJones

PR passed review and got merged. Created a minor release v0.4.1.

Thanks guys for reporting, this is really appreciated.

GaryJones commented 7 years ago

I'll give it a try, thanks!

frenck commented 7 years ago

Thx @GaryJones 👍

One of the last tests I've done is the one you've put into this issue (and even added additional standards in the root of my project).

Result:

PHP CodeSniffer Config installed_paths set to ../../../,../../wimg/php-compatibility/,../../wp-coding-standards/wpcs/

I was able to run every registered coding standard, including the one that gave you errors: Wordpress.

If you encounter any issues, please let me know. We will try to look into it asap.

GaryJones commented 7 years ago

@frenck 0.4.1 seems to work well. Having added another code standard (ObjectCalisethenics), I now correctly get CodeSniffer.conf as:

<?php
 $phpCodeSnifferConfig = array (
  'installed_paths' => '../../wp-coding-standards/wpcs/,../../object-calisthenics/phpcs-calisthenics-rules/src/,../../wimg/php-compatibility/',
)
?>

Thank you :-)

jrfnl commented 7 years ago

@frenck Thanks for this! I'll make sure the info in both WPCS and PHPCompatibility is adjusted where necessary before release.

jrfnl commented 7 years ago

FYI: a fix for the upstream part of this issue has been merged into PHPCS just now, so should be included in the PHPCS 3.1.0 release. That doesn't negate the value of the fix which was merged here, but just thought I'd let you know anyway.

frenck commented 7 years ago

@jrfnl Thanks for letting me know 👍