WIP: Add code-style validator to Travis CI config

[Kolyunya]

Q	A
Is bugfix?	no
New feature?	no
Breaks BC?	no
Tests pass?	yes
Fixed issues

I'd like to suggest to make use of a code-style validator during Travis CI checks to improve the general quality of the framework code and to prevent PSR-2 violations.

[samdark]

I don't think it should be applied to JavaScript.

[Kolyunya]

I think JS should be validated as well. If not by the PHPCS then by what? Don't mind dropping JS validation by PHPSC altogether though.

[samdark]

It's OK to be validated but the standard should not be the same as for PHP. Common placement of { in JavaScript is same line.

[Kolyunya]

Disabled *.js files validation altogether for the time being.

[samdark]

I don't think it should be applied to JavaScript.

[Kolyunya]

#13732 (comment)

[samdark]

I don't think it should be applied to JavaScript.

[Kolyunya]

#13732 (comment)

[samdark]

Does not look correct to me.

[Kolyunya]

This seems to be correct: squizlabs/PHP_CodeSniffer#366 (comment)

I'm not sure what PSR2 actually intends in this case. They don't mention the alternate syntax anywhere. So I still need to enforce the spaces after opening parentheses error...

[samdark]

No PSR mentions it including PSR-12 in the works but Yii's internal doc does: https://github.com/yiisoft/yii2/blob/master/docs/internals/view-code-style.md

Accoding to the doc it should be:

<?php if ($summary['errors'] > 0): ?>

i.e. no space.

[Kolyunya]

Excluded that rule.

[samdark]

Doesn't look correct to me.

[Kolyunya]

#13732 (comment)

[samdark]

Doesn't look correct to me.

[Kolyunya]

#13732 (comment)

[samdark]

Doesn't look correct to me.

[Kolyunya]

#13732 (comment)

[samdark]

Doesn't look correct to me.

[Kolyunya]

#13732 (comment)

[samdark]

Doesn't look correct to me.

[Kolyunya]

#13732 (comment)

[Kolyunya]

This is what PHPCBF thinks it should look like. Haven't reviewed this myself yet.

[samdark]

This fix is OK.

[Kolyunya]

This is what PHPCBF thinks it should look like. Haven't reviewed this myself yet.

[Kolyunya]

This is what PHPCBF thinks it should look like. Haven't reviewed this myself yet.

[rob006]

I think that using https://github.com/FriendsOfPHP/PHP-CS-Fixer would be much more useful and flexible.

[Kolyunya]

@rob006 what are the particular issues of the current setup which could be solved by the PHP-CS-Fixer?

[rob006]

PHP-CS-Fixer is easier to configure and extend. It has more active development and community IMO.

I also made a quick look into fixes from this PR and many of this changes looks invalid and just dumb. I never had such problems with PHP-CS-Fixer before.

I could open alternative PR with PHP-CS-Fixer integration, so we could just compare results and choose better tool for this.

[Kolyunya]

many of this changes looks invalid and just dumb.

@rob006 could you name them explicitly please (excluding */views/*)?

[rob006]

diff --git a/framework/db/Query.php b/framework/db/Query.php
index b20a8b4..c95b734 100644
--- a/framework/db/Query.php
+++ b/framework/db/Query.php
@@ -408,8 +408,7 @@ protected function queryScalar($selectExpression, $db)
             return null;
         }
 
-        if (
-            !$this->distinct
+        if (!$this->distinct
             && empty($this->groupBy)
             && empty($this->having)
             && empty($this->union)

diff --git a/framework/mutex/OracleMutex.php b/framework/mutex/OracleMutex.php
index bf78add..b0f1d88 100644
--- a/framework/mutex/OracleMutex.php
+++ b/framework/mutex/OracleMutex.php
@@ -89,14 +89,14 @@ protected function acquireLock($name, $timeout = 0)
 
         /** inside pl/sql scopes pdo binding not working correctly :(  */
         $this->db->createCommand(
-                'DECLARE
+            'DECLARE
     handle VARCHAR2(128);
 BEGIN
     DBMS_LOCK.ALLOCATE_UNIQUE(:name, handle);
     :lockStatus := DBMS_LOCK.REQUEST(handle, DBMS_LOCK.' . $this->lockMode . ', ' . $timeout . ', ' . $releaseOnCommit . ');
 END;',
-                [':name' => $name]
-            )
+            [':name' => $name]
+        )
             ->bindParam(':lockStatus', $lockStatus, PDO::PARAM_INT, 1)
             ->execute();

Why "excluding /views/"? Don't you want fix views files too?

[Kolyunya]

