[WIP] Extended Zend Framework Coding Standard

[michalbundyra]

Work in progress

Just want to know what you guys thinks about it.

Basically @weierophinney asked me to add just detection for unused imports.
I went "a bit" more far with it and I've added much more rules.
I found easy way how we can write custom rules and we don't need to copy them to Standards directory.

List what we have there now:

• Generic.Arrays.DisallowLongArraySyntax,
• Generic.Formatting.SpaceAfterCast (must be one space after cast: (bool) $var),
• Generic.Formatting.SpaceAfterNot,
• Generic.CodeAnalysis.EmptyStatement,
• Generic.CodeAnalysis.ForLoopShouldBeWhileLoop,
• Generic.CodeAnalysis.UnconditionalIfStatement,
• Generic.CodeAnalysis.UnnecessaryFinalModifier,
• Generic.NamingConventions.ConstructorName,
• Generic.PHP.ForbiddenFunctions,
• Generic.Files.OneTraitPerFile,
• Generic.Strings.UnnecessaryStringConcat,
• Squiz.Operators.ValidLogicalOperators,
• Squiz.PHP.LowercasePHPFunctions,
• Squiz.PHP.NonExecutableCode,
• Squiz.Scope.StaticThisUsage,
• Squiz.Strings.ConcatenationSpacing,
• Squiz.Strings.DoubleQuoteUsage,
• Squiz.WhiteSpace.LanguageConstructSpacing,
• Squiz.WhiteSpace.OperatorSpacing,
• Squiz.WhiteSpace.SemicolonSpacing,
• Squiz.WhiteSpace.SuperfluousWhitespace,
• ZendCodingStandard.Arrays.Format (no space after [ and before ], each element in new line for multi-line arrays, no empty lines, correct indents...),
• ZendCodingStandard.Arrays.TrailingArrayComma (requires comma after last element in multiline arrays),
• ZendCodingStandard.Classes.AlphabeticallySortedTraits,
• ZendCodingStandard.Classes.NoNullValues - remove null values for class properties,
• ZendCodingStandard.Classes.TraitUsage,
• ZendCodingStandard.Commenting.DocComment,
• ZendCodingStandard.Commenting.FunctionComment - order of tags and empty lines between tags,
• ZendCodingStandard.Commenting.FunctionDataProviderTag,
• ZendCodingStandard.Commenting.FunctionDisallowedTag - using some tags is disallowed, for example @expectedException* from PHPUnit annotations should be replaced with according function calls just before place, where the exception should be thrown,
• ZendCodingStandard.Commenting.NoInlineCommentAfterCurlyClose,
• ZendCodingStandard.Commenting.TagCase,
• ZendCodingStandard.Commenting.VariableComment,
• ZendCodingStandard.Formatting.DoubleColon - no spaces before and after,
• ZendCodingStandard.Formatting.NewKeyword - one space after,
• ZendCodingStandard.Formatting.NoSpaceAfterSplat - no space after ...,
• ZendCodingStandard.Formatting.Reference - no space after reference operator &$var,
• ZendCodingStandard.Formatting.ReturnType (formating of return type on PHP 7 and 7.1 with nullable types),
• ZendCodingStandard.Formatting.UnnecessaryParentheses (remove unnecessary parentheses where it does not misaligned the readability),
• ZendCodingStandard.Functions.Param - checks method params and tags - required PHPDocs tags only when type hint is not provided or type hint needs specification (array, iterable, \Traversable),
• ZendCodingStandard.Functions.ReturnType - checks method return statement and return type, tag is required only when return type is not declared or needs specification (array, iterable, \Traversable), if function returns void, return tag must be omitted,
• ZendCodingStandard.Functions.Throws - checks throws tags and throw statement in the function - throw tag must be declared for each exception thrown explicitly in the function,
• ZendCodingStandard.Methods.LineAfter (must be exactly one line after method in class, zero after last method in the class),
• ZendCodingStandard.Namespaces.AlphabeticallySortedUses,
• ZendCodingStandard.Namespaces.ConstAndFunctionKeywords - lower case keywords and single space after,
• ZendCodingStandard.Namespaces.UnusedUseStatement (detects unused use statements),
• ZendCodingStandard.Namespaces.UseDoesNotStartWithBackslash,
• ZendCodingStandard.NamingConventions.ValidVariableName (all variables should be camelCase style),
• ZendCodingStandard.Operators.BooleanOperator (boolean operators are not allowed at the end of the line),
• ZendCodingStandard.Operators.TernaryOperator (if multi-line, each part should be in new line starting from operator ? or :),
• ZendCodingStandard.PHP.CorrectClassNameCase (checks case for defined classes),
• ZendCodingStandard.PHP.ImportInternalConstant (import php built-in constants),
• ZendCodingStandard.PHP.ImportInternalFunction (import php built-in functions),
• ZendCodingStandard.PHP.InstantiatingParenthesis (PSR-12 - always parenthesis when instantiating a new class),
• ZendCodingStandard.PHP.LowerCaseKeyword (extends Generic.PHP.LowerCaseKeyword),
• ZendCodingStandard.PHP.RedundantSemicolon (unnecessary semicolons - for example after closing curly bracket of "if" statement),
• ZendCodingStandard.PHP.TypeCasting (disallow !! casting, require (bool) and (int) instead of (boolean) and (integer) accordingly, disallow (unset) cast, always lowercase),
• ZendCodingStandard.Strings.NoConcatenationAtTheEnd,
• ZendCodingStandard.WhiteSpace.BlankLine (only one blank line allowed between parts of the code),
• ZendCodingStandard.WhiteSpace.CommaSpacing (no space before comma, one after, allowed more than one in multidimensional arrays),
• ZendCodingStandard.WhiteSpace.NoBlankLineAtStart (disallow blank line(s) at start of class, closure, function, interface and trait),
• ZendCodingStandard.WhiteSpace.ScopeIndent (experimental, stricter rules of indents).

