Extend PHPCS rules to enforce all style rules

[aaemnnosttv]

Feature Description

In addition to following WordPress core coding styles for PHP, there are some additional coding style rules which are applied that are not covered by the PHPCS configuration.

This may be partially covered by #547 but some rules will likely still not be enforced.

E.g.

• The order of annotations in a docblock
• The required presence of specific annotations in a docblock
• The presence of blank lines before/after annotations in a docblock

We should extend the PHPCS configuration to include these additions as much as possible.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

There should be PHPCS rules to enforce the following:

Semantic

• Every description of everything starts with a capital letter (descriptions, parameter docs, return docs, ...).
• Every description of everything ends in a dot (descriptions, parameter docs, return docs, ...).
• For functions and methods, the first word of the description must end in an "s" (this is probably 99% reliable, but most importantly should never yield a false positive - I don't think there are any exceptions to the grammatical rule of 3P verbs ending in an "s").

Organizational

• Every property/function/method/class/interface/trait doc-block should have a since tag.
• The order of tags (if present, for each) should be since (multiple entries allowed), deprecated, access, static, global, var, param (multiple entries allowed), return.
• For methods and functions, tags should be grouped: since/deprecated/access/static should be grouped together, separated by a blank line from param/return which should be grouped together. If there is global, it should be separated from both others by a blank line (essentially be its own group).
• For properties, all tags should be in a single group (no blank lines between them).
• All param tag types and descriptions in a single doc-block should be indented in a way that they are aligned with each other.

Bonus points for opening this also against WordPress core, which probably could use all of it :-)

Implementation Brief

• Create a new directory includes/CodeStandards/GoogleSiteKit/ where our new coding sniffs will live.

• Create a new ruleset file: includes/CodeStandards/GoogleSiteKit/ruleset.xml to define our new ruleset:

  <?xml version="1.0"?>
  <ruleset name="GoogleSiteKit">
  <description>Google Site Kit's additional coding standards.</description>
  </ruleset>

• To get composer to include our new coding standard rules whenever the vendor version of phpcs in run, add the following to the phpcs.xml file inside the <ruleset> tags:

  <rule ref="./includes/CodeStandards/GoogleSiteKit">
      <exclude-pattern>tests/*</exclude-pattern>
  </rule>

• For each of the required rules in the table below go through the following steps:

  1. Create a new file for the rule in includes/CodeStandards/GoogleSiteKit/Sniffs/Semantic or includes/CodeStandards/GoogleSiteKit/Sniffs/Organizational using the file name from the table.
  2. Build out the rule from this template, setting the token(s) in the register function from the table and coding the validation in the process function and using the class name from the table:

     
     <?php
     /**
     * This sniff prohibits the use of Perl style hash comments.
     *
     * PHP version 5
     *
     * @category  PHP
     * @package   PHP_CodeSniffer
     * @author    Your Name <you@domain.net>
     * @license   https://github.com/squizlabs/PHP_CodeSniffer/blob/master/licence.txt BSD Licence
     * @link      http://pear.php.net/package/PHP_CodeSniffer
     */

namespace PHP_CodeSniffer\Standards\MyStandard\Sniffs\Commenting;

use PHP_CodeSniffer\Sniffs\Sniff; use PHP_CodeSniffer\Files\File;

class DisallowHashCommentsSniff implements Sniff { /**

• Returns the token types that this sniff is interested in.

• @return array(int) */ public function register() { return array(T_COMMENT);

  }

  /**

• Processes this sniff, when one of its tokens is encountered.
• @param \PHP_CodeSniffer\Files\File $phpcsFile The current file being checked.
• @param int $stackPtr The position of the current token in the
• stack passed in $tokens.

• @return void */ public function process(File $phpcsFile, $stackPtr) { $tokens = $phpcsFile->getTokens(); if ($tokens[$stackPtr]['content']{0} === '#') { $error = 'Hash comments are prohibited; found %s'; $data = array(trim($tokens[$stackPtr]['content'])); $phpcsFile->addError($error, $stackPtr, 'Found', $data); }

  } }

?>

Create a new sniff file for each of the rules in the table:

**Semantic**

