search

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

Add initial CI check #60

Closed jrfnl closed 4 years ago

jrfnl commented 4 years ago

Related to #56, this adds an initial set of CI checks to be run via Travis.

To turn this check on:

I've tested this PR - see here for a passing build: https://travis-ci.org/jrfnl/phpcs-security-audit/builds/651998291

Rulesets: Add XSD schema tags (PHPCS 3.2+/3.3.2+)

As of PHPCS 3.2.0, PHPCS includes an XSD schema for rulesets which defines what can be used in the XML ruleset file.

In PHPCS 3.3.0 a new array format for properties was introduced and as of PHPCS 3.3.2, the new array format is now also covered by this schema.

This commit adds the schema tags to the ruleset.xml file and the example rulesets.

Rulesets: tidy up

Tidy up the XML rulesets so they can pass validation against the XSD as well as for XML code style.

QA: add Parallel-Lint for easy syntax checking of the repo

Includes adding a composer lint convenience script to run the command.

The script as-is, is platform independent and will work on both Linux, Mac as well as Windows and respects the PHP version used by Composer.

Travis: add initial CI check

jmarcil commented 4 years ago

Great stuff.

But I'm afraid I have to ask this: what is the expected maintenance that a sole maintainer like me that can work on this project once every 6 months should expect from this integration?

Is it well recognized to never break and require no attention once it's setup? Or you've seen in the past that sometimes efforts needs to be done.

Right now I'm worry about adding complexity in places that provides no values for the users of this tool. This is just another thing for me to support and I'd prefer to put my time on things that the community has requested such as:

Let me know, worse case I think I'll icebox this PR and work on those things first, but if you say it's fire and forget then I'll just merge and move on.

jrfnl commented 4 years ago

Short answer: what I've done here is fire & forget, though the Travis integration does need to be turned on (one time action).

But I'm afraid I have to ask this: what is the expected maintenance that a sole maintainer like me that can work on this project once every 6 months should expect from this integration?

For the things in this PR, hardly anything, if anything at all.

The checks I've implemented here are more than anything to prevent incorrect code/inconsistencies being committed, so prevent issues from entering the repo and having to clean them up afterwards. (i.e. it should save you work, not give you work)

The only thing which may need an update every so often is the versions of the packages added in require-dev in the composer.json file, but as there is no committed composer.lock file (:heavy_check_mark:), that should only be needed on new major releases of the packages anyway.

Is it well recognized to never break and require no attention once it's setup? Or you've seen in the past that sometimes efforts needs to be done.

Well, tooling changes sometimes over time and new versions of things get released.

So small tweaks may be needed once in a while to keep up with the times - think: adding PHP 8 to the matrix once released -, but most of those are just maintenance and not something which would break the build if not done.

Right now I'm worry about adding complexity in places that provides no values for the users of this tool.

Except that it does add value. Having CI checks in place, prevent issues from entering the repo and getting to end-users. For example: with the checks in place now, an accidental parse error in a sniff will cause the build to fail, instead of it being merged into the repo silently and nobody noticing it until a bug report comes in.

Let me know, worse case I think I'll icebox this PR and work on those things first, but if you say it's fire and forget then I'll just merge and move on.

I'd suggest merge & move on.

CI checks like this are intended to take work away from you by automating things you'd otherwise would need to check manually.

So, in a next step, we could add automated testing, like discussed elsewhere. That way for any PR, you know that it won't negatively affect existing checks without having to do a lot of testing for that yourself.

Same, having automated tests in place should also help with the "reduce false positives" project.

This is just a first set up and it can be expanded with additional checks as described in #56 to make the project easier to maintain in the long run.

jmarcil commented 4 years ago

Sold!

I'd like to point out that the website is not Travis.com like mentioned above but rather travis-ci.org.

Also, I'm not happy about the fact that by default I have to authorize travis to all my repos to just log in.

jrfnl commented 4 years ago

I'd like to point out that the website is not Travis.com like mentioned above but rather travis-ci.org.

My bad. Quick replies while traveling and detail precision don't always go well together ;-)