I would like to add also some more rules/sniffs:

• trait sniff
  • must be one use keyword per declaration - ZendCodingStandard.Classes.TraitUsage,
  • exactly one blank line after traits and before methods in class - ZendCodingStandard.Classes.TraitUsage,
  • alphabetical order - ZendCodingStandard.Classes.AlphabeticallySortedTraits,
  • checking everything inside curly brackets,
• global use statements sniffs
  • alphabetical order - ZendCodingStandard.Namespaces.AlphabeticallySortedUses,
  • unused uses - ZendCodingStandard.Namespaces.UnusedUseStatement ,
  • lower case const and function keywords - ZendCodingStandard.Namespaces.ConstAndFunctionKeywords,
  • one space after const and function keywords - ZendCodingStandard.Namespaces.ConstAndFunctionKeywords,
  • use does not start with backslash - ZendCodingStandard.Namespaces.UseDoesNotStartWithBackslash,
• no empty line on top of the body class/method - ZendCodingStandard.WhiteSpace.NoBlankLineAtStart,
• more than one blank line is not allowed - ZendCodingStandard.WhiteSpace.BlankLine,
• correct camelCase names of internal PHP class (for example DateTime instead of Datetime) - ZendCodingStandard.PHP.CorrectClassNameCase,
• concatenation character shouldn't be at the end of the line ZendCodingStandard.Strings.NoConcatenationAtTheEndSniff,
• logical operator shouldn't be at the end of the line - ZendCodingStandard.Operators.BooleanOperator,
• PHPDocs with @dataProvider:
  • only allowed for test... methods,
  • data provider name doesn't have "Provider" suffix,
  • should be always above @param tags,
  • should be one empty line after and before @dataProvider ...,
• disallow or and and for logical operators (use || and && accordingly) - Squiz.Operators.ValidLogicalOperators,
• disallow method names the same as their class (deprecated error in PHP 7.0: Methods with the same name as their class will not be constructors in a future version) - Generic.NamingConventions.ConstructorName,
• arrays formatting - ZendCodingStandard.Arrays.Format:
  • no white space after array opening bracket and before closing (disallow [ 1, 2, 3 ]),
  • each element in new line for multi-line arrays
  • correct indents,
  • disallow empty lines in arrays,
• no space after splat operator (...$var) - ZendCodingStandard.Formatting.NoSpaceAfterSplat,
• no space before and after double colon operator (ClassName::class) - ZendCodingStandard.Formatting.DoubleColon,
• only one space after new - ZendCodingStandard.Formatting.NewKeyword,
• "When instantiating a new class, parenthesis MUST always be present even when there are no arguments passed to the constructor." (PSR-12),
• unnecessary semicolons (for example after if curly closing bracket) - ZendCodingStandard.PHP.RedundantSemicolon,
• no space after reference operator & - ZendCodingStandard.Formatting.Reference.

Of course would be nice to have also tests for all of our custom sniffs...

So as you can see it's quite long list. Can you think about anything else what will be worth to add?

Let me know what do you think. Thanks!

/cc @weierophinney @Koopzington @xtreamwayz

[geerteltink]

I'm a bit worried about this. This adds a lot of restrictions. I do think that most of them are a good thing, since there is a lot of coding style inconsistency in the current code base. However it's going to be a lot of work to fix all the code in all packages. With the 75 PR's that koopz opened last night, only 19 passed the new tests. I do think that if you change the ruleset it should be done in one go.

One addition that might be useful: Add a sniff for the license header which could replace this PR and add it in one go for all packages.

Also I like to see tests so we know the sniffs are working as intended. And if some sniffs are changed in PhpCodeSniffer, we can adjust accordingly. Maybe a daily travis test can help out with this.

[Koopzington]

