FloeDesignTechnologies / phpcs-security-audit

phpcs-security-audit is a set of PHP_CodeSniffer rules that finds vulnerabilities and weaknesses related to security in PHP code
GNU General Public License v3.0
712 stars 85 forks source link

Fix compliance with PHPCS #50

Closed jrfnl closed 4 years ago

jrfnl commented 4 years ago

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:

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

Fixes #47 Fixes #38 Fixes #45

jmarcil commented 4 years ago

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!

jrfnl commented 4 years ago

@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. So, the choices I made are largely aimed at minimizing support requests and I've found that having people set the installed_paths themselves is a source of lots of support requests ;-)

jmarcil commented 4 years ago

Yeah my conclusion with installed_paths is that you don't need to play with it even if you run it from the cloned repo.

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.

jrfnl commented 4 years ago

@jmarcil Sounds good to me!