[THEME-3651] Update the PHPCS rules #2
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
…dundant parameter The previous change made passing the array `$closer` token to the `process_multi_line_array()` method redundant, so let's remove the parameter.
…sting level As the checks for whitespace around the array opener/closer have now been removed, the code in the `process_single_line_array()` method no longer needs to be nested in condition upon condition. This commit applies the typical "bow out early" pattern to the code in the method. :point_right: This change will be easier to review while ignoring whitespace changes.
…ename-bring-back-catID NamingConventions/ValidVariableName: bring back `$catID`
…tionspacing-phpcsutils-phpcsextra Arrays/ArrayDeclarationSpacing: partially replace the sniff + implement PHPCSUtils in what remains
> ### Trait Use Statements > > Trait use statements should be at the top of a class and should have exactly one blank line before the first use statement, and at least one blank line after the last statement. The only exception is when the class only contains trait use statements, in which case the blank line after may be omitted. > > The following code examples show the formatting requirements for trait use statements regarding things like spacing, grouping and indentation. Refs: * https://make.wordpress.org/core/2020/03/20/updating-the-coding-standards-for-modern-php/ - Trait use statement section * https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#trait-use-statements * WordPress/wpcs-docs 101 * WordPress/wpcs-docs 130 * https://www.php-fig.org/psr/psr-12/#42-using-traits The WP rules for traits are very akin to the PSR12 rules with only a few exceptions: * WP allows a blank line between the class/trait declaration statement and the first trait `use` statement. * In the Make post, the proposed rules allowed for single-line `use` statements with conflict resolution. PSR12 always demands multi-line in that case. The upstream PSR12 trait use sniff therefore seems like a good fit. I propose to exclude the "blank line before first `use`" error code to address the first point mentioned above. As for the second point, the code sample which showed the single-line `use` statement in the docs has been updated to multi-line in PR WordPress/wpcs-docs 130. With that change and taking into consideration that these rules are new to WPCS anyway, it makes sense to actually apply the PSR version of the rule demanding that conflict resolution must always be multi-line. Other than that, this PR contains one more exclusion to prevent duplicate error messages about a spacing issue already covered via another sniff. Note: this exclusion won't work (yet) as there is a typo in the upstream sniff. PR squizlabs/PHP_CodeSniffer 3856 is open to fix this.
PHPCSUtils released version 1.0.8 with a small bug fix. PHPCSExtra has just released version 1.1.0. with 7 new sniffs, most of which we'll want to add to WPCS (will be done in follow-up PRs). Refs: * https://github.com/PHPCSStandards/PHPCSUtils/releases/tag/1.0.8 * https://github.com/PHPCSStandards/PHPCSExtra/releases/tag/1.1.0
…ate-phpcsextra-phpcsutils Composer: update PHPCSUtils + PHPCSExtra
Includes preventing a duplicate notification about "spacing after the `use` keyword". Refs: * `Universal.UseStatements.DisallowMixedGroupUse` - upstream PHPCSStandards/PHPCSExtra 241 * `Universal.UseStatements.NoUselessAliases` - upstream PHPCSStandards/PHPCSExtra 244 * `Universal.UseStatements.KeywordSpacing` - upstream PHPCSStandards/PHPCSExtra 247
... which ought to be replaced by `prinft(...)`. Ref: PHPCSStandards/PHPCSExtra 242
... with two sniffs from PHPCSExtra.
The original WPCS native sniff did two things:
1. Check for a comma after the last item in an array (forbid it for single-line arrays, enforce it for multi-line arrays).
2. Check the spacing around commas in arrays (no space before the comma, one space or new line after the comma with an allowance for aligned trailing comments).
The sniff will now be replaced by the following upstream sniffs:
* `NormalizedArrays.Arrays.CommaAfterLast` for the comma after the last item in an array.
* `Universal.WhiteSpace.CommaSpacing` for the spacing around commas (and not just in arrays).
The behaviour of the sniffs is 100% in line with the original sniff (verified by running both the old and the new sniffs over the test case file for the WPCS sniff being removed).
Additionally, the `Universal.WhiteSpace.CommaSpacing` sniff will now check the spacing around commas in more places, where WPCS previously did not check the comma spacing and will make sure the comma is before a trailing comment, not after.
The `Universal.WhiteSpace.CommaSpacing` sniff by default checks the spacing for _all_ commas with the following exceptions:
* A comma preceded or followed by a parenthesis, curly or square bracket will not be flagged to prevent conflicts with sniffs handling spacing around braces.
* A comma preceded or followed by another comma, like for skipping items in a list assignment, will not be flagged.
* A comma preceded by a non-indented heredoc/nowdoc closer.
These will be flagged, but in that case, unless the `php_version` config directive is set to a version higher than PHP 7.3.0, a new line before will be enforced to prevent parse errors on PHP < 7.3.
This also means that the sniff will flag issues with the spacing around commas in function declarations, function calls and closure use statements, while WPCS already has other sniffs in place to handle this.
Luckily, the upstream sniff offers very modular error codes, so we can selectively silence those errors in the most common situations to prevent duplicate notices about the same issue.
For those places were the `Universal` sniff doesn't offer modular codes, we can, in most cases, selectively silence the errors coming from other sniffs. This commit includes some of those exclusions, but we may need to add more if we discover additional message duplications.
There may, however, still be some places which could get duplicate messages. It is, however, not expected that those will ever lead to fixer conflicts.
Both upstream sniffs contain fixers for all issues.
Ref:
* PHPCSStandards/PHPCSExtra 11
* PHPCSStandards/PHPCSExtra 254
…-arrays-commaafterlastitem-sniff
…t-add-noechosprintf-sniff
…-add-more-import-use-sniffs
Double-checked the ruleset against the handbook. Everything currently in the handbook is now in the Core ruleset. :white_checkmark: * Removed duplicate rule (the `Universal.UseStatements.LowercaseFunctionConst` was previously added here, but subsequently added to the "Use import statement" section as well). * Removed links to closed issues when the issue has been addressed in the mean time. * Made sure that rules which haven't got a sniff associated with them, while they are potentially sniffable, have an issue attached to them. * Made sure that rules which are "covered" by one or more sniffs have the `Covers` prefix.
…ks-tidy-up Rulesets: various minor tweaks
* Add sniff to the list of sniffs using the `MinimumWPVersionTrait`
* Fix incorrect implementation of the minimum WP version check.
This check was never supposed to hide the `QuotedIdentifierPlaceholder` check when the minimum supported WP version is below 6.2.
Instead, the check is needed to flag the `%i` modifier as an "unsupported placeholder" when used with a minimum supported WP version below 6.2.
Includes tests safeguarding the new `UnsupportedIdentifierPlaceholder` error.
Note: I've left the `phpcs:set WordPress.DB.PreparedSQLPlaceholders minimum_wp_version 6.2` before the original set of test in place to prevent having to up the error/warning nrs for all those tests, while this particular error is safeguarded via separate tests anyway.
Includes a few minor doc tweaks.
Includes a minor efficiency fix (condition order in an `if`).
Support '%i' placeholders for escaping Identifiers
... to covered previously uncovered code. Note: some of the messages generated for these (mostly invalid) code samples are not always that clear, but the fact that the queries are being flagged is still correct.
These variables are conditionally set within the `for()` loop. Just declaring them here to be more explicit/complete with the variables in use within the loop.
This change is already covered by existing tests (test case file line 58 and 259).
In both these cases, a "quick check" for the target function call was done prior to walking the tokens, but the token walking with a negative search for `Tokens::$emptyToken` doesn't contain the risk of walking very far, so this quick check isn't really needed and removing it shouldn't impact performance.
… params [1] The `'raw'` key in the parameter arrays returned from the `PassedParameters` class contains - as per the name - the _raw_ contents of the parameter. Since PHPCSUtils 1.0.0-alpha4, the return array also contain a `'clean'` index, which contains the contents of the parameter cleaned of comments. By switching to using that key, a potential false positive gets fixed. Includes unit test demonstrating the issue and safeguarding the fix.
… params [2] The `'raw'` key in the parameter arrays returned from the `PassedParameters` class contains - as per the name - the _raw_ contents of the parameter. Since PHPCSUtils 1.0.0-alpha4, the return array also contain a `'clean'` index, which contains the contents of the parameter cleaned of comments. By switching to using that key, a potential false negative gets fixed. Includes unit test demonstrating the issue and safeguarding the fix.
… params [3] The `'raw'` key in the parameter arrays returned from the `PassedParameters` class contains - as per the name - the _raw_ contents of the parameter. Since PHPCSUtils 1.0.0-alpha4, the return array also contain a `'clean'` index, which contains the contents of the parameter cleaned of comments. By switching to using that key, a potential false positive gets fixed. Includes unit test demonstrating the issue and safeguarding the fix. Also includes unit tests with an unsupported placeholder in the `array_fill()`, which looks like it was so far untested.
When looking for a function calls to `implode()` or `array_fill()`, the sniff did not allow for fully qualified function calls, which would lead to false positives. Fixed now. Includes unit tests demonstrating the issue and safeguarding the fix.
As things were, the `$skip_to` param would lead to the first token _after_ the parenthesis closer also being skipped. This was not the intention and while - for valid code - this won't lead to false positives/negatives, it should still be fixed.
…ifierWithinIN Throw the error for `IdentifierWithinIN` error on the actual text string token in the parameter instead of on the function call to `implode()` to give a better indication where the actual error occurred. Safeguarded by adjusting one of the pre-existing tests.
…dDynamicPlaceholderGeneration Throw the error for `QuotedDynamicPlaceholderGeneration` error on the actual text string token which contains the open quote instead of on the function call to `implode()` to give a better indication where the actual error occurred. Safeguarded via the existing tests.
Add the funding link and section in the readme. Add the link to the v3 release make post in the readme. Add the FUNDING.yml file so that we can add the funding button in the repo. Co-authored-by: Juliette <[email protected]>
Co-authored-by: Gary Jones <[email protected]>
This should either be an array of links or a singular link. The `WP PHP` was regarded as an invalid link (not as a link description). Ref: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/displaying-a-sponsor-button-in-your-repository
Apparently we have to pro-actively inform the team as they don't appear to watch Make. Ref: https://developer.wordpress.org/news/2023/09/whats-new-for-developers-september-2023/#comment-1611
…se-checklist Release checklist: add link to monthly dev blog
…-codes-for-error-generation Security/EscapeOutputSniff: More modular error codes
This commit adds an initial Dependabot configuration to:
* Submit pull requests for security updates and version updates for GH Action runner dependencies.
At a later point in time, we could consider enabling it for Composer dependencies as well.
The configuration has been set up to:
* Run weekly (for now).
* Submit a maximum of 5 pull requests at a time.
If additional pull requests are needed, these will subsequently be submitted the next time Dependabot runs after one or more of the open pull requests have been merged.
* The commit messages for PRs submitted by Dependabot will be prefixed according the unofficial conventions used in this repo up to now.
* The PRs will automatically be labelled with an appropriate label as already in use in this repo.
Refs:
* https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file
* https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuration-options-for-the-dependabot.yml-file#versioning-strategy
Co-authored-by: Juliette <[email protected]> Co-authored-by: Gary Jones <[email protected]>
…r-wpcs-3.0.1-release Add changelog for v3.0.1
Release WordPressCS 3.0.1
…ot-config-for-ghactions
Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
…-update Fix the release date in the changelog
…tions/actions/checkout-4
The posts per page sniff should bail out early if empty string is passed as a value. The tests were added for both posts per page sniff and slow db query sniff, to check if empty string is passed as a value. In the case of SlowDBQuery the sniff should flag cases where there is and isn't a value passed, as that sniff will always flag whenever meta_key and meta_value are used in a query.
…-error Add defensive coding in posts per page sniff
Just noticed this didn't display as intended.
pawelmpc
pushed a commit
that referenced
this pull request
Apr 18, 2024
1. Adjusted the way the correct parameter is retrieved to use the new PHPCSUtils 1.0.0-alpha4 `PassedParameters::getParameterFromStack()` method.
2. Verified the parameter name used is in line with the name as per the WP 6.1 release.
WP has been renaming parameters and is probably not done yet, but it doesn't look like those changes (so far) made it into changelog entries.... _sigh_.
It also looks like in the past, deprecated parameters were being renamed to `$deprecated` on deprecation. This practice should be strongly discouraged in the context of named parameters, but that's a different discussion and outside of the scope of this commit.
For the purposes of this exercise, I've taken the _current_ parameter name as the "truth" as support for named parameters hasn't officially been announced yet, so any renames _after_ this moment are the only ones relevant.
3. Adjusted the error message/data determination to take named parameters into account.
Name verification notes:
* Removed the `comments_link()` function.
Per [the changelog](https://developer.wordpress.org/reference/functions/comments_number/#changelog), the previously deprecated fourth parameter is now being used for something else since WP 5.4.0....
Far from best practice, but it is what it is, so I've removed the listing for that function.
What has **not** been done: a scan of the WP Core codebase to find newly deprecated parameters.
Includes additional unit tests and updating one existing test to ensure the code logic is properly covered.
Note: I thought of updating the error message to no longer refer to the position of the parameter, but to use the parameter name instead (with a "Found: %s" for displaying value), as in:
```bash
# Current:
The parameter "$variable" at position #2 of wp_new_user_notification() has been deprecated since WordPress version 4.3.1. Instead do not pass the parameter.
# Variation:
The $deprecated parameter of the wp_new_user_notification() function has been deprecated since WordPress version 4.3.1. Instead do not pass the parameter. Found: $variable
```
... but as WP renamed all deprecated parameters to `$deprecated`, this wouldn't necessarily be more descriptive than it is now.
Along the same lines, I considered updating the error code to refer to the param name instead of the position, but aside from it not making things more descriptive as things are, that would also constitute a BC-break which I don't think is warranted.
Opinions appreciated.
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.
Sniff::has_whitelist_comment()method and all references to itWordPress.CodeAnalysis.EmptyStatementsniffWordPress.PHP.DisallowShortTernarysniffWordPress.WhiteSpace.DisallowInlineTabssniffT_STRING_CASTWordPress.PHP.StrictComparisonssniffGeneric.Arrays.DisallowShortArraySyntaxwithUniversalversionSquiz.WhiteSpace.LanguageConstructSpacingsniff with theGenericoneisMagicFunctionName()isMagicMethodName()MinimumWPVersionTraitget_wp_version_from_cl()method$cli_supported_versionvariableGetTokensAsString(unset)cast from warning to errorExtratoCoreimagecreatefromwebperror_reporting=-1ramsey/composer-installGeneric.VersionControl.GitMergeConflictsniffTextStringHelperDeprecationHelperVariableHelperVariableHelpermatchexpressionset-output$wpdb$wpdbmatchingWordPress.WhiteSpace.ObjectOperatorSpacingsniffGeneric.CodeAnalysis.UnusedFunctionParametersniffExtratoCoreOneObjectStructurePerFilesniffExtratoCoreExtratoCoreBacktickOperatorsniff fromExtratoCoreWordPress.WP.ClassNameCasesniffnamekey downlatestbin/pre-commitfileWordPress.WhiteSpace.PrecisionAlignmentsniffExtratoCore::classclass resolutionantispambot()to the listMissingVersionto a warning$hookFunctionspropertyWPHookHelperget_hook_name_param()methodwp_version_compare()method (bug fix)has_nonce_check()method to theSecurity/NonceVerificationSniff$cache*Functionsproperties to theDB/DirectDatabaseQuerySniffRulesetPropertyHelperWPDBTraitWPGlobalVariablesHelperVariableHelper@sincetagsglobalstatement closed via close tagTRUNCATEqueries$SQL*Functionsproperties to theDB/PreparedSQLSniffwp_version_compare()methodSnakeCaseHelperload_script_textdomain()functionfinalfinalSuperfluousDefaultTextDomainauto-fix code to allow for named paramscheck_translator_commentspropertyUnorderedPlaceholders*could mangle text string@coverstags@coverstags for Helpers@coverstags for abstract base sniff classesis_class_object_call()utility method to dedicatedContextHelperis_token_namespaced()utility method to dedicatedContextHelperis_in_function_call()utility method to dedicatedContextHelperis_in_type_test()utility method to dedicatedContextHelperis_in_isset_or_empty()utility method to dedicatedContextHelperis_safe_casted()utility method to dedicatedContextHelper[s]printf()placeholdersis_in_array_comparison()utility method to dedicatedContextHelperget_list_variables()utility method to dedicatedListHelperexcludepropertyEndFileNewlinesniff with the PSR2 oneis_use_of_global_constant()utility method to dedicatedConstantsHelperEscapingFunctionsTraitPrintingFunctionsTraitFormattingFunctionsHelperArrayWalkingFunctionsHelperSanitizingFunctionsTrait(Move sanitization functions related functionality to dedicatedSanitizingFunctionsTraitWordPress/WordPress-Coding-Standards#2259)UnslashingFunctionsHelperValidationHelper$inst(ances) array$instmore comprehensibleprivateConstantsHelperclass$matched_contentparameter@internaltags@internaltag to classes marked as internal API@usestags$addedCustomFunctionsproperty$_FILESnot$_FILEneeds_nonce_check()method$catIDusestatementsTwenty*themes classesminimum_wp_versionto WP 6.0Modernize.FunctionCalls.Dirnamesniff$safe_componentsarrayprocess_token()method [1]process_token()method [2]get_printing_functions()method@packagetagsdevelopSanitizingFunctionsTraittoSanitizationHelperTraitSanitizationHelperTrait@returntags@var/@param/@returntag improvements@returntagsmastertomainfinalMissingUnslashmessage more informative$thisshould be staticinit()methodtrigger_error$codeper request