PHP-CS-Fixer is easier to configure and extend.

@rob006 what do you mean by easier? Configuring each of the rules is a matter of a single (couple) lines in the config.

diff --git a/framework/mutex/OracleMutex.php b/framework/mutex/OracleMutex.php
index bf78add..b0f1d88 100644
--- a/framework/mutex/OracleMutex.php
+++ b/framework/mutex/OracleMutex.php
@@ -89,14 +89,14 @@ protected function acquireLock($name, $timeout = 0)
 
         /** inside pl/sql scopes pdo binding not working correctly :(  */
         $this->db->createCommand(
-                'DECLARE
+            'DECLARE
     handle VARCHAR2(128);
 BEGIN
     DBMS_LOCK.ALLOCATE_UNIQUE(:name, handle);
     :lockStatus := DBMS_LOCK.REQUEST(handle, DBMS_LOCK.' . $this->lockMode . ', ' . $timeout . ', ' . $releaseOnCommit . ');
 END;',
-                [':name' => $name]
-            )
+            [':name' => $name]
+        )
             ->bindParam(':lockStatus', $lockStatus, PDO::PARAM_INT, 1)
             ->execute();

This is a violation of the PSR-2.

Code MUST use an indent of 4 spaces, and MUST NOT use tabs for indenting.

[Kolyunya]

@rob006 I simplified the config and left only PSR-2 rules while removing all others. Would you mind reviewing reported issues now?

[Kolyunya]

diff --git a/framework/db/Query.php b/framework/db/Query.php
index b20a8b4..c95b734 100644
--- a/framework/db/Query.php
+++ b/framework/db/Query.php
@@ -408,8 +408,7 @@ protected function queryScalar($selectExpression, $db)
             return null;
         }
 
-        if (
-            !$this->distinct
+        if (!$this->distinct
             && empty($this->groupBy)
             && empty($this->having)
             && empty($this->union)

@rob006 this is an obvious violation of the PSR-2.

Opening parentheses for control structures MUST NOT have a space after them...

[rob006]

what do you mean by easier?

In PHP-CS-Fixer configuration is an object. So you could create own class with configuration and easy share between different projects. You could also easy modify this configuration by extending config class.

We could create composer package with yii2 specific configuration and fixers - then everybody could use Yii2 CS in his own project and made some modifications by extending yii2 cs config class. With phpcs.xml you need copy this file and keep sync manually.

This is a matter of the convention. I like the proposed change.

It is much less readable and consistent. With long conditions it is easy to miss that indentation block is part of condition and not if body.

You also don't use

if ($something) { $this->doSomethig();
    $this->doSomethingElse();
}

so why you want do this in if condition block?

Why would you use 8 spaces for indentation instead of 4 here?

I'm referring to:

-                [':name' => $name]
-            )
+            [':name' => $name]
+        )
             ->bindParam(':lockStatus', $lockStatus, PDO::PARAM_INT, 1)
             ->execute();

Why closing parenthesis is not intended with ->bindParam()?

[Kolyunya]

It is much less readable and consistent.

The original version violates the standard:

Opening parentheses for control structures MUST NOT have a space after them...

Why closing parenthesis is not intended with ->bindParam()?

Why should it be? Could you provide a rule from the standard which is violated please?

With phpcs.xml you need copy this file and keep sync manually.

It's not even required. We can specify the standard name via the CLI.

[rob006]

The original version violates the standard:

Opening parentheses for control structures MUST NOT have a space after them...

To be strict, there is no space after opening parenthesis - there is only a single LF char.
But there is a space before closing parenthesis (from indentation) and it is not fixed. Do you think that it should? :D

Why should it be? Could you provide a rule from the standard which is violated please?

There is no rule, you just broke nice indentation. For what?

[Kolyunya]

There is no rule, you just broke nice indentation. For what?

@rob006 would you mind defining nice? ->bindParam() is indented one block to the right of $this and this indentation is used all over the core in hundreds of places e.g.

[rob006]

Nice:

        $query
            ->select(
                ['COUNT(*)']
            )
            ->from($this->cacheTable)
            ->where('[[id]] = :id AND ([[expire]] = 0 OR [[expire]] >' . time() . ')', [':id' => $key]);

Not nice:

        $query->select(
            ['COUNT(*)']
        )
            ->from($this->cacheTable)
            ->where('[[id]] = :id AND ([[expire]] = 0 OR [[expire]] >' . time() . ')', [':id' => $key]);

Whole call chain should be intended once, so you can easily see when it starts and when it ends.

[dynasource]

@Kolyunya, whats your opinion about PHP-CS-Fixer?

