Fix compliance with PHPCS #50
Conversation
| ./vendor/bin/phpcs --standard=example_base_ruleset.xml tests.php | ||
| ``` | ||
|
|
||
| The package is also on [Packagist](https://packagist.org/packages/pheromone/phpcs-security-audit): |
There was a problem hiding this comment.
what is the reason not to link to packagist anymore?
There was a problem hiding this comment.
Well, the original install instructions were based on git clone with a Composer based install being mentioned as the alternative, which is were the link came in.
This has now been reversed, with the regular install instructions now being to use Composer and the alternative being to use git clone.
As the install instructions now mention doing a composer require for the package:
- There is no real reason to have an explicit link to Packagist anymore.
- The fact that the package is on Packagist is implied.
- No typical text left over which to link to Packagist.
Happy to bring it back, but we'd need to think of some text to link to it then.
There was a problem hiding this comment.
Yeah I might need the link to remind myself this is where the release goes 😅. I remember having to do something on it a few years when we changed from pheromone to FloeDesignTechnologies.
There was a problem hiding this comment.
Well, if it's just for you, then there's no reason to bring the link back: if you go to the Packagist website and login, there is a My Packages link in the drop-down under your user name.
Also: https://packagist.org/users/jmarcil/packages/
There was a problem hiding this comment.
I'm afraid you overestimate my abilities to remember something every 5 years ;-)
I won't even remember packagist is a thing in two weeks.
There was a problem hiding this comment.
grin What about adding some badges to the top of the README with project status information ? Those (in part) would then link to Packagist.
PHPCS has specific naming and directory layout requirements for external standards which the `Security` standard did not comply with.
While things sort of worked with the symlink hack, the net effect was:
- The PHPCS autoloader did not work.
- None of the PHPCS ruleset configuration options worked as PHPCS could not match sniffs to files.
- Some sniffs would never load.
This PR fixes this by:
1. Setting the base namespace to `PHPCS_SecurityAudit\Security` and annotating this in the `ruleset.xml` file in the correct manner.
2. Fixing all namespaces and uses thereof throughout the codebase.
3. Fixing the `Drupal8/Utils.php` file which was missing the namespace and was still referring to an out-of-date class name to extend.
4. Fixing the namespace names and file names of the CVE sniffs.
- The namespace of a sniff has to reflect its path in the standard.
- The file name has to reflect the name of the sniff.
5. Fixing the names of the CVE sniffs in the example rulesets
6. Removing the symlink file and all references to it.
Instead `require` the [DealerDirect Composer PHPCS plugin](https://github.com/Dealerdirect/phpcodesniffer-composer-installer) which will sort out the `installed_paths` for PHPCS .
7. Setting the minimum PHPCS version to `3.0.2` as prior to that external standards weren't fully supported in the 3.x branch.
8. Removing the `autoload` section in `composer.json`. This is no longer needed and in certain situations can cause conflicts/fatal errors.
References:
* https://github.com/squizlabs/PHP_CodeSniffer/wiki/Coding-Standard-Tutorial
* https://github.com/squizlabs/PHP_CodeSniffer/wiki/Version-3.0-Upgrade-Guide
* squizlabs/PHP_CodeSniffer#2481 (comment)
* squizlabs/PHP_CodeSniffer#2606
* squizlabs/PHP_CodeSniffer#1469
* https://github.com/Dealerdirect/phpcodesniffer-composer-installer
Fixes 47
jrfnl
force-pushed
the
feature/fix-compatibility-with-phpcs
branch
from
03a37e1 to
c36e8c6
Compare
| ./vendor/bin/phpcs --standard=./vendor/pheromone/phpcs-security-audit/example_base_ruleset.xml ./vendor/pheromone/phpcs-security-audit/tests.php | ||
| ``` | ||
|
|
||
| This will also install the [DealerDirect Composer PHPCS plugin](https://github.com/Dealerdirect/phpcodesniffer-composer-installer/) which will register the `Security` standard with PHP_CodeSniffer. | ||
|
|
||
| It is also possible to install this based on a git clone. In that case, you will need to [register the package with PHP_CodeSniffer](https://github.com/squizlabs/PHP_CodeSniffer/wiki/Configuration-Options#setting-the-installed-standard-paths) yourself. |
There was a problem hiding this comment.
this is probably just for me (or other lazy contributors), but I would like to have the steps on how to install from a git clone directly in the readme like it was before.
There was a problem hiding this comment.
Happy to bring them back in improved form, however, I wonder if that wouldn't be better placed in a CONTRIBUTING.md file rather than in the readme ?
There was a problem hiding this comment.
Probably, but for now let's keep it simple. Note also that I have 10 years of using the tool and other people have done as such, so I want to keep the documentation (and the way it is) without breaking them / preventing them to use the tool -- even if it's not the current best practices.
Back the the current line, I don't really need to do phpcs --config-set installed_paths if I just composer install in the cloned folder. It just works because the default is to point it to the current directly. So I have:
git clone
composer install
./vendor/bin/phpcs --standard=example_base_ruleset.xml --extensions=php tests.php
seems good?
There was a problem hiding this comment.
haha that's literally the same thing as before... but I'm afraid I have it wrong because when I first tried it it didn't work for me.. perhaps I had composer.lock and vendor/ already there when I did.
|
Overall this PR is great! Now I think phpcs-security-audit will more fit into regular installations and will make it easier to people to use it. The only thing that seems to be an issue for myself is around the documentation. I'll be frank there, I'm used to maintain this tool alone for 10 years now, so I consider my own personal needs around the documentation important. It needs to be self-contained, quick to clone and work on. I'm sure this will change overtime as more contributor gets in, but for now please take my point of view in consideration on the why I might decide to things in a certain way. That doesn't mean your ways are wrong, they are actually better for end users. It just means I don't want to loose what I had created to make this possible for me to work on once or twice a year. So with that in mind, I'll go with a contributing section that gives me what I need (the git clone stuff mainly and the link the packagist) while keeping your changes intact. Thank you very much for your contributions on this project! |
|
@jmarcil No worries. It's not that I want to "force you into the modern age" or anything. I have based most of the choices regarding the documentation on years of experience maintaining external standards, like PHPCompatibility and WordPressCS, with especially WPCS users not being all that tech-savvy. |
|
Yeah my conclusion with BTW I did made a mistake.. I was supposed to create a branch for those changes in order to be able to keep master intact until we're ready with a 3.0 release. Since I don't have much time to spend on this, and right now the readme doesn't work since the package is not released.. I think I'll just create a 3.0 release anyways and we'll work from there. |
|
@jmarcil Sounds good to me! |
PHPCS has specific naming and directory layout requirements for external standards which the
Securitystandard did not comply with.While things sort of worked with the symlink hack, the net effect was:
This PR fixes this by:
PHPCS_SecurityAudit\Securityand annotating this in theruleset.xmlfile in the correct manner.Drupal8/Utils.phpfile which was missing the namespace and was still referring to an out-of-date class name to extend.Instead
requirethe DealerDirect Composer PHPCS plugin which will sort out theinstalled_pathsfor PHPCS .3.0.2as prior to that external standards weren't fully supported in the 3.x branch.autoloadsection incomposer.json. This is no longer needed and in certain situations can cause conflicts/fatal errors.References:
Fixes #47
Fixes #38
Fixes #45