More comprehensive standard

[ghost]

Our custom Backdrop standard currently imports certain sniffs from other standards, as well as adding our own sniffs. However I believe a more comprehensive solution would be to include the full, third-party standards, and then exclude certain sniffs that don't meet our needs.

To copy my explanation from #8:

I [...] decided to try building a standard from scratch to see what sniffs we needed and which we didn't. Here's what I've spent some time over the last few weeks doing:

• Setup a new, blank standard that includes all Generic and Squiz sniffs
• Run that over Backdrop core
• For each issue flagged that was a legitimate issue with our code, I noted that sniff/message as a good one
• For each issue flagged that isn't a problem with our code, I excluded that sniff/message (i.e. it's a bad one)

This eventually helped me to come up with a list of good and bad sniffs.

Of the ~119 sniff messages in the Generic standard, I excluded ~55 bad ones.
Of the ~323 sniff messages in the Squiz standard (that weren't already included in the Generic standard), I excluded ~92 bad ones.

So as you can see, it seems to be better to include the full Generic and Squiz standards, [than to] exclude specific sniffs/messages that don't match our coding standards (rather than just including specific sniffs as we do now).

[indigoxela]

A quick first check ... we might need to exclude some more rules. Some examples:

Array with multiple values cannot be declared on a single line (Squiz.Arrays.ArrayDeclaration.SingleLineNotAllowed)
Multi-line array contains a single value; use single-line array instead (Squiz.Arrays.ArrayDeclaration.MultiLineNotAllowed)
Missing @return tag in function comment (Squiz.Commenting.FunctionComment.MissingReturn)
Comment refers to a FIXME task...
Blank line found at start of control structure...

And false positives with very Squiz-y rules like:

Expected "Squiz Pty Ltd <[email protected]>" for author tag...

So, I ran the new setup on a module I maintain, which causes no errors with current ruleset (1.0.0-beta1).

A TOTAL OF 1139 ERRORS ... WERE FOUND IN 70 FILES

Whoah, that's a big, big change in behavior. Not sure, if we should do that.

[ghost]

A quick first check ... we might need to exclude some more rules.

Yes. I ran it myself and found some more false positives. Not sure why they didn't come up before... I'll update the PR.

And false positives with very Squiz-y rules like...

Not sure what you mean by this. What sniff/message was this referring to?

Whoah, that's a big, big change in behavior. Not sure, if we should do that.

I'm not sure I understand your reasoning here... If there are too many issues in code then we shouldn't add those checks to this repo? I thought this repo was meant to ensure compliance with our coding standards. If code isn't compliant, that's no reason not to include these checks, right? Yes I understand we don't want false positives (we'll fix those by excluding the relevant sniffs), but if the remaining sniffs are legitimate and end up flagging a bunch of issues, that's a good thing.

[indigoxela]

My reasoning here:

We should avoid to introduce such huge changes in behaviour in a "minor" update.

At first I started with a ruleset that's very similar to what the ruleset currently included in coder_review (for an outdated phpcs version) provides. To later add more strict restrictions step by step.
A change from zero errors to more than 1k in existing code isn't a small step at all.

Many of the nagging with the current PR isn't part of our currently documented coding standards at all. If we suggest to switch from a beta1 to a (for example) beta2 ruleset, the difference should be manageable in real usage.

However, I don't object here, I just wouldn't recommend that change size.