Release 2.3.0 #665
Merged
Release 2.3.0 #665
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
The builds are failing a little too often for my taste on the below error. ``` [Composer\Downloader\TransportException] Peer fingerprint did not match ``` I'm prefixing the `composer install` commands now with `travis_retry` in an attempt to get round this problem. Ref: * https://docs.travis-ci.com/user/common-build-problems/#timeouts-installing-dependencies
* Adjust the "abstract method" test to:
1. Expect a warning.
2. Prevent the hook in getting confused with other functions in the same test file.
* Remove the "abstract method implementation" test as it wasn't testing anything.
1. The sniff does not look for child classes, so wouldn't examine that code snippet anyway.
Adding this functionality is not that useful either as in most cases, the child class will not be in the same file as the abstract parent class.
2. And even if the sniff did examine it, it would still not recognize it as a child class as the class doesn't `extend` the abstract.
* Add a test for a typical case where declared functions do not have a scope opener/closer due to a tokenizer bug.
PR squizlabs/PHP_CodeSniffer 3066 is open upstream to fix this.
* Add a "live coding" test case.
… error This implements the suggestion made in #580 (comment) If the method a filter callback points to is an abstract method, a warning will be thrown asking for manual inspection of the child class implementations of the abstract method. In case of parse or tokenizer error, the sniff will bow out and stay silent.
Travis: retry composer install on failure
Co-authored-by: Rebecca Hum <16962021+rebeccahum@users.noreply.github.com>
As VIPCS requires two external standards, including the new `VariableAnalysis` dependency, let's require the DealerDirect plugin to make life easier on people who install via Composer. Includes updating the Readme to mention the `VariableAnalysis` standard, as well as mention that the DealerDirect plugin is now a project requirement. Note: I've widened the version constraints for the DealerDirect plugin to prevent conflicts with customer projects which already required the plugin, but potentially at a different version. The version constraints now set cover all released versions which support external standards properly.
The `register()` method tells PHPCS which tokens a sniff is listening for. A number of sniffs were currently registering the [`Tokens::$functionNameTokens`](https://github.com/squizlabs/PHP_CodeSniffer/blob/9cce6859a595b3fa77fc823d9b0d451951dcf9d2/src/Util/Tokens.php#L570-L583) array. In all cases, these sniffs were only looking for specific function calls, and in most cases, a "name" check is the first thing in the sniff code or else such a check was done at a later point in the code. This makes using the `Tokens::$functionNameTokens` array highly inefficient, as the _only_ token within that array which could have the "name" the sniff is looking for is `T_STRING`. The other eleven (!) tokens registered would **never** match and would never trigger an error, but would be passed to the sniff even so. This commit replaces the use of the `Tokens::$functionNameTokens` array in nearly all places in the code base with `T_STRING` for higher accuracy and more efficient sniffing. Note: in a future PR, once PHPCSUtils is used, these sniffs should be adjusted to take the [PHP 8.0 namespaced identifier name tokens](https://wiki.php.net/rfc/namespaced_names_as_token) into account, but that is for later.
…back Ignore return values when a filter callback is abstract
Performance: more selective sniffing for efficiency
…plugin Composer: require the DealerDirect plugin
... as an upstream PR containing a bug fix for the tokenizer issue this test documents has been merged. Ref: * squizlabc/PHP_CodeSniffer 3066
Hooks/AlwaysReturnInFilter: adjust expectations for a test
* Move the extensions against which checks are being run to private properties.
* Removes the `.` from the part in the regex which is being remembered.
Note: The `.` before the extension is still required, it is just no longer _remembered_.
* Changes the extension checks from using the relatively expensive `in_array()` to the lightweight `isset()`.
The regex will work just the same without the `.*`.
* The existing unit tests only tested against `require_once` and `include`, while the sniff looks for all variations.
The newly added unit tests make sure that all four variations have at least one positive/negative test associated with it.
* Add some capitalization variations to the `include|require[_once]` keywords.
* Make sure the `include|require[_once]` keyword is always referred to in lower case in the error message. * Make the error message more informative by displaying the text snippet which triggered the error.
…ension The regex used to retrieve an extension, would retrieve it case-insensitively, but the previous `in_array()` check (now `isset()`), checked the extension in a case-sensitive manner. Whether the file extension is used in upper, lower or mixed case, does not matter to the include as long as a matching file is found, so the sniff should check the extension in a case insensitive manner. As things were, a file name like `file.PHP` would trigger a false positive for the `IncludingNonPHPFile` error and a file called `file.Css` would incorrectly throw the `IncludingNonPHPFile` error instead of the `IncludingSVGCSSFile` error which it should have thrown. Fixed now. Tested by adjusting some of the existing unit tests.
PHAR (PHP Archive) files should be regarded as PHP files for the purposes of this sniff. Includes unit tests. Fixes 478
…trings The sniff looks for `Tokens::$stringTokens` when examining the statement. The `Tokens::$stringTokens` array contains two tokens: 1. `T_CONSTANT_ENCAPSED_STRING` - these are either single or double quoted text strings without variables within them. 2. `T_DOUBLE_QUOTED_STRING` - these are double quoted text strings with interpolated variables in the text string. As things were, the quotes would be stripped off the first, but not off the contents of `T_DOUBLE_QUOTED_STRING`-type tokens. As the regex matches extensions at the very end of the text string and doesn't allow for a double quote at the end, this effectively meant that text in double quoted strings would never be recognized as containing a non-PHP extension. (false negative) Includes unit tests.
…atement An `include`/`require` statement can be used within template files to include another template. In that case, the statement can be ended by a PHP close tag without there ever being a semi-colon. Even though the `findNext()` function call indicated that it only wants to look "locally", the `findNext()` method itself does not take a PHP close tag into account when determining whether the statement has ended. This could be considered a bug upstream, but that's another matter. In practice, this meant that when an `include|require[_once]` statement ended on a PHP close tag, the `findNext()` would continue looking for text string tokens and could report an error on a text string which is unrelated to the `include|require[_once]` statement. (false positive). Determining the end of the statement properly before calling `findNext()` fixes this, however, the `findEndOfStatement()` method will return the last non-whitespace token in a statement, which for statements nested in brackets can be a token which is _part_ of the statement. As the `findNext()` method does not look at the `$end` token, we need to `+1` the result of `findEndOfStatement()` to make sure the whole statement is considered. Includes unit tests.
To prevent an assignment in condition, the same function call was executed twice. By changing the code around slightly and using a `do - while` loop instead of a `while`, the duplicate function call can be removed.
… at end of statement An `include|require[_once]` statement can only include one (1) file and the file extension of that file will normally be at the _end_ of the statement. So far, the sniff would walk from the start of the statement to the end, while walking from the end of the statement to the start, will get us a result more quickly and more accurately. Includes unit tests. The first would be a false negative, the second a false positive without this fix.
… per statement An `include|require[_once]` statement can only include one (1) file, so there should only ever be one file extension in the statement and by extension, only ever be one error for any particular `include|require[_once]` statement. Once a file extension has been found which generates an error, we can therefore stop sniffing that statement. This is similar to the sniff breaking off checking the statement once a text string with `.php` as a file extension is found, as was already done. Includes unit tests.
... to document the behaviour of the sniff for certain situations.
`preg_match()` will return `false` for an invalid regex, `0` for "no match" and `1` for a match as it only looks for one match - in contrast to `preg_match_all()`-. As the regex uses an "end of text string" anchor, using `preg_match()` is the right function, but that also means we can simplify the return value check a little.
The existing text was unrelated to the actual sniff. Probably legacy copy/paste.
In namespaced files, it is a good habit to use fully qualified function calls or `use function ...` statements for global functions to prevent PHP from looking for the function in the current namespace. As things were, fully qualified function calls would be ignored by the sniff, leading to false negatives. Tested by adjusting some existing tests.
The `public` `$escaping_functions` property was never _intended_ for allowing it to be changed via a custom ruleset. As the array _format_ of the property has now changed, adjusted values for this property set in a custom ruleset will no longer work. This is a breaking change, but as: 1. The property was never intended to be changed from a custom ruleset and 2. The chances of any custom rulesets actually existing which _do_ change this property being very, very small... I believe it is justified to make the breaking change in the format in a minor release as the format change provides a real functional benefit. Now, as this breaking change will be made anyway, we may as well do it right and mark the property as `protected` preventing it from ever being changed via a custom ruleset.
ProperEscapingFunction: Flag when esc_attr is being used outside of HTML attributes
... and make two others more specific.
Includes: * Removing severity changes related to these from the VIP Go ruleset. * Removing the sniff tests related to these. * Removing the ruleset tests related to these. Partially fixes 614 Note: "removed" test lines have replaced with blank lines to prevent having to adjust the line numbers for all test after it in the "expected errors/warnings" arrays.
Includes: * Removing severity change related to this from the VIP Go ruleset. * Removing the sniff tests related to these. * Removing the ruleset tests related to these. Partially fixes 614 Note: "removed" test lines have replaced with blank lines to prevent having to adjust the line numbers for all test after it in the "expected errors/warnings" arrays.
### Composer This installs two additional PHP packages in `require-dev`: * [`php-parallel-lint`](https://packagist.org/packages/php-parallel-lint/php-parallel-lint) which allows for linting PHP files in parallel (faster), as well as automatically recursively walking directories. * [`php-console-highlighter`](https://packagist.org/packages/php-parallel-lint/php-console-highlighter) which provides PHP code highlighting in the command line console, allowing the linter to display the results in a more meaningful manner. It also adjusts the existing `bin/php-lint` script to use this new dependency instead of using *nix specific commands. ### GH Actions As it was, PHP linting was only done on PHP 7.4 as part of the `basics` workflow. This commit changes this to run linting on the lowest (PHP 5.4) and highest (latest/PHP 8.0) supported PHP versions as part of the `test` workflow. (And by "supported", I mean _runtime_ support in this case, not _syntax support_.) The running of the tests has been made dependent on the linting jobs passing as if the linting turns up failures, we can be sure that there will be (or should be) failures in the test runs anyway. This new job uses the "CS2PR" tool which will show any errors as feedback in the actual PR code view. It also runs the linter against PHP 8.1 - against which we are currently not yet running the tests - as an early warning system in case of upcoming issues. Ref: * https://github.com/php-parallel-lint/PHP-Parallel-Lint/releases/tag/v1.3.0 * https://github.com/php-parallel-lint/PHP-Console-Highlighter/releases/tag/v0.5
…led code Issue reported by GaryJones in Automattic/VIP-Coding-Standards 628#issuecomment-790653774 In rare cases, it is possible that the call to `eval()` with "safe" tokens only, would still result in a parse error. An example of such a case is when the PHP 5.6 `**` (`T_POW`) operator is used. PHPCS backfills the tokenization, which means that the operator is correctly recognized by the sniff as a "safe" token to use in the `eval()` statement. However, when the sniff is being run on PHP 5.4/5.5, the `eval()` will also be run on PHP 5.4/5.5 and while PHPCS backfills the token, PHP does not, resulting in a parse error in the `eval`-ed code, upon which the `eval()` will return `false`. This commit: * Silences the parse error notice. * Checks the output of the `eval()` for it being boolean `false` and if so, flags the statement for manual inspection, same as when any "non-safe" token is encountered.
…-annotations QA: remove redundant PHPCS ignore annotations
…error-in-eval LowExpiryCacheTime: improve handling of potential parse errors in eval-ed code
…ting GH Actions: switch over to parallel linting of PHP files
…ted-errors RestrictedFunctions/RestrictedVariables: remove usermeta related errors
The `phpcsstandards/phpcsdevtools` package includes a script which can check whether sniffs are feature complete, i.e. whether all sniffs have unit tests and documentation. By adding this check to the `basics` GH Actions workflow, we prevent untested and/or undocumented sniffs from entering the repo. For now, the documentation check is silenced. P.S.: the `PHPCSDevTools` package contains a few more goodies which having the package (dev-)required now make available to developers, like the `PHPCSDebug` standard to get detailed information about the tokens in a file. Have a look at the package [readme](https://github.com/PHPCSStandards/PHPCSDevTools) for more information.
RestrictedFunctionsSniff: Change default message of switch_to_blog()
…plete-check GH Actions: always check that all sniffs are feature complete
The "quicktest" stage will basically run the same tasks as the "test" workflow (linting, unit tests and the ruleset tests), but only against low/high PHP/PHPCS/WPCS combinations. This should catch most issues. This "quicktest" stage will run for pushes and for merges to `develop`. The more comprehensive complete build against a larger set of PHP/PHPCS/WPCS combinations will now only be run on PRs and merges to `master`.
... just as a reminder. Includes minor fix for improved markdown.
GH Actions: add "quicktest" stage for non-PR/merge builds
Release template: add checkbox for dependency check
Update changelog to reflect 2.3.0
Changelog: minor tweak
Changelog: minor tweak
jrfnl
approved these changes
Apr 19, 2021
There was a problem hiding this comment.
Between the work done by @rebeccahum and me, I think this is a valuable release. Time to let people benefit from all the improvements! 🎉
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.
Remaining work for this Milestone
PR for tracking changes for the 2.3.0 release. Target release date: MONDAY 19 APRIL 2021.
master.