Release 2.2.0 #578
Merged
Release 2.2.0 #578
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
Docs: Changelog for 2.1.0
Exclude files from release archives. This will also make them unavailable when using Composer with `--prefer-dist`. If you develop for VIPCS using Composer, use `--prefer-source`. See https://blog.madewithlove.be/post/gitattributes/
Helps to auto-tag the repo maintainers for reviews when PRs are created.
chore: Add CODEOWNERS file
chore: Add `.gitattributes` file
Introduces a new dependency of `sirbrillig/phpcs-variable-analysis` which is a more maintained version of the existing `VariableAnalysis` sniff in VIPCS. Requires ^2.8.3, since 2.6.3 – 2.8.2 had PHP 5.6 as the minimum version, but 2.8.3 changes it down to PHP 5.4 as the minimum. The new sniff is added to the `WordPressVIPMinimum` ruleset. `WordPress-VIP-Go` ruleset inherits this, and maintains its dropping of the severity for `UnusedVariable` and `UndefinedVariable` violations. The ruleset tests have set the previously undefined variables (or changed some variable names), so that violations on each line are still specific to the thing being tested, and doesn't include extra violations due to the use of throwaway variables. One of the reasons for changing was the previously false positive with respect to the use of `$this` inside closures inside class methods. A ruleset test has been added to cover this, and it correctly does not get flagged in the ruleset test. The old `VariableAnalysis` has been marked as deprecated, until it can be removed in the next major release of VIPCS. This will stop any breakage of setups which reference `WordPressVIPMinimum.Variables.VariableAnalysis` in a custom ruleset. If called, it throws a warning, and then passes control to the new VariableAnalysis sniff. The deprecation notice is excluded if the sniff is not explicitly included, and the duplicate violation messages are also silenced. The previously used `VariableAnalaysisHelper.php` has been removed completely, as no-one should have been referencing that directly. Fixes #449.
Switch to sirbrillig/phpcs-variable-analysis
As the "trusty" environment is no longer officially supported by Travis, they decided in their wisdom to silently stop updating the PHP "nightly" image, which makes it next to useless as the last image apparently is from January.... This updates the Travis config to: * Use the `xenial` distro, which at this time is the default. * Sets the distro for low PHP versions explicitly to `trusty`.
Travis: change from "trusty" to "xenial"
As GH was so kind as to point me to the "contributing guidelines which have been changed since you last contributed" I quickly scanned through the file and most notably, came across a link to the WPCS wiki page, while this link should have gone to the VIPCS wiki. As I was editing the file now anyway, I've made some other changes along the way. * Ask people to search the issue list before opening an issue. * Made a little table to make it easier for people to report upstream issues to the correct repository. * Updated the "custom properties" link to point to the VIPCS wiki. * Updated the PHPUnit installation link. * Updated the `phpunit.xml` example to be in line with the current `phpunit.xml.dist` file. * Updated the expected unit test output.
1. According to the `composer.json` file, the minimum supported PHP version is PHP 5.4, so PHPCompatibility should be set to check for that, not for PHP 5.6 and up.
2. Excluding a complete sniff instead of including it twice and excluding the single errorcode.
This is more efficient as it prevents the sniff from running completely instead of letting the sniff run and silencing the error at the moment it is being thrown.
This is probably just needed for me, but when running the ruleset tests locally on Windows, they wouldn't run and would show a `'$PHPCS_BIN' is not recognized as an internal or external command, operable program or batch file.` error. By not relying on (linux) CLI variable expansion, but expanding the variable to its real path value and using the literal value within the script, the tests can be run on Windows too.
CS ruleset: minor tweaks
RulesetTest: fix compatibility with Windows
…walk()` ... and `Walker::paged_walk()`. The method signature of both the `Walker::walk()` method as well as the `Walker::paged_walk()` method were changed in WordPress 5.3 to make the variadic nature of the allowed arguments explicit. This changes update the sniff to verify the signature of these methods in child classes against the new method signature as it is in WordPress Core. Refs: * https://make.wordpress.org/core/2019/10/09/wp-5-3-introducing-the-spread-operator/ * https://core.trac.wordpress.org/changeset/46442 Fixes 448
…-walk-signature DeclarationCompatibility: fix incorrect signature check for `Walker::walk()`
The builds on PHP 5.4, 5.5 and 5.6 should have been failing on the ruleset test for a while now, but aren't. Example: https://travis-ci.org/github/Automattic/VIP-Coding-Standards/jobs/710513865#L311-L315 What is happening here is that when several commands are run via a script, Travis will only look at the exit code of the last script to decide whether the build should be failed or not. This means that failures from the `WordPressVIPMinimum` ruleset test were never failing the build as they were hidden by a passing ruleset test for the `WordPress-VIP-Go` ruleset. By chaining the commands together, the exit codes of both commands are taken into account when determining whether the build should be failed or not.
Ok, this is a fun one. Line 5 of the ruleset-test is not supposed to error. It is just setting up the variables for the VariableAnalysis test. Line 7, however, is supposed to error on a PHP parse error (`Generic.PHP.Syntax`). The above is true and working for PHP 7. However on PHP 5, line 7 will not throw a parse error, while line 5 actually will - `Cannot re-assign $this`. To fix this, I've: * Replaced the `$this` variable assignment on line 5 with an assignment to `$stdClass`. * Replaced the PHP 7 parse error on line 7 with a parse error which will error on all currently known PHP versions. Fun times ;-)
Turns out the test script was not set up to be able to deal with `0` values and would throw a very informative "_Expected 0 errors, found 0 on line 7._" error in such a case. Fixed now.
…ling-rulesettest
... instead use the PHP version used when this script was called. `shell_exec()` opens a new process and that process does not inherit the PHP executable which was used to run the tests, but uses the system default PHP version instead, which means that the PHPCS command may run on a different PHP version than the tests. The current solution is a simple one and not exhaustive, but should suffice in most cases. If a more complex/more comprehensive solution is desired, I'd recommend using the `PhpExecutableFinder` from the `Symfony\Process` package.
…em-default-php RulesetTest: don't use the system default version of PHP
In 485, three tests were commented out because they seemed to break the ruleset tests.
At the request of Gary, I've had a look see to try and find the underlying cause.
Turned out to be a combination of three things:
1. A new warning having been introduced in PHP 7.4 about using array access on non-arrays.
2. The code in the test case file being incomplete/incorrect - required parameters for function calls were missing.
3. The WPCS `EscapeOutput` not having enough defensive coding for when a faulty function call without parameters would be encountered.
Obviously we can't do anything about 1.
As for 3, I've opened a PR in WPCS upstream to fix this, but that fix won't be available to VIP until WPCS 3.0.0 has been released and VIPCS upgrades.
See: WordPress/WordPress-Coding-Standards 1939
So, in the mean time, let's make sure the function calls in the test case file which aren't intended to trigger the `EscapeOutput` sniff, don't trigger it.
And that's what's done in this PR.
---
To reproduce my findings / How to investigate this in the future:
* Make sure you are set up to use PHP 7.4.x (or whatever other PHP version where the issue was first seen).
* Run `phpcs -ps --severity=1 --standard=WordPressVIPMinimum ./WordPressVIPMinimum/ruleset-test.inc`
* Take note of the missing errors/warnings.
* Scroll to the top of the full report and see the cause:
```
1 | ERROR | An error occurred during processing; checking has been aborted. The error message was: Trying to access array offset on value of type bool
| | in path/to/WordPress/Sniffs/Security/EscapeOutputSniff.php on line 198 (Internal.Exception)
```
Ruleset Tests: revert 485 and fix it
Oops... that line isn't needed anymore.
Now the test is actually running under the proper namespace, it comes to light that the test for the `exclude` property is not working as it is using an outdated syntax. Fixed now.
* Remove some code duplication by using an interim variable. * Check that the PHPCS native bootstrap file can be found. * Remove the question about using PHPCS 3.x. That's not in question anymore. * Fix the link to the contributing.md file - this was now pointing to the WPCS one. * Set the `PHPCS_IGNORE_TESTS` environment variable automatically to the correct value to prevent running tests from other standards.
This adds a second unit test which is based on the actual code which originally triggered the error. The error was caused by the code in question using inline control structures (without braces) for the early main query check. After the previous fix, that code would now throw a false positive. I've fixed this now by adding an additional check for a `return` statement straight after the parenthesis closer of the `if()` statement. Fixes 499
…support-short-arrays Hooks/AlwaysReturnInFilter: add support for hook-ins using short arrays
As VIPCS is currently using WPCS 2.x, we can use the WPCS `Sniff::find_array_open_close()` method to get the opener/closer for an array independently of the type of array (long/short). Once VIPCS implements PHPCSUtils, this method call should be swopped out for the PHPCSUtils `Arrays::getOpenClose()` method. Addresses #358 for the `PreGetPosts` sniff. Includes unit tests.
…ort-arrays Hooks/PreGetPosts: add support for hook-ins using short arrays
Disregard when the existence of a restricted variable is being checked. This uses the upstream WPCS `Sniff::is_in_isset_or_empty()` method.
This means that variables will not be reported as "used" when they are wrapped in a call to:
* `isset()`
* `empty()`
* `array_key_exists()`
This also means that the `if ( isset( $_SERVER['REMOTE_ADDR'] ) ) {` test on line 13 will no longer report a warning.
As `$_SERVER['REMOTE_ADDR']` was then no longer tested for and `$_SERVER['HTTP_USER_AGENT']` was being tested twice (line 14 and line 28), I've changed the occurrence on line 14 to use `$_SERVER['REMOTE_ADDR']` to make sure both are still tested.
Includes additional unit test for the case as reported by the op.
Fixes 568
In contrast to WPCS, disallow the use of Yoda conditions.
Fixes made with the auto-fixer available in the Slevomat standard via the `SlevomatCodingStandard.ControlStructures.DisallowYodaComparison` sniff and visually reviewed.
…sregard-when-in-isset-or-empty RestrictedVariables: don't report on "use" in `isset()`
* PHPUnit config: add code coverage configuration.
By default, when there is a code coverage configuration and Xdebug is enabled, code coverage will be generated.
For now, code coverage is set up for local use with an HTML code coverage report being generated in a `build/logs/` directory.
Note: generating code coverage is slow.
* Git ignore the `build` directory as created by PHPUnit to store the log files.
* Adjust the "normal" test script to not generate code coverage information.
* Add a `bin/unit-tests-coverage` to run the unit tests with code coverage enabled.
* Add a Composer script to call the `unit-tests-coverage` script.
... to all unit test files, as well as enable strict coverage recording.
We don't require or require-dev any packages that are not stable, so no need to continue allowing RC at this time. Fixes #562.
Composer: Change minimum stability to stable
Recognise that a form `action` attribute expects to have a URL value, and therefore expected to be escaped with `esc_url()`, rather than `esc_attr()`. Fixes #554
Docs: Update notes for releasing
…per-escaping-function ProperEscaping: Fix message for action attribute
…-props Docs: Add comparisons and props for old versions
Change log for VIPCS 2.2.0
pkevan
approved these changes
Sep 7, 2020
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.
PR for tracking changes for the 2.2.0 release. Target release date:
Wed 09 September 2020Mon 07 September 2020.Remaining work for this Milestone
master.