[Kolyunya]

@dynasource at the time of posting this PR I haven't tried it yet. I tried it a couple of days ago and it seemed to me that it wasn't a silver bullet either and should be tweaked too to provide sensible reports.

I think we should stick to the tool which would provide most accurate reports. I just proposed a tool I was aware of. This PR is a proposal to use some code-style validation tool, not specifically phpcs.

[dynasource]

I think it is a good suggestion and that it is useful to have one. Now we have 2 different solutions proposed here, we should choose one. Would be nice to have opinions from the @community.

[photodude]

If you are just applying PSR2 why not use style-ci see https://styleci.io/

PHPCS and others are more for code styles that have complex requierments beyond things like PSR2

[Kolyunya]

We will probably want to enforce custom Yii2 code-style.

[photodude]

Do you have an existing list for what you want to enforce?

for example

• https://developer.joomla.org/coding-standards.html
• https://codex.wordpress.org/WordPress_Coding_Standards
• https://www.drupal.org/docs/develop/standards
• http://symfony.com/doc/current/contributing/code/standards.html
• https://laravel.com/docs/5.4/contributions#coding-style
• https://www.codeigniter.com/user_guide/general/styleguide.html

[Kolyunya]

Sure we have.

[photodude]

I think you will find some of those items can't be enforced without custom enforcer scripts (for example the way the comments and docblocks are written) as there is no standard available for what your presenting.

[photodude]

There is also a big difference between a written code style with examples, and just an example file that shows acceptable coding (the latter is what you presented).

[gsherwood]

Hi all. I maintain PHPCS so I thought I'd drop in and clear some things up.

PHPCS is a tool for checking coding standards in your project, whatever they may be. It ships with a few standards (including the old PEAR standard and the much newer PSR-1 and PSR-2 standards) but it is designed for you to pick and choose from the included rules and design your own. You can also write your own checks and auto-fixes if none of the included checks meet your requirements. For projects that have no idea what standard to use, using a built-in one is probably the easiest thing to do, but that's not a requirement. A lot of projects customise their coding standards, even if it is a simple as PSR-2 minus a couple of things they don't like.

PHPCS ships with lots of different report types to help you integrate it into IDEs, CI systems, text editors, and custom code, but you can also define your own custom report classes if you want. One report that might be useful is the information report. This one is used to tell you information about your code base and is helpful when deciding on a standard. I use this report to generate my Analysis of Coding Conventions in PHP projects. Note that I have Yii2 in there and I've found that your code is very consistent. You can view the report of how the project's coding conventions have changed over the last 2+ years here: https://squizlabs.github.io/PHP_CodeSniffer/analysis/yiisoft/yii2/index.html

Writing your own standard for PHPCS is a matter of creating an XML file. The format is quite extensive and allows for customisiation of the checks performed, but also the severity of each message, the message content itself, regex based file exclusions for checks (some projects don't want specific checks done on specific files or folders), and overriding a complete set of config variables.

That XML format is documented here: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-ruleset.xml

Also note that you can commit your standard as a file called phpcs.xml (or phpcs.xml.dist) in your project's root directory so that developers can just run phpcs and the standard tells PHPCS which files and directories to process, and what checks to perform. But you don't have to. You could also just pass around the XML file and use it with PHPCS to ensure people are running the same checks over their code. Or a sub-project could create their own custom standard that imports your XML file and changes a few things.

WordPress does this with their coding standards. You can see they have a main WordPress Core standard, but also extensions like VIP and Extra. They do a great job with their coding standards, have a lot of custom sniffs, contribute a lot back to PHPCS, and can fix quite a lot of the found errors automatically. It's worth taking a look at that repo if you want to see PHPCS in use.

Drupal's Coder is another example of this, but it is less extensive. I also get a lot of good contributions from the developers of this project though.

The 3.0 version is the best to use right now if you are starting fresh, and that version also allows you to make use of PHPCS directly in code. For example, if you just want to run PHPCS over a snippet of code, you could do it like this: https://gist.github.com/gsherwood/aafd2c16631a8a872f0c4a23916962ac . Or if you'd like to tokenize JS files th same way PHP files are tokenized, you can use this code: https://gist.github.com/gsherwood/f17cfc90f14a9b29eeb6b2e99e6e7f66 . It adds a bit more flexibility to what is typically just a CLI tool.

PHPCS also comes with a tool called PHPCBF (officially, the PHP Code Beautifier and Fixer, but maybe you can figure out the unofficial name) which is used to fix coding standard issues. PHPCS reports errors and PHPCBF fixes them. Quite a lot of errors that PHPCS finds are fixable, and I've spent several years tweaking the fixers (especially the one that does indents) to work with a wide range of code.

Despite concerns here that PHPCS development and community is not very active, I can tell you that I get a lot of contributions from developers and I've seen code in bug reports that no developer should probably see. But that helps me make PHPCS more robust and able to deal with a range of issues, including graceful handling of parse errors, which is pretty important if you want developers to be constantly checking standards in their IDE.

I hope this clears up a few things about PHPCS and hopefully I haven't taken up too much of your time.

Ultimately, you should choose whichever tool you think works best for your project. But I wanted to make it clear that PHPCS has been actively developed for more than 10 years and still gets 30-40k downloads a day. It's an active project with an active user base, and I think it has a very extensive feature set that makes it flexible enough for most projects. I'd encourage you to give it a go if you haven't already. It's free :)

