Code Sniffer Rules #182

jim-parry · 2016-07-20T16:07:35Z

Looking for someone familiar with PHP_CodeSniffer to create a ruleset matching our style guide.
This is initially intended for use inside an IDE, and ultimately perhaps as part of our travis-ci build.

louisl · 2017-06-22T22:12:58Z

Hi,

I've previously built sniffers for CodeIgniter 2 and 3 in PHP_CodeSniffer 2 which included phpcbf functionality, I've been using these pretty much daily in SublimeText as linters and fixing up old code for a few years.

I never announced these publicly as I included a bunch of personal preferences and best practices which were being argued about at the time, these were mostly commenting, spacing alignment, string concatenation to name a few.

[deprecated link removed]

I'm currently in the process of converting these to PHP_CodeSniffer 3 and I have a basic working CI 4 version.

You can't make these with just a ruleset.xml as some CodeIgniter style guidelines are not available anywhere within the existing PHP_CodeSniffer sniffs so I have some sniffs created purely for CodeIgniter 2, 3 and 4.

I have some questions about interpreting the new style guide and I guess it could change a bit if needed?, how would you like me to proceed?, should I ask here or would you like to start a thread on the forum?, my username there is also 'louisl'.

Regards

Louis

lonnieezell · 2017-06-23T03:02:50Z

Hey Louis, that would be awesome! I started to look at it a week or so ago, realizing I would need to write sniffs for it and put that aside, since I don't have any experience with that :)

Post your questions here, that's fine. If we open it up on the forums we get too many opinions lol. Or you could simply shoot me an email and we could discuss it there.

louisl · 2017-06-23T10:18:04Z

OK, lets get the ball rolling.
https://bcit-ci.github.io/CodeIgniter4/contributing/styleguide.html

The example titled 'Good control structure examples:'

if ( $foo )
{
    $bar += $baz;
}
else
{
    $baz = 'bar';
}

conflicts with:
Other

Opening parentheses SHOULD NOT be followed by a space character.
Closing parentheses SHOULD NOT be preceeded by a space character. (1 'e' in preceded BTW)

So we want this right?

if ($foo)
{
    $bar += $baz;
}
else
{
    $baz = 'bar';
}

lonnieezell · 2017-06-23T13:11:04Z

Good catch. You're correct. There should not be spaces inside the parentheses.

louisl · 2017-06-23T16:26:35Z

I've run CodeIgniter4 through and it's showing up a ton of inconsistencies... Lots of space indenting, tab aligning, wrong indenting, mixed tab and space indenting, non visibility declared functions, spaces missing around operators, upper case keywords, missing newline at end of file, OR, AND !=, ==, if(,

} else
{

to name a few.

Thats good though right?, proof we're getting somewhere! Most of it's fixable with the phpcbf.

OK some questions...

It's not mentioned in the current style guide, but am I ok to require === and !== (good practice). There's a mixture in current CodeIgniter4.

Quote marks for strings. - I think we should to pick one or another, my preference is single quotes unless containing a single quote then I use double quotes. Current CodeIgniter4 is mostly using single quotes. (Already have the sniff with fixer made for this too).

String concatenation - guidelines are suggesting no space around ., however spaces around operators and such are the norm elsewhere in the guide. Current CodeIgniter4 has a mixture of both. (I vote spaces around but either is fine).

lonnieezell · 2017-06-23T16:39:21Z

And that's exactly why we need this! :)

I'm fine with requiring strict comparisons. The only issue I can think of is if you want a possible type conversion (like testing for truthiness) and we can fix that by being a little more specific, I suppose. Not sure if there's anywhere in the framework that would affect, though.
Quote marks. I've always preferred single in the past. According to stats from Blackfire, string concatenation is actually faster within double-quotes now, so double-quotes makes sense from that micro-optimization level. :) But since my habit is single quotes and I'm the primary, I guess we go with that. It won't automatically change those, I assume.
Concatenation. You're right it says no spaces. My habit before working on this was more complex. no space around the string portion, space between variables, but we can't make a rule for that I"m sure lol. Not sure on this. I'd be tempted to leave as is, but I have no specific reason for that. @jim-parry what are your thoughts on this?

jim-parry · 2017-06-23T16:47:27Z

I have always used spaces around concatenation operators, in both PHP and Java. It makes more sense to me.

lonnieezell · 2017-06-23T17:00:15Z

Then let's go with a space around it to maintain consistency.

louisl · 2017-06-29T01:35:39Z

Preview version ;)

