Incorrect relative paths for WPCS

@jrfnl

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.

@GaryJones

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.

I've pulled a fix for the upstream bug as squizlabs/PHP_CodeSniffer#1581

@GaryJones

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

@frenck

@frenck Thanks! Much appreciated.

@jrfnl

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.

PHPCS 3.0 allows pointing to the directory containing the ruleset.xml
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?

@GaryJones

@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.

@frenck

@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:

No subdirectories are found with ruleset.xml files and Sniffs subdirectories
And PHPCS 3.x is detected.

There are two reasons for this:

In the past - and not even that long ago -, it was recommended for a custom (project) ruleset to be called ruleset.xml. Since I believe PHPCS 2.2 (I don't remember the exact version), the recommendation is to call that file phpcs.xml. Your current methodology could easily confuse a custom project ruleset with a standards ruleset and register directories which aren't standards.
I've so far discovered ~8 bugs to do with the new autoloading in PHPCS 3.x and, to be honest, I don't think that will be the end of it. The top-level standards directory method to register installed_paths is tried & tested, the root-directory way not so much, so chances are there will be more bugs discovered related to this.
While using the root-directory way to register the standards will expose these bugs more quickly, that is not a task of this plugin and I'm afraid most end-users will just get annoyed and drop the use of this plugin and/or even stop using PHPCS because of it, which is counter to the intention of this plugin.

Explicitly recommend v0.3.x of the DealerDirect Composer plugin

@jrfnl

@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.

@frenck

@frenck Thank you! You're awesome!

Fixes #33. Changes the way the installed_paths are set.

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

Merge pull request #34 from DealerDirect/hotfix/change-installed-path…

@jrfnl

@jrfnl @GaryJones

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

Thanks guys for reporting, this is really appreciated.

I'll give it a try, thanks!

@GaryJones

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.

@frenck

@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 :-)

@frenck

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

Explicitly recommend v0.4.1+ of the DealerDirect Composer plugin

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.

@jrfnl

@jrfnl Thanks for letting me know 👍

Composer: make the version requirement for the Composer PHPCS plugin …

Navigation Menu

Repository navigation

Incorrect relative paths for WPCS #33

Problem/Motivation

Expected behaviour

Actual behaviour

2 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Incorrect relative paths for WPCS #33

Description

Problem/Motivation

Expected behaviour

Actual behaviour

Activity

jrfnl commented on Jul 28, 2017

jrfnl commented on Jul 29, 2017

frenck commented on Jul 31, 2017

jrfnl commented on Jul 31, 2017

frenck commented on Jul 31, 2017

frenck commented on Jul 31, 2017

jrfnl commented on Aug 1, 2017

frenck commented on Aug 1, 2017

jrfnl commented on Aug 1, 2017

2 remaining items

frenck commented on Aug 1, 2017

frenck commented on Aug 1, 2017

GaryJones commented on Aug 1, 2017

frenck commented on Aug 1, 2017

GaryJones commented on Aug 1, 2017

jrfnl commented on Aug 1, 2017

jrfnl commented on Aug 16, 2017

frenck commented on Aug 16, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions