Missing: OO declaration statement formatting rules

Question

Missing: OO declaration statement formatting rules

jrfnl opened this issue 2 years ago · 2 comments

Is your feature request related to a problem?

I was looking at this part of the March 2020 Make post:

Traits and interfaces

....
For everything else, the same formatting rules as already in place for class declarations apply.

And then realized that there appears to be a gap in the sniffs included in the Core ruleset which check the formatting of class (and interface/trait/enum) declaration statements, even though there are explicit and implied rules in the coding standards and these are being consistently followed (mostly).

Explicit rules:

Class, trait, interface and enum names should use capitalized words separated by underscores. Any acronyms should be all upper case.
Checked by PEAR.NamingConventions.ValidClassName.
Ref: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#naming-conventions
One object structure per file.
Checked by Generic.Files.OneObjectStructurePerFile.
Ref: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#only-one-object-structure-class-interface-trait-per-file

Rules I can discern based on code samples and applied practices in WP Core are as follows:

Keywords should be lowercase - this is already being checked via the Generic.PHP.LowerCaseKeyword sniff.
Opening brace should be on the same line as the declaration - this is already being checked via the Generic.Classes.OpeningBraceSameLine sniff
There should be exactly one space before/after each keyword.
The declaration statement should be on one line, i.e. extends... and implements... should be on the same line as the class keyword.
Except for empty classes, the class close brace should be on a line by itself, indented the same as the start of the class declaration.
There should be no blank line between the end of the body of the class and the class close brace.

Describe the solution you'd like

I would like to propose to add one or more sniffs to enforce the implied rules which are currently not being checked.

// OK.
class FooBar extends FooBar_P implements FooBar_I, FooBar_J {}

// Not OK.
final   readonly  class   Foo_ClassA   extends   DateTime  implements   Countable {}

abstract   Class   Foo_ClassB
Extends
	DateTime
	Implements
		Countable {}

Trait    Foo_Trait
{
}

Interface    Bar_I
	EXTENDS   Foo_I,
		Baz_G
	{
		public function overload_me(); }

Enum   Foo_Enum : string   implements  Countable

{
	public function do_something() {}


}

Analysis of available sniffs

PHPCS contains the following sniffs checking OO declaration statements:

PSR1.Classes.ClassDeclaration
PSR2.Classes.ClassDeclaration
PEAR.Classes.ClassDeclaration
Squiz.Classes.ClassDeclaration

`PSR1.Classes.ClassDeclaration`

The PSR1 sniff is stand-alone and checks the following:

"Each * must be in a file by itself"
"Each * must be in a namespace of at least one level"

Rule 1 applies, but we already check this via another sniff.
Rule 2 does not apply to WP.

Conclusion: Nope, not a good match.

`PEAR/PSR2/Squiz.Classes.ClassDeclaration`

These three sniffs are closely related. The PEAR sniff acts as the base sniff, the PSR2 sniff extends on the PEAR sniff, the Squiz sniff extends the PSR2 sniff.

The PEAR sniff largely only checks the open brace position with the expectation that it should be on the line after the class keyword.
This contradicts the WP coding standards where we want to open brace on the same line as the class keyword.

Conclusion: The PEAR sniff does not do enough + what it does is not what we want, so no matter what sniff we end up with, we will need to exclude one or more error codes related to the open brace.

The PSR2 sniff builds on the PEAR sniff and adds additional checks for:

The spacing after the keywords and names;
Checks that the extends and implements keywords are on the same line as the class keyword.
Checks that the closing brace directly follows the body of the structure and is on a line by itself.
Note: the CloseBraceAfterBody check would currently yield ~136 errors for WP Core, which also means that the vast majority of the classes in WP Core comply with the rule.
As we also added a rule for the function close brace to directly follow the body, I think keeping the CloseBraceAfterBody check should be acceptable and the current errors can be auto-fixed when WP Core upgrades.

The sniff does allow for multi-line extends/implements declarations, but that should not be problematic.

Conclusion: The PSR2 sniff is a good candidate to add and would only need an exclusion for the open brace check.

The Squiz sniff builds on the PSR2 sniff and adds additional checks for:

Single class/interface per file. (already checked in a better way via another sniff)
Checks that the class keyword is at the start of a line.
This is problematic for conditional class declarations, which are being used in WP in themes and test files.
Add different checks for the close brace being on a line by itself and being at the start of the line.
Again, this is problematic for conditional class declarations.

Conclusion: The extras in the Squiz sniff would need to be excluded anyway, so the PSR2 sniff is a better fit.

Proposal

Add the PSR2 sniff with an exclusion only for the "open brace on new line" error.

With the above code sample and excluding sniffs which we already include in the ruleset, this would yield the following errors for the code sample listed above:

------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 24 ERRORS AFFECTING 14 LINES
------------------------------------------------------------------------------------------------------------------------------------------------------
  6 | ERROR | [x] Expected 1 space between readonly and class keywords; 2 found (PSR2.Classes.ClassDeclaration.SpaceBeforeKeyword)
  6 | ERROR | [x] Expected 1 space after class keyword; 3 found (PSR2.Classes.ClassDeclaration.SpaceAfterKeyword)
  6 | ERROR | [x] Expected 1 space after class name; 3 found (PSR2.Classes.ClassDeclaration.SpaceAfterName)
  6 | ERROR | [x] Expected 1 space before extends keyword; 3 found (PSR2.Classes.ClassDeclaration.SpaceBeforeExtends)
  6 | ERROR | [x] Expected 1 space before "DateTime"; 3 found (PSR2.Classes.ClassDeclaration.SpaceBeforeName)
  6 | ERROR | [x] Expected 1 space before implements keyword; 2 found (PSR2.Classes.ClassDeclaration.SpaceBeforeImplements)
  6 | ERROR | [x] Expected 1 space before "Countable"; 3 found (PSR2.Classes.ClassDeclaration.SpaceBeforeName)
  8 | ERROR | [x] Expected 1 space between abstract and class keywords; 3 found (PSR2.Classes.ClassDeclaration.SpaceBeforeKeyword)
  8 | ERROR | [x] Expected 1 space after class keyword; 3 found (PSR2.Classes.ClassDeclaration.SpaceAfterKeyword)
  9 | ERROR | [x] The extends keyword must be on the same line as the class name (PSR2.Classes.ClassDeclaration.ExtendsLine)
 10 | ERROR | [x] Expected 1 space before "DateTime"; 5 found (PSR2.Classes.ClassDeclaration.SpaceBeforeName)
 11 | ERROR | [x] The implements keyword must be on the same line as the class name (PSR2.Classes.ClassDeclaration.ImplementsLine)
 12 | ERROR | [x] Expected 4 spaces before interface name; 8 found (PSR2.Classes.ClassDeclaration.InterfaceWrongIndent)
 14 | ERROR | [x] Expected 1 space after trait keyword; 4 found (PSR2.Classes.ClassDeclaration.SpaceAfterKeyword)
 18 | ERROR | [x] Expected 1 space after interface keyword; 4 found (PSR2.Classes.ClassDeclaration.SpaceAfterKeyword)
 19 | ERROR | [x] The extends keyword must be on the same line as the interface name (PSR2.Classes.ClassDeclaration.ExtendsLine)
 19 | ERROR | [x] The first item in a multi-line extends list must be on the line following the extends keyword
    |       |     (PSR2.Classes.ClassDeclaration.FirstExtendsInterfaceSameLine)
 20 | ERROR | [x] Expected 4 spaces before interface name; 8 found (PSR2.Classes.ClassDeclaration.InterfaceWrongIndent)
 21 | ERROR | [x] Expected 0 spaces before opening brace; 4 found (PSR2.Classes.ClassDeclaration.SpaceBeforeBrace)
 22 | ERROR | [x] The closing brace for the Interface must go on the next line after the body (PSR2.Classes.ClassDeclaration.CloseBraceAfterBody)
 24 | ERROR | [x] Expected 1 space after enum keyword; 3 found (PSR2.Classes.ClassDeclaration.SpaceAfterKeyword)
 24 | ERROR | [x] Expected 1 space before implements keyword; 3 found (PSR2.Classes.ClassDeclaration.SpaceBeforeImplements)
 24 | ERROR | [x] Expected 1 space before "Countable"; 2 found (PSR2.Classes.ClassDeclaration.SpaceBeforeName)
 30 | ERROR | [x] The closing brace for the Enum must go on the next line after the body (PSR2.Classes.ClassDeclaration.CloseBraceAfterBody)
------------------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 24 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------------------------------------------

The only thing I'm in doubt about is whether or not to also exclude the FirstExtendsInterfaceSameLine (and related) error code(s) ?

Note: when run against WP Core, only the above mentioned ~136 errors for "blank line between structure body and close brace" are thrown. Other than that, WP Core already fully complies with this proposed addition.

Answer 1 · 2023-06-19T20:12:16.000Z

The only thing I'm in doubt about is whether or not to also exclude the FirstExtendsInterfaceSameLine (and related) error code(s) ?

If it was excluded, could that be re-enabled in WordPress-Extra?

Answer 2 · 2023-06-19T21:34:34.000Z

The only thing I'm in doubt about is whether or not to also exclude the FirstExtendsInterfaceSameLine (and related) error code(s) ?

If it was excluded, could that be re-enabled in WordPress-Extra?

Of course it could ;-)

Having said that, I'm not sure we should disable them. I mean, we don't really want multi-line declarations, but if those are used (and they are not completely forbidden via this sniff), making sure that the formatting is consistent seems like a good thihng.