[cebe]

@gsherwood thanks for sharing your thoughts about this!

[samdark]

So overall PHPCS looks like a good match and the changes in the pull request do make sense. @cebe how about merging it?

[Kolyunya]

@samdark I think we should tweak config a bit. PHP CS Fixer reported some issues which PHPCS didn't. We should decide which we would like to adopt.

[samdark]

OK. Moving to 2.0.13 then.

[jrfnl]

Hiya, I've had a quick look at some of the things PHP CS Fixes found which weren't reported by PHPCS so far and would like to make the below suggestions for rules to add to your ruleset to cover these.
You may also want to review the existing exclusions in your ruleset carefully as they are partially to blame for the issues which weren't found.

N.B.: If you'd include the below suggestions, you may need/want to tweaks them a bit to exclude extraneous error codes coming from these sniffs which you are not interested in.

<!-- Covers: https://github.com/yiisoft/yii2/pull/13778/files#diff-ef822e03c117e20fd1d786afc7c5b5e3L772 and similar -->
<rule ref="Squiz.ControlStructures.ControlSignature" />

<!-- Covers: https://github.com/yiisoft/yii2/pull/13778/files#diff-03b64c04d37c13a3118b4b8119f5ae8dL439, https://github.com/yiisoft/yii2/pull/13778/files#diff-ef822e03c117e20fd1d786afc7c5b5e3L219, https://github.com/yiisoft/yii2/pull/13778/files#diff-85a8556277e468a30afc79beb7aa25dfL660 and similar -->
<rule ref="Generic.WhiteSpace.ScopeIndent">
    <properties>
        <property name="exact" value="4" />
    </properties>
</rule>

<!-- Covers: https://github.com/yiisoft/yii2/pull/13778/files#diff-c6dd99ed2184ed7a7f316aee5fe67513L492 and similar -->
<rule ref="Squiz.Commenting.PostStatementComment" />

<!-- Covers: https://github.com/yiisoft/yii2/pull/13778/files#diff-709ff61c838c754d533b87ec33f14908L115 and similar -->
<rule ref="PSR2.ControlStructures.ElseIfDeclaration.NotAllowed">
    <type>error</type>
</rule>

<!-- Covers: https://github.com/yiisoft/yii2/pull/13778/files#diff-f000680354101249e9f60e71e6278e3bL136 and similar -->
<rule ref="Generic.Functions.OpeningFunctionBraceBsdAllman" />

<!-- If you also want to enforce braces on the next line for closures: -->
<rule ref="Generic.Functions.OpeningFunctionBraceBsdAllman">
    <properties>
        <property name="checkClosures" value="true" />
    </properties>
</rule>

<!-- Covers: https://github.com/yiisoft/yii2/pull/13778/files#diff-3f97b6f0e759a8a52920b07abe345579L69, https://github.com/yiisoft/yii2/pull/13778/files#diff-74bb1570acfdb8abc2d6d89e79b0ab54L88 and similar -->
<rule ref="Squiz.WhiteSpace.SuperfluousWhitespace" />

I'm pretty sure there's actually also a sniff which will catch the extraneous blank lines, but I can't think of the name of the sniff off-hand.

Hope this helps ;-)

[rob006]

see #14100

[samdark]

I like @rob006 approach more. Closing this one. @Kolyunya thank you for preparing the pull request. At least it triggered the continuation @rob006 did.

[Kolyunya]

@samdark I'm just curious what do you mean by the approach? We can extract config and custom rules to a separate repository and it's possible to enable same rules. I really want to know what are exact pros and cons of each of the options.

[samdark]

PHP configs are a bit more flexible than XML IMO but both approaches (tools) are OK. Also @rob006 covered a lot of cases without false positives so it doesn't make sense to re-do the same using different tool, I think.