https://github.com/louisl/CodeIgniter4-Standard

Have a few issues to sort out but it's linting well and fixing most errors, although there's currently a problem with PHP_Codesniffer 3.0.1 phpcbf not saving files.

louisl · 2017-06-29T03:23:10Z

Couple of updates, grab the latest develop branch.

It's now checking snake case functions (out of scope ie helpers).
Fixed an error with file comment missing being flagged when it wasn't missing.

TODO

Php tags (allowing '<?=') is this just going to be for views?
View syntax checks
Vertical function spacing.
Eliminate blank lines after '{' opener.
Composer install.

The core of it's done. I've taken a mix of CodeIgniter 3, CodeIgniter 4 and PSR2 guides, consistencies in the CodeIgniter 4 code and some general good practices to shape it so far, so let me know what you think.

louisl · 2017-06-30T02:37:36Z

OK, it's been added to composer and install instructions have been done.

lonnieezell · 2017-06-30T02:46:22Z

It's looking pretty good so far! Thanks so much for doing this. The only that stood out to me is you seem to have both lowerCaseConstants and UpperCaseConstant rules listed in ruleset.xml?

louisl · 2017-06-30T08:27:21Z

No problem,

Generic.PHP.LowerCaseConstant Checks TRUE, FALSE, NULL are lower case.
Generic.NamingConventions.UpperCaseConstantName Checks user defined constants are upper case.

lonnieezell · 2017-06-30T13:19:10Z

Oh, that makes sense, then. Great.

louisl · 2017-06-30T22:27:14Z

The 'system' contains a mix of underscore prefixed private and protected methods and properties and non underscore private and protected.

Whats the preference?, we can exclude methods from the rules if needed, eg public _remap() .

louisl · 2017-07-03T12:26:30Z

1.0.0-beta0005 released.
https://github.com/louisl/CodeIgniter4-Standard

Allows use of short echo <?=.
No longer warns or fixes scope opener ':' to be on a new line for alternate syntax (used in views) eg. <?php if (! is_array($value) && ! is_object($value)) : ?>.
Added descriptive text to ruleset.xml.
Warns and fixes blank lines after '{' scope opener.
Improved whitespace checks and fixes.
Checks and fixes control structure spacing with type hints.

Still waiting for squizlabs to release PHP_Codesniffer 3.0.2 to use the beautifier (phpcbf), however you can patch the file saving error in 3.0.1 easily for now. squizlabs/PHP_CodeSniffer#1526

louisl · 2017-07-06T09:09:16Z

Hi Guys,

Could do with an answer on the private/protected methods and properties requiring underscore prefix or not?

This analysis may help to make some decisions Analysis of Coding Conventions

lonnieezell · 2017-07-07T21:05:26Z

Sorry for the delay. As far as underscores, typically I wouldn't use them for private methods. As noted, the _remap method exists mainly because of backward compatibility. And a few of the libraries that support handler classes have them, but they could be renamed. So - as far as I know, only the _remap would really be kept.

louisl · 2017-07-13T02:56:52Z

1.0.0-beta0006 released.
https://github.com/louisl/CodeIgniter4-Standard

Removed underscore prefixes for private & protected.
Improved commenting sniffs.
Multiple statements must now have their "=" aligned.
Removed some discouraged functions.
Improved descriptions in ruleset.xml.
Fixed various typo's.
Refactored some sniffs.
Requires short array syntax "[]" instead of "array()".
Type hints in functions & methods must be used and match their comment type.

louisl · 2017-07-13T08:31:31Z

