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

Replace deprecated Sensiolabs security checker #131

Closed Potherca closed 3 years ago

Potherca commented 3 years ago

As mentioned in #130, the Sensiolabs security checker has been deprecated and needs to be replaced by an alternative.

Steps to take:

  1. [x] Create a longlist of potential replacement candidates
  2. [x] Define selection criteria
  3. [x] Reduce the longlist to a shortlist based on criteria
  4. [x] Decide on a final replacement

Longlist

Replacement criteria

Shortlist

Out of all the suggested candidates, only the Enlightn security-checker remains, as it is the only one that meets all 6 current criteria. (Pending other candidates or criteria).

Final choice

🔜 @TODO

Potherca commented 3 years ago

@mjrider and @jrfnl Could you chime in here? I want all involved to be heard but I'd like to reach a conclusion with due speed on this.

jrfnl commented 3 years ago

@Potherca

Re: the criteria - the versioning criteria seems arbitrary as for security tools always using the latest info is kind of relevant.

Re: the shortlist: for documentation purposes it might be an idea to show a checklist for each of the tools on the long list and which criteria they met/didn't meet. Possibly with a short additional paragraph with the reasoning to (not) prefer the tool ?

Potherca commented 3 years ago

versioning criteria seems arbitrary

It's more about when the code changes, independent of the DB/list it uses. Requiring dev-master is an anti-pattern I'd rather avoid.

for documentation purposes it might be an idea to show a checklist [..]

:+1: I'll update the issue.

jrfnl commented 3 years ago

@Potherca #130 has been merged, but the arguments for using that solution over the other solutions were never added to this ticket... (which is why I didn't merge it before).

Potherca commented 3 years ago

This issue got closed when I merged the accompanying MR, I'll re-open it as I do still intend to update it with more info.

jrfnl commented 3 years ago

Okay, so I've started working on moving the rest of the CI from Travis to GH Actions and the first thing I run into straight away is that this new dependency conflicts with our supported PHP versions, so unless we want to jump through hoops to use it, I would strongly advocate replacing the enlightn/security-checker with a tool which actually is compatible with the PHP requirements of this project.

jrfnl commented 3 years ago

PR #137 will remove the enlightn/security-checker (which has a minimum PHP requirement of PHP 5.6, while this projects PHP requirement is >= 5.3) and replace it with the local-php-security-checker.

From the commit message:

Regarding the choice for this tool:

  • This tool will run independently of the PHP version used, making it more versatile.
  • It complies with most other criteria as set forth in #131 (not deprecated, lightweight, versioned, easy to install/update).
  • As for running it locally - either devs can download the tool and run it locally or they can use the tooling available to run GH actions locally, so IMO that part is covered too.

If anyone has any objections or concerns about this, please speak up.

Potherca commented 3 years ago

Closing this as #137 has been merged.