YoastCS: more docblock/comment rules #364
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…am` tag This rule is disabled in WordPressCS as it doesn't play nice with the convoluted WP native array annotation. For YoastCS it has been decided to apply the rule anyway. We'll need to evaluate on a case-by-case basis how to convert from the WP array annotation to another form of documenting the array format. Related to 303 Impact on Yoast packages: | Plugin/Tool | Errors/Warnings | |-------------------|-----------------| | PHPUnit Polyfills | -- | WP Test Utils | -- | YoastCS | -- | WHIP | -- | Yoast Test Helper | -- | Duplicate Post | -- | Yst ACF | -- | Yst WooCommerce | 6 | Yst News | -- | Yst Local | 6 | Yst Video | 1 | Yst Premium | 11 | Yst Free | 77 Note: this rule was previously already "silently" enforced via clean-up sweeps.
This rule is disabled in WordPressCS as it doesn't play nice with the convoluted WP native array annotation. For YoastCS it has been decided to apply the rule anyway. We'll need to evaluate on a case-by-case basis how to convert from the WP array annotation to another form of documenting the array format. Related to 303 Impact on Yoast packages: | Plugin/Tool | Errors/Warnings | |-------------------|-----------------| | PHPUnit Polyfills | -- | WP Test Utils | -- | YoastCS | -- | WHIP | -- | Yoast Test Helper | -- | Duplicate Post | -- | Yst ACF | -- | Yst WooCommerce | 1 | Yst News | -- | Yst Local | 1 | Yst Video | -- | Yst Premium | -- | Yst Free | 11 Note: this rule was previously already "silently" enforced via clean-up sweeps.
This rule is disabled in WordPressCS as WP Core does not want `@return void`, but consistency and explicitness of _intend_ is important, so we're going to turn it back on. Note: `@return` tags will still not be enforced for constructors which should always be "never" methods anyway. Related to 303 Impact on Yoast packages: | Plugin/Tool | Errors/Warnings | |-------------------|-----------------| | PHPUnit Polyfills | 57 // = Test fixtures. | WP Test Utils | -- | YoastCS | -- | WHIP | 1 | Yoast Test Helper | 18 | Duplicate Post | 280 | Yst ACF | 14 | Yst WooCommerce | 172 | Yst News | 92 | Yst Local | 204 (but there are also still 54 functions without a docblock altogether) | Yst Video | 381 | Yst Premium | 804 + 2 invalid void | Yst Free | 4162 + 3 invalid void (and 4 function without a docblock altogether)
I.e. `bool` not `boolean`, `int` not `integer`. Related to 303 Impact on Yoast packages: | Plugin/Tool | Errors/Warnings | |-------------------|-----------------| | PHPUnit Polyfills | -- | WP Test Utils | -- | YoastCS | -- | WHIP | -- | Yoast Test Helper | -- | Duplicate Post | -- | Yst ACF | -- | Yst WooCommerce | -- | Yst News | -- | Yst Local | -- | Yst Video | -- | Yst Premium | -- | Yst Free | 1 Note: this rule was previously already "silently" enforced via clean-up sweeps.
(whenever relevant) Related to 303 Impact on Yoast packages: | Plugin/Tool | Errors/Warnings | |-------------------|-----------------| | PHPUnit Polyfills | -- | WP Test Utils | -- | YoastCS | -- | WHIP | -- | Yoast Test Helper | -- | Duplicate Post | -- | Yst ACF | -- | Yst WooCommerce | -- | Yst News | -- | Yst Local | -- | Yst Video | -- | Yst Premium | -- | Yst Free | -- Note: this rule was previously already "silently" enforced via clean-up sweeps.
This sniff:
* Disallows plain `array` type annotations. These should always be made specific.
* Flags missing type annotations.
Optionally the sniff can enforce PHP native type declarations instead of annotations in a docblock.
This functionality has been **disabled** for the following reasons:
* Adding type declarations without using `strict_types` leads to bugs as the original (scalar) data will be juggled by PHP, even if the original data was never correct to start with. The loss of the type information of the original data passed, means that the function can no longer discern whether the input is invalid/incorrect.
Think: a function expecting an integer, but being passed `false`. This will be juggled to `0`, while it should have been flagged as unusable input.
* Adding `strict_types` in the context of WP is a no-no to begin with as it can cause fatal errors (= white screen of death) when the exceptions are not caught and they generally are not caught and WP doesn't have a native exception handler in place either.
* Adding `strict_types` is useless anyway as it is only enforced for files containing such a declaration, which means that parameters passed to a function call from an external source which doesn't have the `strict_types` declaration (callback called from WP) will still be juggled, which again leads to bugs.
* Adding type declarations in functions hooked into WP filters is especially dangerous due to the loss of type info.
All in all, with the type system offered by PHP as-is, adding PHP native type declarations is next to useless unless you have full control of all code + the environment the code is being run on, which for the Yoast code is just not the case (with the exception of Platform).
Related to 303
Impact on Yoast packages:
| Plugin/Tool | Errors/Warnings |
|-------------------|-----------------|
| PHPUnit Polyfills | --
| WP Test Utils | --
| YoastCS | --
| WHIP | 2
| Yoast Test Helper | 2
| Duplicate Post | 1
| Yst ACF | 4
| Yst WooCommerce | 7
| Yst News | 3
| Yst Local | 40
| Yst Video | 44
| Yst Premium | 85
| Yst Free | 366
This sniff:
* Disallows plain `array` type annotations. These should always be made specific.
* Flags missing type annotations.
Optionally the sniff can enforce PHP native type declarations instead of annotations in a docblock.
This functionality has been **disabled** for the following reasons:
* Adding type declarations without using `strict_types` leads to bugs as the original (scalar) data will be juggled by PHP, even if the original data was never correct to start with. The loss of the type information of the original data passed, means that the function can no longer discern whether the input is invalid/incorrect.
Think: a function expecting an integer, but being passed `false`. This will be juggled to `0`, while it should have been flagged as unusable input.
* Adding `strict_types` in the context of WP is a no-no to begin with as it can cause fatal errors (= white screen of death) when the exceptions are not caught and they generally are not caught and WP doesn't have a native exception handler in place either.
* Adding `strict_types` is useless anyway as it is only enforced for files containing such a declaration, which means that parameters passed to a function call from an external source which doesn't have the `strict_types` declaration (callback called from WP) will still be juggled, which again leads to bugs.
* Adding type declarations in functions hooked into WP filters is especially dangerous due to the loss of type info.
All in all, with the type system offered by PHP as-is, adding PHP native type declarations is next to useless unless you have full control of all code + the environment the code is being run on, which for the Yoast code is just not the case (with the exception of Platform).
Related to 303
Impact on Yoast packages:
| Plugin/Tool | Errors/Warnings |
|-------------------|-----------------|
| PHPUnit Polyfills | 4
| WP Test Utils | 3
| YoastCS | --
| WHIP | 4
| Yoast Test Helper | 10
| Duplicate Post | 37
| Yst ACF | 1
| Yst WooCommerce | 37
| Yst News | 17
| Yst Local | 176
| Yst Video | 147
| Yst Premium | 263
| Yst Free | 913
This sniff:
* Disallows plain `array` type annotations. These should always be made specific.
* Flags missing type annotations.
Optionally the sniff can enforce PHP native type declarations instead of annotations in a docblock.
This functionality has been **disabled** for the following reasons:
* Adding type declarations without using `strict_types` leads to bugs as the original (scalar) data will be juggled by PHP, even if the original data was never correct to start with. The loss of the type information of the original data passed, means that the function can no longer discern whether the input is invalid/incorrect.
Think: a function expecting an integer, but being passed `false`. This will be juggled to `0`, while it should have been flagged as unusable input.
* Adding `strict_types` in the context of WP is a no-no to begin with as it can cause fatal errors (= white screen of death) when the exceptions are not caught and they generally are not caught and WP doesn't have a native exception handler in place either.
* Adding `strict_types` is useless anyway as it is only enforced for files containing such a declaration, which means that parameters passed to a function call from an external source which doesn't have the `strict_types` declaration (callback called from WP) will still be juggled, which again leads to bugs.
* Adding type declarations in functions hooked into WP filters is especially dangerous due to the loss of type info.
All in all, with the type system offered by PHP as-is, adding PHP native type declarations is next to useless unless you have full control of all code + the environment the code is being run on, which for the Yoast code is just not the case (with the exception of Platform).
Related to 303
Impact on Yoast packages:
| Plugin/Tool | Errors/Warnings |
|-------------------|-----------------|
| PHPUnit Polyfills | 22
| WP Test Utils | 9
| YoastCS | --
| WHIP | 5
| Yoast Test Helper | 21
| Duplicate Post | 44
| Yst ACF | 5
| Yst WooCommerce | 48
| Yst News | 22
| Yst Local | 132
| Yst Video | 237
| Yst Premium | 269
| Yst Free | 1066
An exception is made for the tests, as a data provider for input validation within a function could well be passing every single different type, in which case, `mixed` is perfectly valid. Related to 303 Fixes 196 Impact on Yoast packages: | Plugin/Tool | Errors/Warnings | |-------------------|-----------------| | PHPUnit Polyfills | 49 (this is valid though as these are assertion declarations) | WP Test Utils | -- | YoastCS | 2 | WHIP | -- | Yoast Test Helper | 4 | Duplicate Post | 12 | Yst ACF | 1 | Yst WooCommerce | 2 | Yst News | 1 | Yst Local | 42 | Yst Video | 12 | Yst Premium | 17 | Yst Free | 151
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
YoastCS rules: enforce alignment of the description parts for a
@paramtagThis rule is disabled in WordPressCS as it doesn't play nice with the convoluted WP native array annotation.
For YoastCS it has been decided to apply the rule anyway. We'll need to evaluate on a case-by-case basis how to convert from the WP array annotation to another form of documenting the array format.
Impact on Yoast packages:
Note: this rule was previously already "silently" enforced via clean-up sweeps.
YoastCS rules: enforce proper grammar/punctuation in param description
This rule is disabled in WordPressCS as it doesn't play nice with the convoluted WP native array annotation.
For YoastCS it has been decided to apply the rule anyway. We'll need to evaluate on a case-by-case basis how to convert from the WP array annotation to another form of documenting the array format.
Impact on Yoast packages:
Note: this rule was previously already "silently" enforced via clean-up sweeps.
YoastCS rules: enforce that docblocks should have a valid @return tag
This rule is disabled in WordPressCS as WP Core does not want
@return void, but consistency and explicitness of intend is important, so we're going to turn it back on.Note:
@returntags will still not be enforced for constructors which should always be "never" methods anyway.Impact on Yoast packages:
YoastCS rules: enforce short form type annotations
I.e.
boolnotboolean,intnotinteger.Impact on Yoast packages:
Note: this rule was previously already "silently" enforced via clean-up sweeps.
YoastCS rules: enforce type annotations to have null as last type
(whenever relevant)
Impact on Yoast packages:
Note: this rule was previously already "silently" enforced via clean-up sweeps.
YoastCS rules: verify property type annotations
This sniff:
arraytype annotations. These should always be made specific.Optionally the sniff can enforce PHP native type declarations instead of annotations in a docblock.
This functionality has been disabled for the following reasons:
strict_typesleads to bugs as the original (scalar) data will be juggled by PHP, even if the original data was never correct to start with. The loss of the type information of the original data passed, means that the function can no longer discern whether the input is invalid/incorrect.Think: a function expecting an integer, but being passed
false. This will be juggled to0, while it should have been flagged as unusable input.strict_typesin the context of WP is a no-no to begin with as it can cause fatal errors (= white screen of death) when the exceptions are not caught and they generally are not caught and WP doesn't have a native exception handler in place either.strict_typesis useless anyway as it is only enforced for files containing such a declaration, which means that parameters passed to a function call from an external source which doesn't have thestrict_typesdeclaration (callback called from WP) will still be juggled, which again leads to bugs.All in all, with the type system offered by PHP as-is, adding PHP native type declarations is next to useless unless you have full control of all code + the environment the code is being run on, which for the Yoast code is just not the case (with the exception of Platform).
Impact on Yoast packages:
YoastCS rules: verify parameter type annotations
This sniff:
arraytype annotations. These should always be made specific.Optionally the sniff can enforce PHP native type declarations instead of annotations in a docblock.
This functionality has been disabled for the following reasons:
strict_typesleads to bugs as the original (scalar) data will be juggled by PHP, even if the original data was never correct to start with. The loss of the type information of the original data passed, means that the function can no longer discern whether the input is invalid/incorrect.Think: a function expecting an integer, but being passed
false. This will be juggled to0, while it should have been flagged as unusable input.strict_typesin the context of WP is a no-no to begin with as it can cause fatal errors (= white screen of death) when the exceptions are not caught and they generally are not caught and WP doesn't have a native exception handler in place either.strict_typesis useless anyway as it is only enforced for files containing such a declaration, which means that parameters passed to a function call from an external source which doesn't have thestrict_typesdeclaration (callback called from WP) will still be juggled, which again leads to bugs.All in all, with the type system offered by PHP as-is, adding PHP native type declarations is next to useless unless you have full control of all code + the environment the code is being run on, which for the Yoast code is just not the case (with the exception of Platform).
Impact on Yoast packages:
YoastCS rules: verify return type annotations
This sniff:
arraytype annotations. These should always be made specific.Optionally the sniff can enforce PHP native type declarations instead of annotations in a docblock.
This functionality has been disabled for the following reasons:
strict_typesleads to bugs as the original (scalar) data will be juggled by PHP, even if the original data was never correct to start with. The loss of the type information of the original data passed, means that the function can no longer discern whether the input is invalid/incorrect.Think: a function expecting an integer, but being passed
false. This will be juggled to0, while it should have been flagged as unusable input.strict_typesin the context of WP is a no-no to begin with as it can cause fatal errors (= white screen of death) when the exceptions are not caught and they generally are not caught and WP doesn't have a native exception handler in place either.strict_typesis useless anyway as it is only enforced for files containing such a declaration, which means that parameters passed to a function call from an external source which doesn't have thestrict_typesdeclaration (callback called from WP) will still be juggled, which again leads to bugs.All in all, with the type system offered by PHP as-is, adding PHP native type declarations is next to useless unless you have full control of all code + the environment the code is being run on, which for the Yoast code is just not the case (with the exception of Platform).
Impact on Yoast packages:
YoastCS rules: disallow the "mixed" type in annotations
An exception is made for the tests, as a data provider for input validation within a function could well be passing every single different type, in which case,
mixedis perfectly valid.Fixes #196
Impact on Yoast packages:
Related to #303