Release WordPressCS 3.0.0 #2369
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
Prevent a potential "Undefined array key "bracket_closer"" PHP notice in the case of parse errors.
... to allow for splitting the test case file up for properly pure tests and for adding additional test case files.
…ate test case file This prevents tests which involve the global scope from influencing each other (which was happening for an upcoming bug fix).
... to raise code coverage.
There were plenty of tests which checked the nonce verification in combination with a superglobal being used in `isset()` and `array_key_exists()`, but none in combination with `empty()`, while the sniff does handle that as well and treats it the same as `isset()`/`array_key_exists()`. This adjusts a few pre-existing tests to make sure that the handling of `empty()` is covered by tests too. Note: it is important that these have separate tests as `empty()` has its own token.
The `NonceVerification` sniff extends the WPCS base `Sniff` class, so has access to `$this->tokens`. No need for the call to `$phpcsFile->getTokens()`.
No need to store the sub-array if it's only accessed a couple of times.
* As the sniff itself now only handles the nonce functions, we don't need a multi-dimensional array (the `'nonce'` subkey).
* To prevent potential conflicts with properties in traits, make the property name more specific.
* And even though the class has not been made `final`, I do think it makes sense to make this property `private`.
The property is only intended for use by the (`protected`) `mergeFunctionLists()` method. If a child class overloads that method, they are responsible for whatever properties their overloaded method needs.
PHP does not have a `$_FILE` superglobal... Ref: https://www.php.net/manual/en/language.variables.superglobals.php Looks like this typo has been in this sniff from the start: * WordPress/WordPress-Coding-Standards 325 * 1480350 A similar typo elsewhere was fixed long time ago: WordPress/WordPress-Coding-Standards 368 Adjusted some existing tests now to safeguard this.
No idea why this was made a function local `static` variable, but it makes more sense (and results in cleaner code) if we cache this in a property. As the cache wasn't previously accessible from child classes anyway, I'm making this a `private` property.
As things were, the "cache" would only ever contain information about one search. Now, while this is fine for files which have all code in the global scope òr all code within function scopes, for mixed files, this makes the cache useless as the cache would be reset based on global or function scope and results would not be re-used as intended. In this commit, I'm changing the cache to a multi-dimensional array, which can hold information about all searches done within a file, which should improve performance for (large) mixed files. When I then add some debug code, you can see the difference based on the new test file: **Before** ``` Start: 0 End: 321 ---- Cache re-used ---- Cache re-used ---- Start: 113 End: 150 ---- Cache re-used ---- Start: 0 End: 188 ---- Cache re-used ---- Start: 212 End: 249 ---- Cache re-used ---- Start: 0 End: 321 ---- Cache re-used ``` **After** ``` Start: 0 End: 321 ---- Cache re-used ---- Cache re-used ---- Start: 113 End: 150 ---- Cache re-used ---- Cache re-used ---- Cache re-used ---- Start: 212 End: 249 ---- Cache re-used ---- Cache re-used ---- Cache re-used ```
Move the logic for getting and setting the cache to separate methods. Includes removing the cache "priming", which isn't actually necessary as the cache will be set, no matter what, before the function returns. Even though the class isn't `final`, as the cache was previously not accessible from child classes anyway, I'm making these methods `private`.
Properties with the same name as a superglobal should be ignored as these are not the actual superglobal. Note: I'm not adding a similar check for function parameters as a function parameter with the same name as a superglobal with cause a fatal error since PHP 5.4, so IMO checking for that is not relevant. Includes tests.
Sometimes I hate my brain.... Okay, so, while hopefully not common, assignments to superglobals can also happen via list constructs. As this sniff (correctly) disregards all assignments as safe, we should also disregard list assignments. This commit makes it so. Includes tests safeguarding the fix, which would previously lead to false positives.
The "does this superglobal use need a nonce ?" check was previously contained in the `process_token()` method and the "must the nonce check be done before or is it allowed after ?" check was done in the `has_nonce_check()` method. With an eye on expanding the number of checks in this field, I'm moving the logic for these two checks to a new `needs_nonce_check()` method which will either return (string) `'before|after'` or `false` when no nonce check is needed. As the class isn't `final`, I'm making this a `protected` method (instead of `private`) to allow some flexibility for child classes. Note: I'm deliberately _not_ moving the (new) "is OO property" check to the `needs_nonce_check()` method as in that case it isn't a superglobal in the first place, so not a variable this sniff should look at.
…ce verification method early As things were, if the superglobal being examined was _in_ a call to one of the nonce verification functions, the sniff would still loop from the start of the scope (start of the function/file) to the current token before coming to that conclusion. This is, of course, highly inefficient for long functions/large files. I've been in two minds about whether to add this check to the `needs_nonce_check()` method or the `has_nonce_check()` method. For optimal efficiency, the best place is at the start of the `needs_nonce_check()` method as it will prevent having to do all the other "is in type check, is in sanitization function" checks, which are redundant if the call is a nonce check itself. And after all, if the current superglobal is _in_ a nonce check, it doesn't _need_ a nonce check as it already has one. To allow for caching the found nonce check, the gathering of the basic information needed for the caching has to be moved to the `process_token()` method though, so both the `needs_nonce_check()` method, as well as the `has_nonce_check()` method now get an extra `$cache_keys` parameter. It also means that the lists of custom and known nonce verification functions have to be merged earlier, so the call to `$this->mergeFunctionLists()` has been moved up.
…key without nonce verification If all that's being done is to unset a value, there is no risk, so let the sniff stay silent. Includes tests. Note: I've checked in with the security team to confirm this is the correct behaviour. Fixes 1981
…ator This is already being handled correctly as the WPCS `VariableHelper::is_comparison()` method (optionally) treats null coalesce the same as other comparison operators. This behaviour is now safeguarded with a test.
Assignments to a superglobal using the PHP 7.4 null coalesce equals operator should be treated the same as if the variable was used in an `isset()` or with a `??` (PHP 7.0 null coalesce) operator. I.e. as long as there is a nonce check _before_ or _after_, it's fine. As things were, null coalesce equals was treated the same as an assignment (which does not need nonce verification), even though the value of the superglobal key may not be overwritten due to the use of null coalescence. To fix this, I'm adding a new `$include_coalesce` parameter to the `VariableHelper::is_assignment()` method to allow the method to include/exclude the null coalesce equal operator based on the value passed to that parameter. The default value of the parameter is `true` which maintains the pre-existing behaviour of the function. This mirrors the similar method signature and behaviour in the `VariableHelper::is_comparison()` method. The `NonceVerificationSniff::needs_nonce_check()` method will now call that method twice, once to exclude variable assignments (with the exception of null coalesce assignments) and once to allow null coalesce equals assignments to set the expectation for the nonce verification to "after". This may seem inefficient (and is in a way), but the footprint of the `VariableHelper::is_assignment()` method is small and the duplicate code we'd need to handle this differently not worth the effort. Includes tests safeguarding the change.
…ermining search scope As arrow functions are "open", we should not limit a search for a nonce verification to the scope of the arrow function. Includes tests documenting the behaviour.
…nction when searching As an arrow function is a callback and we don't trace whether a callback is called, any nonce check found within an arrow function should be disregarded. This mirrors the sniff behaviour with regards to closures, meaning nonce checks in nested arrow functions and in nested closures will now be treated the same. Includes test.
This just adds a test documenting the behaviour in combination with `match` constructs.
The use of `$_POST` in the `default` branch should realistically be flagged as the nonce check will not have been executed (as it is executed conditionally, only for `'value'`.
However, this is a structural flaw in the sniff as a whole, as it doesn't check at all whether a nonce check is done conditionally or not.
Example:
```php
function test() {
if ( $foo ) {
wp_verify_nonce( $_POST['nonce'] );
} else {
echo $_POST['key']; // Should be flagged as nonce check will definitely not have run, but isn't.
}
echo $_POST['key']; // Should potentially be flagged, depending on value of $foo, but isn't.
}
```
If would be very hard to fix this as a lot of conditions involve variables and we don't have access to runtime values, so it would, in most cases, be very hard to determine whether a conditionally run nonce check will have run.
With that in mind, I think allowing the sniff to treat `match` the same as other conditional constructs are currently treated is the correct behaviour for now.
If/when at some point in the future someone would attempt to make the conditionally run nonce check more precise, `match` should be handled the same as all other control structures.
Just like other nested constructs, the search for a nonce check should limit itself to the current scope and skip over nested closed scopes/not look outside the current scope. This is already handled correctly for enums due to the sniff using the PHPCSUtils `Collections::closedScopes()` method. This just adds tests to safeguard this.
…ive-coding VariableHelper::is_comparison(): minor defensive coding tweak
Security/NonceVerification: use PHPCSUtils, modern PHP + other bug fixes
Looks like between my last WP Core scan and now, `$catID` is back... _sigh_
…eyed lists Since PHP 7.2, lists can also contain keys and the double arrow in those will tokenize as `T_DOUBLE_ARROW`, same as for arrays. An array value can contain a list assignment, so we need to take this into account. The PHPCSUtils `Arrays::getDoubleArrowPtr()` method does so correctly since PHPCSUtils 1.0.7, so upping the minimum supported PHPCSUtils version in combination with switching to the PHPCSUtils method for retrieving the double arrow will fix this. Includes removing the "initial check" for a double arrow as that only serves to cause the array to be walked twice when an arrow is found, so it's more efficient to not do this initial check. Includes tests safeguarding the issue with lists. :point_right: Reviewing with _ignore whitespace changes_ recommended.
When a superglobal is used in the match return expressions, it should be validated, unslashed and sanitized. This is already handled correctly, this just adds a test to safeguard this.
…informative Previously, the error would read `$_POST data not unslashed...`. Now, the error message will show the exact variable with keys we're talking about like `$_POST['key'] not unslashed...`.
…ut-use-phpcsutils-and-more-fixes
... which isn't that often...
As pointed out by GaryJones, as all utility methods have now been made self-contained, it is no longer necessary to have a separate `initi()` method in the `Sniff` class.
…arations-when-possible Helpers: add type declarations whenever possible
Sniff: remove `init()` method
It's the only error flagged by a non-inclusive terminology checker.
This commit updates the README guide to be in line with WordPressCS 3.0.0.
This commit:
* Removes the "Last commit to stable" and "Nr of contributors" badges.
* Updates the "Tested on PHP..." badges.
* Updates the table of contents to match the new content of the file.
* Removes the "Project history" section (has been moved to the wiki).
* Makes the pre-requisites for the package more explicit and puts them in a separate section.
* Updates the installation instructions and adds update instructions.
- Removes the instructions related to stand-alone installs and using `composer create-project`.
- The new installation instructions take the changes in Composer 2.2 (permission required for plugins to run) into account.
- The new installation instructions include a link to the end user/ruleset maintainer upgrade guide for WPCS 3.0.0 as the initial upgrade may need extra work.
* Updates references to the Composer PHPCS plugin to make sure these reference the new name and link to the correct GH repo (as the repo has moved).
* Removes the reference to the VIP ruleset which was removed in WPCS 2.0.0 from the "Rulesets" section.
* Adds links to the "customizable properties" section for where to find customizable properties for PHPCS native sniffs and sniffs from PHPCSExtra.
* Adds the `VariableAnalysis` standard and the `VIP Coding Standards` to the list of optional additional rulesets to use.
* Updates the command and output for the example run.
* Removes the links to IDE related tutorials in favour of a link to the wiki (to save us having to maintain this list in multiple places).
* Ensures the text consistently uses `WordPressCS` instead of `WPCS`.
Closes #481
Closes #1821
Closes #1901
This commit updates the CONTRIBUTING guide to be in line with WordPressCS 3.0.0. This commit: * Updates the information about usage of upstream sniffs and reporting upstream issues. * Updates the information on creating a pull request. * Removes the information about the use of a long running release PR (as we don't do this anymore in practice). * Updates the information on running the unit tests, including the information on how to get set up with cloned dependencies. * Updates the examples test run output and removes the outdated the asciinema image. * Updates the code fragments and example output used in the tutorial part to be in line with the current code and run instructions. * Ensures the text consistently uses `WordPressCS` instead of `WPCS`. * Replaced some ablist language.
Change `WPCS` to `WordPressCS` in miscellaneous files.
…ntributing-et-al-3.0.0 Update documentation (README, CONTRIBUTING) for release of WordPressCS 3.0.0
GaryJones
approved these changes
Aug 20, 2023
There was a problem hiding this comment.
This is the culmination of an unbelievable amount of work by one volunteer. It's taken 3 years, but the changes made in relation to WordPressCS also help the wider PHP community as well.
It sets up the WordPress community for checking coding standards (including "modern" code that WordPress Core itself can't use yet) with a high degree of confidence in avoiding false positives and false negatives.
Congratulations Juliette! 👏🏻 🙇🏻
…nction-name Tests: Remove redundant placeholder function
This changelog is based on going through every single PR merged in the WordPressCS 3.0.0 milestone. The changelog includes only limited, high-level information about the internal changes. More detailed information about the changes to the internals have been included in the upgrade guide for developers/extenders. This means that things like removing a method from a class which is now `final` has not been listed in the changelog (as the `final` keyword prevents extension anyway, so the method removal is not relevant for extenders). In contrast to earlier changelogs, I have chosen to credit people who have contributed to the release with "Props" mentions. Note: the "Props" have only been added for contributions from people outside of the WPCS team.
|
Congrats for the release! |
|
Trac ticket for WP Core: https://core.trac.wordpress.org/ticket/59161 + the actual patch: WordPress/wordpress-develop#5047 |
|
Make post is published: https://make.wordpress.org/core/2023/08/21/wordpresscs-3-0-0-is-now-available/ |
|
Release toot: https://phpc.social/@jrf_nl/110928334999599191 |
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.
Please do add approvals if you agree as otherwise we won't be able to release.
PR for tracking changes for the 3.0.0 release. Target release date: Monday August 21, 2023.
Release checklist
General
composer.json- N/AMinimumWPVersionTrait::$default_minimum_wp_versionand if so, action it - PR Update theminimum_wp_versionto WP 6.0 #2321✏️ Make sure the "last updated" annotation in the docblocks for these lists has also been updated!
List based sniffs:
WordPress.WP.ClassNameCase- PR WP/ClassNameCase: update the class lists based on WP 6.3-RC1 #2317WordPress.WP.DeprecatedClasses- PR WP/DeprecatedClasses: update the class list based on WP 6.3-RC1 #2318WordPress.WP.DeprecatedFunctions- PR WP/DeprecatedFunctions: update the functions list based on WP 6.3-RC1 #2319WordPress.WP.DeprecatedParameters- PR WP/DeprecatedParameters: update the list based on WP 6.3-RC1 #2320WordPress.WP.DeprecatedParameterValues- PR WP/DeprecatedParameterValues: support modern PHP + update for WP 6.1 #2174 (no further update was needed)$allowed_core_constantsinWordPress.NamingConventions.PrefixAllGlobals- PR NamingConventions/PrefixAllGlobals: update the constants list #2315$pluggable_functionsinWordPress.NamingConventions.PrefixAllGlobals- PR NamingConventions/PrefixAllGlobals: allow non-prefixed declarations for pluggable functions and classes #2286$pluggable_classesinWordPress.NamingConventions.PrefixAllGlobals- PR NamingConventions/PrefixAllGlobals: allow non-prefixed declarations for pluggable functions and classes #2286$target_functionsinWordPress.Security.PluginMenuSlug- N/A$reserved_namesinWordPress.NamingConventions.ValidPostTypeSlug- PR NamingConventions/ValidPostTypeSlug: update the reserved post types list #2316$wp_time_constantsinWordPress.WP.CronInterval- N/A$known_test_classesinIsUnitTestTrait- PR IsUnitTestTrait: update test class list #2050Release prep
✏️ Remember to add a release link at the bottom!
README(if applicable) - PR Update documentation (README, CONTRIBUTING) for release of WordPressCS 3.0.0 #2367Release
main(careful, GH defaults todevelop!) & copy & paste the changelog to it.✏️ Check if anything from the link collection at the bottom of the changelog needs to be copied in!
developto be equal tomain.After release
Publicize
Closes #1877