tbh i could've probably avoided most of the currently failing PRs (most of them are due to the newly introduced rule to have one space after the NOT operator) but i'm gonna work my way through them.
Some repos will have to introduce BC breaking changes to incompatible method names in classes.

I like @xtreamwayz' idea for the license header sniff. It should help us getting those updates through way faster.

Most of the Sniffs seem to have the ability to automatically fix issues which would help a lot in case they're getting merged.
I actually can't tell how many errors would show up if we test ZendCodingStandard.NamingConventions.ValidVariableName but due to the lack of a automatic fix for them this one could take a bit of time to get applied.

[weierophinney]

Some repos will have to introduce BC breaking changes to incompatible method names in classes.

Don't do this. Add exclusion rules for these files or add @codingStandardsIgnoreStart / @codingStandardsIgnoreEnd declarations around the method names.

We can also introduce new methods to which the originals proxy, but we do not want to break compatibility just to make the code pass the standards at this point.

[geerteltink]

Can't you mark some tests as warnings so they will be displayed, but won't fail the tests?

[Koopzington]

phpcs returns error code 1 once a warning shows up and causes the build to fail.
You can either configure it to display warnings and return error code 1 or not display them and return error code 0.

[Koopzington]

I just realized something regarding that License Header Sniff:
Any file that get's excluded from phpcs won't get updated.

[Gamewalker]

Hey, if you use this ruleset and you have annotations like:

    /**
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="AUTO")
     * @ORM\Column(type="integer", nullable=false, options={"unsigned"=true})
     */
    protected $id;

phpcs tells you that

use Doctrine\ORM\Mapping as ORM;

isn`t used. Seems like Annotations aren´t checked.

Also the following isnt working:

@ORM\Column(type="integer", tag is not allowed in member variable comment

[mrVrAlex]

Please bare in mind that PSR-12 is not yet approved...

I know, but currently it in Review status https://www.php-fig.org/psr/ where:

Unless a PSR is marked as “Accepted” it is subject to change. Draft can change drastically, but Review will only have minor changes.

What part is not consistent? Can you provide more details?

1. Compound namespaces is allowed in PSR - https://github.com/php-fig/fig-standards/blob/master/proposed/extended-coding-style-guide.md#3-declare-statements-namespace-and-import-statements
   With Zend Coding Standart there will 2 errors:

There must be one USE keyword per declaration
Missing indent. Expected 4 spaces

2. Not sure, but PSR12 have:

When you have a return type declaration present there MUST be one space after the colon followed by the type declaration. The colon and declaration MUST be on the same line as the argument list closing parentheses with no spaces between the two characters.

[weierophinney]

@shandyDev

Compound namespaces is allowed in PSR

Allowed, but not required. As such, we'd still comply. (Remember, this coding standard is for the ZF repos, and for enforcing consistency within our projects; we're choosing to enforce one way to handle imports. That choice is compatible with PSR-12.)

When you have a return type declaration present there MUST be one space after the colon followed by the type declaration.

This one is currently being debated, with Larry Garfield and several others reviewing the spec suggesting that the format we use here makes more sense. Since this particular point is still being debated, we will leave it as-is, and revisit once it goes to a vote.

[mrVrAlex]

Allowed, but not required.

Exactly, I agree

Why you think what space between parentheses and colon makes more sense (even in RFC no spaces in examples))?
Awaiting PSR12 & ZCS ;)

[Ocramius]

Why you think what space between parentheses and colon makes more sense (even in RFC no spaces in examples))?

Mostly for readability purposes. It's a heated topic, like most CS rules, but doctrine/coding-standard also enforces the ) : foo { style

[mrVrAlex]

Well, if such expert "mastodon" like Ocramius for space, then I am fully convinced. ))

[grizzm0]

You would never see a space before ":" while writing which makes it feels really "unnatural" to do so. Atleast for me. That's why my vote is for function(Foo $bar): Baz {}.

There's also no space before ":" in a switch case, before ":" in if (): (short syntax) nor before a scope resolution operator (::).

If you would describe the function in writing you would type
The function Foo should return: Bar
not
The function Foo should return : Bar

[weierophinney]

You would never see a space before ":" while writing which makes it feels really "unnatural" to do so. Atleast for me. That's why my vote is for function(Foo $bar): Baz {}.

You do see it with ternary statements, however: $x ? $y : $z.

Personally, I find the whitespace surrounding it makes it harder to accidentally omit when adding your return type, leading to fewer mistakes.

Regardless of either of our own personal preferences, we will follow the guidelines of PSR-12 once it's finalized. (Interestingly, this is one of the very topics that forced it back into draft status, as quite a number of people prefer spaces surrounding the : when defining return types; the WG for the spec is debating that, and several other items, currently.)

[tux-rampage]

Is this still a thing since #5 (that seems to fulfill the same purpose) was merged?

[michalbundyra]

It's not. All work done here has been moved to https://github.com/webimpress/coding-standard