| File Name/Class Name | Register Token(s) | Process Function Brief |
| --- | --- | --- |
| `Semantic/DescriptionStartsWithCapitalLetter.php` / `DescriptionStartsWithCapitalLetter` | T_COMMENT | Every description of everything starts with a capital letter (descriptions, parameter docs, return docs, ...). |
| `Semantic/DescriptionEndsWithFullStop.php` / `DescriptionEndsWithFullStop` | T_COMMENT | Every description of everything ends in a dot (descriptions, parameter docs, return docs, ...). |
| `Semantic/DescriptionStartsWithThirdPersonVerb.php` / `DescriptionStartsWithThirdPersonVerb` | T_COMMENT | For functions and methods, the first word of the description must end in an "s" (this is probably 99% reliable, but most importantly should never yield a false positive - I don't think there are any exceptions to the grammatical rule of 3P verbs ending in an "s"). |

**Organizational**

| File Name/Class Name | Register Token(s) | Process Function Brief |
| --- | --- | --- |
| `Organizational/CommentHasSinceTag.php` / `DescriptionStartsWithThirdPersonVerb` | T_COMMENT | Every property/function/method/class/interface/trait doc-block should have a since tag. |
| `Organizational/CommentTagsInCorrectOrder.php` / `CommentTagsInCorrectOrder` | T_COMMENT | The order of tags (if present, for each) should be since (multiple entries allowed), deprecated, access, static, global, var, param (multiple entries allowed), return. |
| `Organizational/CommentTagsCorrectlyGrouped.php` / `CommentTagsCorrectlyGrouped ` | T_COMMENT | For methods and functions, tags should be grouped: since/deprecated/access/static should be grouped together, separated by a blank line from param/return which should be grouped together. If there is global, it should be separated from both others by a blank line (essentially be its own group). |
| `Organizational/CommentTagsHaveNoSpaces.php` / `CommentTagsHaveNoSpaces ` | T_COMMENT | For properties, all tags should be in a single group (no blank lines between them). |
| `Organizational/CommentTagsDescriptionsAligned.php` / `CommentTagsDescriptionsAligned ` | T_COMMENT | All param tag types and descriptions in a single doc-block should be indented in a way that they are aligned with each other. |

## Changelog entry

* <!-- One sentence summarizing the PR, to be used in the changelog. -->

[aaemnnosttv]

@felixarntz I've been looking into the IB for this one and it looks like the work to be done is fairly substantial - especially since we have no custom sniffs of our own yet - and may need to be broken into multiple issues. There's also a question as to whether this repo is the right place for these rules to be defined as it seems like some of these are filling in rules that aren't implemented in WPCS yet.

Either way I don't think it makes sense to plan this one for the upcoming sprint as there is more investigation/definition needed here.

[felixarntz]

@aaemnnosttv I believe we should move forward with this and implement these rules as part of the repository. We're now enforcing them in JS (see #1493), and it makes sense to add the same strictness to our PHP codebase.

I believe contributing this to WPCS would be a great effort, and we should do so, but I suggest decoupling that. We can replace our own implementation with theirs if such rules get adopted there.

[aaemnnosttv]

Sounds good, thanks for confirming @felixarntz.

I'm going to remove the GFI label though since I don't think it meets that criteria based on my previous investigation here.

[benbowler]

I've added an IB and an estimate.

[aaemnnosttv]

@benbowler – is this ready for review?

[benbowler]

Yes, moved into the review column.

[tofumatt]

A minor few issues with the IB:

1. There's a mention of a folder named includes/CodeStandards/GoogleSiteKit/Sniffs/Semantical but "Semantical" isn't a word. Did you mean "Semantic"?
2. There's a mixture of mentions of includes/**CodeStandards**/GoogleSiteKit and includes/**CodeStandards**/GoogleSiteKit. Are they meant to be different folders? That's sort of confusing if so.

I think the approach looks proper though—I'm not wildly familiar with writing PHP Sniff rules but I'd think this would work similar to the ESLint rules and this approach matches that from what I can tell.

Can you address the two issues I mentioned? After that this should be good to go 🙂

[benbowler]

Hey @tofumatt, I've replaced all examples of Semantical with Semantic and updated all of the folder names to CodeStandards.

[tofumatt]

IB ✅

[aaemnnosttv]

Bumping the estimate here since this one will undoubtedly be a beast and will also require a large number of changes across the codebase in order to conform to the new style rules.

cc: @eclarke1 @fhollis

[benbowler]

Ready for review on implementation.

[felixarntz]

@aaemnnosttv Assigning to you for now; can you check who would want to proceed with the work that Ben started?

[aaemnnosttv]

@felixarntz – happy to take this over although there are a few things I'd like to discuss first; let's discuss on our next call.

[aaemnnosttv]

@felixarntz given our conversation about the changes here, do you think this makes sense to keep in execution or move it back to definition since the implementation will be substantially different? I suppose the ACs could remain the same, but we should probably move and plan this issue to be specific to the final integration of the new sniffs since the majority of the engineering will no longer be part of the SK codebase.

[felixarntz]

@aaemnnosttv Yeah, let's move this back to ACs for now and later update them once the path forward is clear.

[aaemnnosttv]

@eclarke1 @fhollis moving back to ACs and removing from the sprint as this issue will be redefined to be much simpler but won't be something we can do until later.

[aaemnnosttv]

@felixarntz moving to stalled for now as this likely won't be actionable in the near future.

[felixarntz]

Moving this back to Backlog as we now have the new repository and #4187 as a prerequisite so this can soon move forward.

[swissspidy]

Just stumbled upon https://github.com/GoogleChromeLabs/wpcs-core-extra-sniffs & this issue.

Are you planning on submitting some of these rules to WPCS directly? Seems like most of them would be happily accepted there as they also apply to core.

[felixarntz]

@swissspidy Eventually, yes. We wanted to start them in their own repository though to be independent of WPCS itself. It would also allow us to "prove" them in the wild (in Site Kit) before proposing them for merge.

[aaemnnosttv]

@felixarntz what do you think about converting this to an epic? I think we probably have a single issue for laying the foundation and then each rule could probably have its own issue which could also potentially allow for some to happen at the same time.

[felixarntz]

@aaemnnosttv Good idea, that makes sense. Although I think we can skip the epic here and break this down into multiple issues without one - we already have a label to mark anything related to that other repository. First we should get started with #4187 though.