PHPLIB-330: Implement checks for coding standard #668
Conversation
| <file>src</file> | ||
| <file>tests</file> | ||
|
|
||
| <rule ref="Doctrine"> |
There was a problem hiding this comment.
Should we change this to "MongoDB" if it's only going to be derived from Doctrine and modified for our needs? Alternatively, is this just indicating that we're inheriting the Doctrine rule set and then applying our own modifications below?
There was a problem hiding this comment.
This pulls in all rules from the Doctrine coding standard and allows us to exclude/modify some of them. Since we just configure phpcs.xml our ruleset doesn't have a name, that would only happen if we create a ruleset.xml in the standardised directory structure, which isn't necessary if we're not going to distribute the coding standard.
| // Since doctrine/coding-standard requires PHP 7, we can't add it as a dependency | ||
| // yet. This autoload file adds more information to the phpcs error message, | ||
| // telling the user how they can fix the error presented to them by phpcs. | ||
| if (! file_exists(__DIR__ . '/../vendor/doctrine/coding-standard')) { |
There was a problem hiding this comment.
Is there a particular reason we only pull doctrine/coding-standard in from the smoke test stage in .travis.yml and don't include it in our require-dev dependencies?
There was a problem hiding this comment.
Since doctrine/coding-standard requires PHP 7.1, including it in require-dev would implicitly bump our PHP requirement to 7.1 on CI systems, breaking tests for older PHP versions. That's why the check stage requires it manually.
To make the generic error message from phpcs more helpful I decided to use the autoload functionality of phpcs to include this error message if doctrine/coding-standard wasn't installed. Now that you mention it, it's probably best to also mention this in the contribution docs.
| env: | ||
| - CHECKS=phpunit | ||
| - stage: Smoke Testing | ||
| php: "7.1" |
There was a problem hiding this comment.
Is there a particular reason to use 7.1?
There was a problem hiding this comment.
Some of the sniffs we use have feature detection where they disable/enable functionality based on PHP version. For example, the sniff requiring type hints does not enforce an object type hint on methods unless running on PHP 7.2 or later. Thus, it's always a good idea to target the lowest supported version for the coding standard in CI.
| @@ -202,7 +203,7 @@ private function onIteration($incrementKey) | |||
|
|
|||
| /* Return early if there is not a current result. Avoid any attempt to | |||
| * increment the iterator's key. */ | |||
| if (!$this->valid()) { | |||
| if (! $this->valid()) { | |||
There was a problem hiding this comment.
Should this have spaces on both sides of the !? I see some later occurrences of ! were left as-is so I'm wondering if the coding standard permits both but only applies a trailing space when formatting incorrectly styled code.
There was a problem hiding this comment.
There is only a sniff to enforce a space after the negation operator, but not to enforce it before it, hence the different styles. The space after the opening parenthesis in function calls is forbidden by PSR-2, which serves as the base of doctrine/coding-standard. The space before is left over from me excluding a sniff for now (see comment below about control structure spacing)
| use function stream_copy_to_stream; | ||
| use function stream_get_meta_data; | ||
| use function stream_get_wrappers; | ||
| use function urlencode; |
There was a problem hiding this comment.
You'll get used to them. It greatly reduces eyestrain from seeing too many \ all over the place. FWIW, if you give in and start using PhpStorm, it will do this for you automagically.
| @@ -156,7 +156,7 @@ public function __construct($databaseName, $collectionName, JavascriptInterface | |||
| } | |||
|
|
|||
| if (isset($options['finalize']) && ! $options['finalize'] instanceof JavascriptInterface) { | |||
| throw InvalidArgumentException::invalidType('"finalize" option', $options['finalize'], Javascript::class); | |||
| throw InvalidArgumentException::invalidType('"finalize" option', $options['finalize'], JavascriptInterface::class); | |||
This introduces a coding standard that is automatically checked during the build using phpcs. The coding standard itself is derived from doctrine/coding-standard. I've tried to keep :
sizeofinstead ofcount)This PR was designed to not really have to be reviewed. Most fixes were applied automatically (see last commit), with only some changes done manually.
Note: I've made a point of disabling some sniffs in
DocumentationExamplesTest. This was done to keep the examples "portable" and allow people to simply copy/paste them to their code, e.g. by preserving fully qualified namespaces.