Proposal: Alter our PHP coding standards

Unknown · July 2018

Our current coding standard is largely unused.

It seems that:

No one was running it locally (in PHPStorm or from the command line)
Our installation docs don't mention the years old version of CodeSniffer that is actually required to run our Sniffs and say to install it with PEAR. I've updated the docs for this
We don't run it CI Issue here

All of these things in mind there was nothing actually validating our new code. This isn't exactly the path towards enforcing a coding standard.

Part of the reason no one was running it locally besides the installation instructions was that the standards were way to strict in certain areas. Old code had so many violations that it seemed impossible to even work with it on.

Analysis

I've including a log of the phpcs issues with our DiscussionController:

1 | ERROR   | There must be exactly one blank line before the
  |         | first param tag in a doc comment
1 | ERROR   | There must be a single blank line after the last
  |         | param tag

90% of these have to do with comment validation
A couple are for variable naming
A couple are for line length (More than 150 characters?)
There are none for the various unused variables
There are none for dynamically created class properties

In summary all of our warnings here are for cosmetic fixes (that aren't actually inline with our coding standard), but there is nothing picking up the potential bugs.

Fixes

I'm proposing a few changes the standard to relax this.

Comments

Our standard says:

Descriptions MUST BE a full sentence with a capital to start and period to end

Short, return and parameter descriptions MUST be included if present

There MUST be one space before and after parameters

Currently our CodeSniffer setup requires that in addition to this:

The first line of the description can only be 1 line.
A long description MAY come after
Param annotations are REQUIRED for every parameter
A return annotation is REQUIRED no matter what.
There must be empty comment lines between [short description, long description, params, return].
You are not allowed to use the @see tag in a class comment. What is the purpose of this?

Removing validation of all of these things that are not outlined in our standard would remove a lot of our CodeSniffer validation errors, because almost none of our code does these things. We do generate documentation from our comments, so param annotations are sometimes helpful but not always necessary, and info about returns is generally available in the method name.

Additionally:

An @inheritdoc statement should void all other validation of the comment.

Line length

From our standard

There MUST NOT be a hard limit on line length; the soft limit MUST be 120 characters; lines SHOULD be 80 characters or less.

I actually am having trouble parsing this, but I guess this means we shouldn't actually validate line length?

Either way what we currently enforce (150 characters) in code sniffer is ridiculous.

Missing validation

A bit of missing validation that would go a long way:

Warnings for high cyclomatic complexity in a function
Warnings for excessive method length
Warnings for excessive amounts of parameters (more than 5 is lot!)
Unused private fields, local variables, and private methods.
Trailing commas in multiline arrays!!!

There are built in code sniffer sniffs for all of these.

<rule ref="rulesets/codesize.xml/CyclomaticComplexity"/>
<rule ref="rulesets/codesize.xml/ExcessiveMethodLength"/>
<rule ref="rulesets/codesize.xml/ExcessiveParameterList"/>
<rule ref="rulesets/unusedcode.xml/UnusedPrivateField"/>
<rule ref="rulesets/unusedcode.xml/UnusedLocalVariable"/>
<rule ref="rulesets/unusedcode.xml/UnusedPrivateMethod"/>

Unknown · July 2018

I like your suggestions and trying the code quality warnings. The one I really want is a fix for the return type-hint. Right now it complains about the colon after the closing bracket. Did you see anything like that?

As an FYI: We only have our own standard because we want lord brackos brackets. Otherwise we are PSR-2. If we could easily use off-the-shelf rules for that we'd be able to update phpcs versions more easily.

Unknown · July 2018

So I've merged a PR from @Ryan that brings us up to the latest version of PHPCS. If you pull master from vanilla/standards and get the latest version of PHPCS from composer (3.3 I think?) that should take care of it.

So a rough outline of a quick overhaul so that we can actually get on our standard:

[x] Update to phpcs 3.x
[x] Fix our comment sniffs
[ ] Fix the line length validation
[ ] Try enabling some code quality sniffs
[ ] Publish our standard to packagist
[ ] Make phpcs and our standard dev dependencies of vanilla/vanilla
[ ] Put together a script that lints only the changed files from a pull request.
[ ] Run this script in travis

Unknown · August 2018

WRT the line length validation. I just want to clarify that since it was your opinion that it was too long where I thought you were thinking it was too short. I think it means it's fine right now?

Proposal: Alter our PHP coding standards

Analysis

Fixes

Comments

Line length

Missing validation

Comments

Categories