May I suggest we change the file doc block copyright tag to:

 * @copyright  2014-2017 British Columbia Institute of Technology (http://bcit.ca/)

Because

That's how it's suggested at phpDocumentor.
To save me from a preg_match headache!

louisl · 2017-07-13T12:05:45Z

1.0.0-beta0007 released.
https://github.com/louisl/CodeIgniter4-Standard

Fixed issues with class comment tags.

lonnieezell · 2017-07-13T12:31:44Z

To save me from a preg_match headache!

LOL. I have no problems with that. @jim-parry ?

jim-parry · 2017-07-13T14:19:46Z

I am fine with the doc block copyright change. That should be a bulk change that either you or I push directly, rather than going through a PR. Did you want to do that or shall I? @lonnieezell

lonnieezell · 2017-07-13T16:35:47Z

You can hit it if you'd like, @jim-parry

jim-parry · 2017-07-13T17:24:15Z

Copyright notices changed, per above.

louisl · 2017-07-13T17:57:42Z

Copyright notices changed, per above.

Thank you.

louisl · 2017-07-13T20:35:33Z

Sorry to be a pain but it shouldn't have the comma after '2017'.

jim-parry · 2017-07-13T21:22:53Z

phpDocumentor says "The format of the description is governed by the coding standard of each individual project."
The examples, however, do not show the comma, so I have removed it :-/

louisl · 2017-07-13T23:06:09Z

The examples, however, do not show the comma, so I have removed it :-/

Thank you for indulging me.

OK this is not high priority right now but it's worth thinking about.

There is this rule in the guidelines,
Files declaring functions SHOULD be named in snake_case.php.

Can I get some clarification on the interpretation of it?
It's obvious this covers xyz_helper.php files, but are there any other cases where you want the files snake case. I'm asking because I need a consistent way of identifying those files. I can do things like:

if filename contains _helper.php etc.
if the file contains or does not contain any function, use, class, interface etc. (Most things have a unique identifier token so it's not a string search & compare).

jim-parry · 2017-07-13T23:08:20Z

Users can build their own helpers, inside application/Helpers. I would expect those to use snake_case, like ours inside system/Helpers do.

jim-parry · 2017-07-13T23:12:48Z

That is a should rule, not engraved in stone. Is it something that should be enforced?
Is it reasonable to insist that any helpers (*_helper and/or inside a Helpers folder) be snake_case?

louisl · 2017-07-13T23:22:47Z

Well if someone were to drop My_big_Bunch_of_Functions.php in application/helpers would it load with helper('my_big_bunch_of_functions'); or would they have to get the case exactly right for file systems that are case sensitive?

jim-parry · 2017-07-13T23:29:15Z

If something (BananaPie) is intended to hold helper functions, CI will look for the snake_case filename (bananapie_helper.php)

louisl · 2017-07-13T23:40:48Z

So it would be safest to enforce *[lowercase]_helper.php in application/helpers and system/helpers then? I guess if someone needed to use something from ThirdParty or elsewhere they could just 'require' it in a helper they create rather than drop junk filenames in application/helpers.

jim-parry · 2017-07-13T23:47:52Z

or, could we enforce that any filename ending with "_helper" be snake_case, and contain only function declarations and no classes?

louisl · 2017-07-14T00:00:03Z

OK, I'm happy to go with that.

louisl · 2017-07-19T11:40:27Z

Currently working on array sniffs and fixes, almost got it. I want to get that right before I release the next beta version.

louisl · 2017-07-21T13:04:47Z

1.0.0-beta0008 released.
https://github.com/louisl/CodeIgniter4-Standard

Array formatting and fixing.
Disallow tabs in alignment.
*_helper.php functions filename check.
*_helper.php files must only contain functions.
Ignores 'application/Views/', 'application/ThirdParty/' and 'system/ThirdParty/'.
Improved comment sniffs.
Updated allowed multi class files.
Added some unit tests.
Added unit test instructions.
Now requires PHP with mbstring extension due to language arrays in the core containing multibyte characters.
Now requires PHP_Codesniffer 3.0.2 or greater.

louisl · 2017-07-25T14:03:35Z

1.0.0-beta0009 released.
https://github.com/louisl/CodeIgniter4-Standard

Fixed issues with some array declarations not being caught.
Catches excessive vertical empty lines.
1 empty line expected after file doc comment.
Updated unit tests.
Additional installation instructions.

This is pretty solid now, from here on I want feedback, changes, feature requests, bugs and issues.

I'll be adding some more unit tests and optimising but I'm not going to change any more functionality now without instruction.

@jim-parry If you want me to hand the repo over so it's officially in bcit-ci github, I'm happy to do that. Also I don't mind if you change all the header copyrights if/when we do ;)

Happy coding

Let me know how you get on with it!

lonnieezell · 2017-07-26T02:49:18Z

This is all looking great. @louisl Thanks so much!

I can't speak for @jim-parry, but I think it would be great as an official package. I'd like to see more official offshoot packages, but that's a discussion for another day. :) Jim's heading for vacation for few weeks, or has just left, so it might be a bit before he can get an answer.

In the meantime, I'll start trying to use it on the CI repo and see what changes, if any, we discover.

jim-parry · 2017-07-26T04:58:14Z

I haven't left yet, but I'll be off in a couple of days, and don't think I will have a chance to check this out :(
Thank you @louisl and I would be happy to welcome this into the CodeIgniter fold :)

louisl · 2017-07-26T14:04:59Z

1.0.0-beta0011 released.
https://github.com/louisl/CodeIgniter4-Standard

Fixed issues with some more array declarations not being caught.

Thanks for the thanks!

natanfelles · 2017-12-03T23:17:50Z

Not sure how this rule must works. Is it OK currently?

louisl · 2017-12-04T09:07:07Z

@natanfelles We decided further up in this thread that we preferred to go with a space around dot's for concatenation, so the standard is right but the style guide hasn't been updated.

louisl · 2017-12-11T14:50:39Z

1.0.0-beta0012 released.
https://github.com/louisl/CodeIgniter4-Standard

Catches more array declarations types.
Ignores migration files class name and filename do not match.
No longer requires full stops after param comments.
Updated unit testing - https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.1.0
A few more unit tests.

jim-parry · 2017-12-12T10:33:50Z

We have ignition ... https://github.com/bcit-ci/CodeIgniter4-Standard

jim-parry added the help wanted More help is needed for the proper resolution of an issue or pull request label Jul 20, 2016

jim-parry added the in progress label Jul 13, 2017

jim-parry mentioned this issue Jul 13, 2017

Coding style checker #21

Closed

jim-parry closed this as completed Dec 12, 2017

Code Sniffer Rules #182

Code Sniffer Rules #182

Comments

jim-parry commented Jul 20, 2016

louisl commented Jun 22, 2017 • edited Loading

lonnieezell commented Jun 23, 2017

louisl commented Jun 23, 2017

lonnieezell commented Jun 23, 2017

louisl commented Jun 23, 2017 • edited Loading

lonnieezell commented Jun 23, 2017

jim-parry commented Jun 23, 2017

lonnieezell commented Jun 23, 2017

louisl commented Jun 29, 2017 • edited Loading

louisl commented Jun 29, 2017

louisl commented Jun 30, 2017

lonnieezell commented Jun 30, 2017

louisl commented Jun 30, 2017

lonnieezell commented Jun 30, 2017

louisl commented Jun 30, 2017

louisl commented Jul 3, 2017

louisl commented Jul 6, 2017 • edited Loading

lonnieezell commented Jul 7, 2017

louisl commented Jul 13, 2017 • edited Loading

louisl commented Jul 13, 2017

louisl commented Jul 13, 2017

lonnieezell commented Jul 13, 2017

jim-parry commented Jul 13, 2017

lonnieezell commented Jul 13, 2017

jim-parry commented Jul 13, 2017

louisl commented Jul 13, 2017 • edited Loading

louisl commented Jul 13, 2017

jim-parry commented Jul 13, 2017

louisl commented Jul 13, 2017 • edited Loading

jim-parry commented Jul 13, 2017

jim-parry commented Jul 13, 2017

louisl commented Jul 13, 2017

jim-parry commented Jul 13, 2017

louisl commented Jul 13, 2017

jim-parry commented Jul 13, 2017

louisl commented Jul 14, 2017

louisl commented Jul 19, 2017

louisl commented Jul 21, 2017

louisl commented Jul 25, 2017

lonnieezell commented Jul 26, 2017

jim-parry commented Jul 26, 2017

louisl commented Jul 26, 2017

natanfelles commented Dec 3, 2017

louisl commented Dec 4, 2017

louisl commented Dec 11, 2017

jim-parry commented Dec 12, 2017

louisl commented Jun 22, 2017 •

edited

Loading

louisl commented Jun 23, 2017 •

edited

Loading

louisl commented Jun 29, 2017 •

edited

Loading

louisl commented Jul 6, 2017 •

edited

Loading

louisl commented Jul 13, 2017 •

edited

Loading

louisl commented Jul 13, 2017 •

edited

Loading

louisl commented Jul 13, 2017 •

edited

Loading