PSR-2 Compliant Sniffer

[bcrowe]

This PR aims to implement PSR-2 in its entirety, as well as preserving existing CakePHP rules that do not conflict with PSR-2.

After running php-cs-fixer on core, and then running phpcs with this updated sniffer -- it's largely docblock indentation fails... which PHP-CS-Fixer doesn't seem to fix. 😢

To Do's:

• Find a solution for the docblock indentations. Taken care of by @ceeram. (Add docblock indent fixer PHP-CS-Fixer/PHP-CS-Fixer#790) 👏
• Update "...fail.php" test files to include correct PSR-2 standards (outside of CakePHP standards) to give a more focused/accurate test of what it's actually evaluating.

I'm opening this as a "WIP" in case I've overlooked/removed any CakePHP rules, as well for looking for any docblock indentation insight anyone may have.

This PR's eventual merge will be released as 2.0 so that continuous integration between Cake 2 and 3 can live side by side.

[markstory]

If this is for cake3, this will never be true.

[bcrowe]

Thanks. 🙏

[bcrowe]

The latest phpcs output: https://gist.github.com/bcrowe/1a33b92a9b5ab0c9444f on src/.

Remaining errors outside of what php-cs-fixer solves:

• Multi-line function call errors.
• const docblocks being indented. (The Line indented incorrectly; expected at least 1 spaces, found 0 errors) (Mentioned this to @ceeram)
• Switch/case indentation errors. (Only a few.)
• The one block of Double space found errors, which is the result of converting the multi-sized tabs here: https://github.com/cakephp/cakephp/blob/3.0/src/Validation/Validation.php#L150-L169, to spaces. (Only this one case.)

Outside of a missing php-cs-fixer multi-line function call fixer, the important thing to note here is that the sniffer now has PSR-2 and CakePHP standards working side by side, are the errors are valid ones.

Would be nice to get this merged before worrying about a multi-line fixer for core, so that we can tag 1.0 and 2.0 for this repo, finish up the cakephp/migrations and cakephp/app PRs, and be available to get a start on any other non-BC-merging repos.

[ADmad]

@bcrowe It would be nice if you could also make required changes for the 2.0 branch to update the squizlabs/php_codesniffer dependency to 2.0.

[ADmad]

The FunctionCommentSniff is the one causing problem if using php_codesniffer 2.0, as it uses PHP_CodeSniffer_CommentParser_FunctionCommentParser which has been removed. We could try directly using squiz/FunctionCommentSniff or pear/FunctionCommentSniff directly or with modifications.

[bcrowe]

@ADmad The sniffer is now working with the phpcs 2.0 branch.

Random Good News: phpcbf (included with phpcs 2.0), now fixes all those extra things I've mentioned (outside of the "Double space found" errors). 👏 👏 👏

I've completely removed the FunctionCommentSniff, which I think is now irrelevant anymore based on what I saw within the copy-pasted Squiz -> Cake diffs.

I've also completely removed the FunctionCommentThrowTagSniff, in order to get where we presently are, which will need reimplemented to maintain our @throws coding standard.

Edit:
Almost forgot... phpcs hates us having multiple classes in one file in our tests. So, this explains the exclude-pattern additions to the ruleset. Potentially need to stub these out -- we can either knock the test stubs out now, or move on with the sniffer. Either way. ¯_(ツ)_/¯

nose goes

[ADmad]

I've completely removed the FunctionCommentSniff

So does PSR2 rulesset include sniffs which covers those sniffs removed or were new sniffs included to our set from Squiz or PEAR standarsds? If am I am pretty sure there is a net loss.

[bcrowe]

Earlier today, I took the Squiz and PEAR FunctionCommentSniff's and compared diffs against Cake's FunctionCommentSniff... and it was pretty much deletions (although, it was moreso muddied up by the 2.0 branch). It looked like Cake's FunctionCommentSniff was just circumventing some rules from Squiz/PEAR, which no longer apply to us, since we're adopting PSR-2, and PSR-2 doesn't have docblock rules. I'm not sure who initially moved and modified these rules over, but they could certainly provide some insight. If the initial author isn't available, I can certainly investigate further.

[ADmad]

since we're adopting PSR-2, and PSR-2 doesn't have docblock rules.

Just because PSR2 does not have rules for docblocks doesn't mean we can't have extra terseness to ensure proper doc blocks which are the basis of having proper API docs generated. Any user who doesn't like these extra checks can just PSR2 ruleset instead of CakPHP's.

I'm not sure who initially moved and modified these rules over..

I did. I copied the sniff from squizlabs and then modification / deletions were done because we didn't need some of the checks like ensuring the tag were descriptions aligned etc.

[bcrowe]

Just because PSR2 does not have rules for docblocks doesn't mean we can't have extra terseness to ensure proper doc blocks which are the basis of having proper API docs generated. Any user who doesn't like these extra checks can just PSR2 ruleset instead of CakPHP's.

Absolutely! I just didn't see any API doc generation losses at first glance. Let me look again, unless you've seen some losses already?

I did. I copied the sniff from squizlabs and then modification / deletions were done because we didn't need some of the checks like ensuring the tag were descriptions aligned etc.

So this looks okay?

[ADmad]

Absolutely! I just didn't see any API doc generation losses at first glance.

Currently all docblocks are already good so how would you :) Removing the FunctionCommentSniff without adding a replacement means inconsistencies will creep in in future.

