[xml doc] RemovedPCREModifiers #1272
Closed
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
Contributor
afilina
commented
Mar 25, 2021
afilina
commented
- Give a general solution.
- Provide before/after example.
jrfnl
reviewed
Mar 25, 2021
There was a problem hiding this comment.
Hi Anna, awesome to see the first docs PR!
As I expect this sniff to only need one doc as it only covers one change, the file name should be PHPCompatibility/Docs/ParameterValues/RemovedPCREModifiersStandard.xml so it will also be usable from the command-line for those people who use that PHPCS feature.
The title should probably also be filled in, a human readable version of the sniff name is the usual go-to for that. In this case, that would be Removed PCRE Modifiers.
The command-line docs generator has a limited width, so the code may need some layout tweaking to also look good on the command-line.
This is what it would currently look like (red circles added by me):
You can see it for yourself by running [vendor/bin/]phpcs --standard=PHPCompatibility --generator=Text from the root of the project.
Once the docs are merged and in a released version, anyone who has PHPCompatibility installed in their project can run that command from the root of their project to see the same.
Other than that, you can probably leave out the <?php in the code samples.
While PHPCS currently still supports PHP, CSS and JS, the majority of sniffs out there are for PHP and CSS and JS support will be dropped in the next major.
Oh and you may want to rebase the PR on develop. Looks like your branched off master now, which is the release branch.
|
@jrfnl I see this becoming a problem with some sniffs, such as those that deal with removed functions. One sniff detects a great many removed functions, so trying to fit docs for all of them into one file would be unreasonable. Just the removal of |
I totally agree. I thought we discussed this in our call a while back ? From what I remember, we discussed doing it like this:
My feedback was based on that, as I would consider this particular one, one of the relatively "simple" sniffs as it focuses on one particular issue. (not simple in internal sniff logic, but that's beside the point). |
The parameters in the function were passed in the wrong order, so the condition would never match on a PHP `8.x` version. Ref: https://docs.github.com/en/actions/reference/context-and-expression-syntax-for-github-actions#startswith
…e/ghactions-minor-fix GH Actions: minor fix in condition
PHP 8.1 deprecates passing `null` to PHP native functions where the parameter(s) is not explicitly nullable. While in PHP 8.1 this is only a deprecation, it should, of course, still be fixed. This commit contains a fix for the one issue in PHPCompatibility found related to this so far (based on the unit tests). Passing `null` to `trim()` is now deprecated, so more defensive coding or explicit type casting is needed. This comes into play for the `TestVersionTrait::getTestVersion()` method. Includes removing a stray line of commented out code.
As it looks like PHPCS 4.x is still quite a while away (2022 at the earliest), let's stop testing against PHPCS 4.x for the time being. This should allow build failure reporting to be more accurate, as currently every PR has a failure on PHPCS 4.x due to a bug in some of the new code in 4.x (fix for this was pulled six months ago and still not merged). The build against PHPCS 4.x should be re-enabled closer to the PHPCS 4.x release. Note: PHPCSUtils will continue to test against PHPCS 4.x and will update the provided utilities ahead of time, so with a bit of luck, by the time the build against PHPCS 4.x is re-enabled, the build should largely pass thanks to the compatibility layers in PHPCSUtils.
…e/ghactions-disable-testing-against-phpcs-4.x GH Actions: don't test against PHPCS 4.x (yet)
…1/various-compatibility-fixes PHP 8.1 compatibility: fix deprecation notice
…FunctionParametersSniff
This is a technical refactor and contains no significant functional changes.
The `OptionalToRequiredFunctionParameterSniff` class previously extended the `RequiredToOptionalFunctionParametersSniff` class, which in turn extended the `AbstractComplexVersionSniff`.
The sniff has now been refactored to directly extend the `AbstractFunctionCallParameterSniff` class instead.
The purpose of this refactor is three-fold:
1. Multiple people have reported issues with fatal "class already declared" errors for this sniff extending another sniff.
While this is primarily an issue with the autoloader in PHPCS, the issue can be avoided by not allowing one sniff to directly extend another.
Note: the same problem does not exist when extending an _abstract_ sniff, so this solution will be fine.
2. The (global) function call determination in the `AbstractFunctionCallParameterSniff` sniff is better than the one which was previously contained in the `RequiredToOptionalFunctionParametersSniff`, preventing some potential false positives as demonstrated by the additional unit tests.
3. In the (near) future, PHPCSUtils is expected to contain a version of the `AbstractFunctionCallParameterSniff` which is better still.
By decoupling this sniff from the `RequiredToOptionalFunctionParametersSniff` and by extension from the `AbstractComplexVersionSniff` sniff, we pave the way for the breaking change of removing the `AbstractComplexVersionSniff` class.
The switch over to the PHPCSUtils version of the `AbstractFunctionCallParameterSniff` in itself is not a breaking change and can therefore safely be done in a future minor release.
Note: Aside from the tests still passing, I have also verified that the sniff error codes are not affected by this change, so from a "custom ruleset"/"ignore annotations" point of view this refactor does not contain any breaking changes.
Fixes 608
Fixes 638
Fixes 793
Fixes 1042
…rsionSniff
This is a technical refactor and contains no significant functional changes.
The `RequiredToOptionalFunctionParametersSniff` class previously extended the `AbstractComplexVersionSniff`.
The sniff has now been refactored to extend the `AbstractFunctionCallParameterSniff` class instead.
The purpose of this refactor is two-fold:
1. The (global) function call determination in the `AbstractFunctionCallParameterSniff` sniff is better than the one which was previously contained in the `RequiredToOptionalFunctionParametersSniff`, preventing some potential false positives as demonstrated by the additional unit tests.
2. In the (near) future, PHPCSUtils is expected to contain a version of the `AbstractFunctionCallParameterSniff` which is better still.
By decoupling this sniff from the `RequiredToOptionalFunctionParametersSniff` and by extension from the `AbstractComplexVersionSniff` sniff, we pave the way for the breaking change of removing the `AbstractComplexVersionSniff` class.
The switch over to the PHPCSUtils version of the `AbstractFunctionCallParameterSniff` in itself is not a breaking change and can therefore safely be done in a future minor release.
Note: Aside from the tests still passing, I have also verified that the sniff error codes are not affected by this change, so from a "custom ruleset"/"ignore annotations" point of view this refactor does not contain any breaking changes.
As part of the Attributes feature and as per the Attribute amendments RFC, a new PHP native `Attribute` class has been introduced. Ref: https://wiki.php.net/rfc/attribute_amendments
> Declaring a function called `assert()` inside a namespace is no longer allowed, and issues `E_COMPILE_ERROR`. The `assert()` function is subject to special handling by the engine, which may lead to inconsistent behavior when defining a namespaced function with the same name. Ref: https://www.php.net/manual/en/migration80.incompatible.php This adjusts the existing sniff to take the change to fatal error in PHP 8.0 into account. Note: the error code of the sniff has been changed! This is a minor breaking change and should be annotated in the changelog.
PHP 8.0 introduces Attributes as a form of structured, syntactic metadata to declarations of classes, properties, functions, methods, parameters and constants. Attributes allow to define configuration directives directly embedded with the declaration of that code. The syntax for attributes in PHP has changed a number of times via various RFC amendments. As a result of that, it took a little while as well before the final tokenization of attributes in PHPCS was known, but upstream PR squizlabs/PHP_CodeSniffer 3203 has now (finally) been merged and released as part of PHPCS 3.6.0. This PR introduces a new sniff to detect the use of attributes. This new sniff _only_ focusses on detecting the _syntax_ for attributes being used - `#[...]`. It does not verify whether the _contents_ within the attribute is valid, nor whether the attribute is used in a _supported location_. Invalid attributes like that would be a parse error in PHP 8.0+. It can be argued that single-line attributes should not throw an error, but a warning, as those will not necessarily cause problems when used in code which also needs to run on PHP 7, as they will just be ignored and treated as a comment. This is however not the case for multi-line or in-line attributes, which would cause parse error in PHP < 8.0, so for consistency, I've chosen to let the sniff always throw an error. In rare cases, specifically for the code sample in the last test case, the PHPCS tokenizer cannot prevent the rest of the file becoming mangled on PHP < 8.0. This is similar to the tokenizer problems caused by indented heredocs/nowdocs as introduced in PHP 7.3, where the rest of the file after the new syntax will be tokenized incorrectly. In that case, the sniff will still (correctly) throw an error for the attribute, however detection of (other) issues in the rest of the file will be broken. Also see: squizlabs/PHP_CodeSniffer 3294 As the attributes feature is likely to be expanded in the future, the sniff has been placed in a new dedicated category which will allow for additional attribute related sniffs to be added at a later point in time. If and when new attribute related features are added to PHP, validation of whether an attribute is valid in PHP 8.0 may need to be added to this sniff, but this is a moot point until that time. Includes unit tests. Refs: * https://wiki.php.net/rfc/attributes_v2 * https://wiki.php.net/rfc/attribute_amendments * https://wiki.php.net/rfc/shorter_attribute_syntax * https://wiki.php.net/rfc/shorter_attribute_syntax_change * https://www.php.net/manual/en/language.attributes.php
This sniff can throw an error or a warning depending on whether a `destruct()` method has been found in the class (error) or not, but the class has a parent class or uses traits (warning). This improves the message shown for the `warning` case to make it clearer how to take action on the issue. It also ensure that the error and the warning each have a unique error code.
As per 1163: * Deprecations should throw warnings. * Removals should throw errors. * Both should use a different error code. Note: the `RemovedAssertStringAssertion` sniff is new to PHPCompatibility 10.0.0, so this error code change has no consequences. The error code change for the `RemovedTernaryAssociativity` should be noted in the changelog though as the sniff was introduced in PHPCompatibility 9.2.0. Fixes 1163
... as per the discussion and similar test case upstream in squizlabs/PHP_CodeSniffer 3294.
This was referenced Apr 13, 2021
Closed
This fell through the cracks. Thanks for reminding me. I'll address them as soon as I am able. |
…nly used in return/exit [2] Follow up on 1208. Improve the previously applied fix to bow out from reporting "use" of a passed parameter before a call to `func_get_args()` when the parameter is used in a `return` or `exit` statement. The previous fix would break when the `return`/`exit` statement was within an unscoped condition. Unscoped conditions (no curly braces) are a bane to the existence of sniff writers anywhere and can't be fully accounted for. Even so, there are some common situations in which the previous fix can be improved to reduce false positives. This current fix will skip over `return`/`exit` statements completely, instead of walking back from a matched variable to check if something is within a `return`/`exit` statement. This also makes the sniff more efficient. Secondly, the sniff will now also ignore parameter use in `throw` statements. Includes additional unit tests. Fixes 1240
…r empty() Those uses are innocent, will not change the value or the passed parameter and can be safely ignored. Includes unit tests.
When one of the passed parameters is used in a call to `unset()`, we know for sure it has been changed, so the sniff should throw an error, not a warning. Includes unit test.
…nly used in simple assignment Prevent a false positive (warning) when a parameter is only _assigned_ to another variable and this is a "simple assignment". For the purposes of this check, a "simple assignment" is defined as: - The variable is not nested in parenthesis, i.e. not used in a potential function call which may adjust the variable by reference. - Not preceded by a reference operator (reference assignment). - Has an assignment operator before it and none after. Any more complex statements where a passed parameter is being assigned to another with a call to one of the argument functions after it, will still be reported as "needs inspection". Includes additional unit tests. Fixes 1240 Related to 796
…e/newclasses-add-attribute-class NewClasses: detect Attribute class
…0/removedoptionalbeforerequired-constpropprom-test RemovedOptionalBeforeRequiredParam: add test covering constructor property promotion
…0/removednamespacedassert-upgrade-to-error PHP 8.0 | RemovedNamespacedAssert: adjust for removal in PHP 8.0
…e/1163-message-type-code-review QA: consistency of error types and codes
…0/removeddestructafterconstructorexit-improve-error-msg RemovedCallingDestructAfterConstructorExit: improve warning message
…e/argfunctionsreportcurrentvalue-ignore-isset-empty ArgumentFunctionsReportCurrentValue: various improvements/ ignore isset/empty / error on unset
…e/608-638-793-1042-decouple-required-to-optional-param-sniffs Refactor OptionalToRequiredFunctionParameterSniff and RequiredToOptionalFunctionParametersSniff
…0/new-newattributes-sniff PHP 8.0 | New `NewAttributes` sniff
As things were, coverage would only be run on pull requests and on `master`. Generally speaking, this is sufficient, but it does mean that the coverage over time is no longer properly tracked, which is a shame. The tweaks I'm making now should: * Still run the normal `test` and `coverage` jobs on pull requests and merges to `master. * For merges to `develop`, skip the `test` job, but do run the `coverage`, so it will be tracked again. * Skip the `quicktest` on `develop` as those are the same builds as for `coverage`, so no need to run those twice. * Also make sure that the coveralls-finish will actually run. Should after a build running just coverage already, but didn't.
…e/ghactions-coverage-for-develop GH Actions: tweak coverage run
The PHPUnit 4.x version used was based on a PHPUnit bug previously encountered in 511. It appears though that with changes which have been made to the test set up in the mean time, this bug no longer rears its ugly head, so we can now switch back to using the highest available PHPUnit 4.x version again.
…e/composer-higher-phpunit4-version Composer: tweak the PHPUnit 4.x version used
Turns out the default setting for `error_reporting` used by the SetupPHP action is `error_reporting=E_ALL & ~E_DEPRECATED & ~E_STRICT` and `display_errors` is set to `Off`. For the purposes of CI, I'd recommend running with `E_ALL` and `display_errors=On` to ensure **all** PHP notices are shown. In our script, we already enabled error_reporting, but the error display was not yet fixed. Sorted now.
…e/ghactions-turn-on-error-reporting GH Actions: turn display_errors on
|
@jrfnl Fixed output: |
|
The issue seems with a dependency that is incompatible with PHP 8.1 and not related to this PR. |
afilina
changed the title
You mean the build failures ? For the linting on PHP 8.1 this should now be solved. My PR to fix things has been merged over the weekend and a new version has been tagged. The test failures on PHP 8.1 is another matter. This needs investigation, but most likely is related to PHPCS upstream. Not a blocker for this PR though. |
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.