So this looks okay?

Not for me until alternative for the removed comment sniff is added :)

[bcrowe]

Latest output: https://gist.github.com/bcrowe/03d25d5f0d8db4ddac92

• Copied Squiz's FunctionComment sniff into our own CakePHP FunctionComment sniff
  • Modified to allow our bool and int docblock types.
  • Remove "param/throws descriptions must begin with a capital letter and end with a period" rules.
  • Remove typehint suggestion errors for methods params based on the docblock param type.
  • Remove wacky param/type/name/description spacing rules.
  • New FunctionComment sniff includes checking for @throws existence, which makes up for the previous mention of FunctionCommentThrowTagSniff's removal.
• Added additional exclusions of rules. (To temporarily ignore our method name underscore prefixes ... and ... to ignore method name camelcase errors in the case of things like blahBlahPHP() and blahBlahSQL().)

[markstory]

That is looking like a manageable list. We clearly won't be done for the first RC, but by the second we stand a chance.

[bcrowe]

I think we can just nuke the Comment missing for @throws tag in function comment errors too. We do have a comment after @throws at http://book.cakephp.org/2.0/en/contributing/cakephp-coding-conventions.html#method-function-docblocks ... but it's not really like we explicitly require it or care (maybe we do care?). This accounts for 83 of the errors... heh.

Edit: Ya, this wasn't sniffed for nor enforced in 2.x regardless our our CS example... heh.

[bcrowe]

Latest output: https://gist.github.com/bcrowe/0731b74f02763ab9f812

I've removed all the "description/comment" requirements from @param and @throws doc annotations. While our coding standards doc page sets an example for these, historically we haven't had a sniffer for these or have strict enforcement of these anyways.

This brings us to a small amount of work to update some missing return related stuff.

It'll be best to start furnishing the @param and @throws with comments incrementally, and reintroduce the CS rules afterwards, as rushing to squeeze in descriptions/comments before the forthcoming RC releases will just end up in half-assed descriptions.

By the way, PSR-2 doesn't have docblock rules. So, this brings us to a PSR-2/CakePHP compliant sniffer, outside of the for-mentioned exlcusions for underscores and camel casing.

[ADmad]

Modified to allow our bool and int docblock types.

Allow or ensure? The sniffer should throw a warning if boolean is used instead of bool.

Remove "param/throws descriptions must begin with a capital letter and end with a period" rules.

I wish we could keep this but it would be a lot of work to update the docblocks 😄

I think we can just nuke the Comment missing for @throws tag in function comment errors too.

I would keep them. They are a bit painful only when throws statement is of type throw new $expClass but it can be got around by doing $exp = new $expClass(); throw $exp.

Also all docblock related sniffs should throw warnings, not errors (I made that change to exiting FunctionCommmentSniff). That way users can easily ignore them in their app using -n switch if they don't want to be too pedantic about them.

[ADmad]

@bcrowe All said it's ok if we can't get all docblock related sniffs to our liking at the get go. They can be added incrementally. Please just create issues for ones you remove/skip so that they are not forgotten.

[bcrowe]

Allow or ensure? The sniffer should throw a warning if boolean is used instead of bool

It now ensures. (cd1fbd7)

I wish we could keep this but it would be a lot of work to update the docblocks 😄

I would keep them. They are a bit painful only when throws statement is of type throw new $expClass but it can be got around by doing $exp = new $expClass(); throw $exp.

Also all docblock related sniffs should throw warnings, not errors (I made that change to exiting FunctionCommmentSniff). That way users can easily ignore them in their app using -n switch if they don't want to be too pedantic about them.

All of the nice-to-have and future incremental fixes have been reinstated and dropped down to warnings.

PHPCS Output (src/ and tests/):

• https://gist.github.com/bcrowe/ae580d82d92fa20e5e63 (with warnings suppressed)
• https://gist.github.com/bcrowe/6c95623d0b1d79f37cdb (including warnings)

I believe this brings us to PSR-2 compliance, outside of "should..." suggestions, and some eventual/future/nice-to-have home-brewed docblock rules.

[ADmad]

👏 The list of error with warnings suppressed seems quite manageable.

[bcrowe]

@markstory Looks like it's been 29 days since beta3, want to me to squeeze in the applicable "with warnings suppressed" changes on core before our RC1? Will obviously only take a few minutes or so tomorrow. Looks like all outstanding PRs to 3.0 are by people capable of rebasing.

[markstory]

@bcrowe Sounds good to me. I don't know if we'll do the RC1 right on schedule as the RulesChecker and Form work is not complete yet.

[ADmad]

Anything else that needs to be taken care of before we can tag current master 1.0 and merge this?

[markstory]

Nope. I can take care of the tagging.

[bravo-kernel]

Great (and interesting) job 👍

[markstory]

I've created a 1.x branch and a 1.0.0 tag for the existing codesniffer rules. The new PSR2 compliant sniffer (2.x) will now be on master.

[ADmad]

@bcrowe Great job 👏