Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PEAR/FunctionCallSignature: minor tweaks + extra test #3667

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Sep 17, 2022

Originally reported in WordPress/WordPress-Coding-Standards#2083

The fixer for the OpeningIndent error for multi-line function calls could inadvertently remove a PHP close tag (or other inline HTML content) on the previous line if the first non-whitespace token on a line was T_INLINE_HTML. In the case of a PHP close tag, this would cause a parse error in the file.

This parse error would then result in the file being incorrectly tokenized for the second fixer loop, with the <?php after the inline HTML being tokenized as below, which causes problems with other sniffs:

 13 | L1 | C 43 | CC 1 | ( 0) | T_WHITESPACE               | [  3]: ⸱⸱⸱
 14 | L1 | C 46 | CC 1 | ( 0) | T_LESS_THAN                | [  1]: <
 15 | L1 | C 47 | CC 1 | ( 0) | T_INLINE_THEN              | [  1]: ?
 16 | L1 | C 48 | CC 1 | ( 0) | T_STRING                   | [  3]: php

Fixed now.

Includes unit test.

Includes minor defensive coding fix ($first !== false) on line 345 and adding of an inline comment to clarify the code within the fixer.

@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 17, 2022

Note: the test failure on PHP 8.2 is unrelated to this PR and also happening on master.

Also note that this PR will conflict (on the tests) with PR #3636. I will rebase and fix the conflict depending on which PR gets merged first.

@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 17, 2022

Note: the test failure on PHP 8.2 is unrelated to this PR and also happening on master.

This is related to the PHP 8.2 disjunctive normal types feature which changed the tokenization - see: php/php-src#9512

This needs to be fixed when support for DNF is added to the PHPCS tokenizer.

Copy link
Contributor

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the fix for the original WPCS report, and all seems well. ✅

@jrfnl jrfnl changed the title PEAR/FunctionCallSignature: bug fix - fixer created parse errors PEAR/FunctionCallSignature: bug fix - fixer creates parse errors Sep 19, 2022
@jrfnl
Copy link
Contributor Author

jrfnl commented Sep 19, 2022

Fixed by commit 9445108 in response to issue #3666, though this PR still contains some minimal differences (defensive coding, comment + minor difference in the fix) which may be worth having a look at.

@gsherwood
Copy link
Member

This is related to the PHP 8.2 disjunctive normal types feature which changed the tokenization

Found myself here while looking into the failure. This looks to caused by tokenization of function calls changing when the name is readonly:

$var = readonly()

So where the string readonly used to be the token T_STRING, it's now T_READONLY. The code still runs fine so I'm not sure if the change was intentional.

@jrfnl
Copy link
Contributor Author

jrfnl commented Oct 21, 2022

This is related to the PHP 8.2 disjunctive normal types feature which changed the tokenization

Found myself here while looking into the failure. This looks to caused by tokenization of function calls changing when the name is readonly:

$var = readonly()

So where the string readonly used to be the token T_STRING, it's now T_READONLY. The code still runs fine so I'm not sure if the change was intentional.

The change in PHP was intentional to support disjunctive normal types for readonly properties, see the issue I linked above.

class Dnf {
    private readonly (B&C)|A $d;
}

@jrfnl
Copy link
Contributor Author

jrfnl commented Oct 21, 2022

(and yeah, DNT is not going to be fun to sort out in the tokenizer.... I have a feeling we'll probably need to add T_OPEN_TYPE_PARENTHESIS and T_CLOSE_TYPE_PARENTHESIS tokens or something as otherwise the chances of sniffs which check spacing around "normal" parentheses will all start triggering on DNT, while I imagine people may want to special case those for the spacing around and inside the parentheses.)

Minor defensive coding and documentation improvements + an additional unit test.
@jrfnl jrfnl force-pushed the feature/pear-functioncallsignature-prevent-fixer-creating-parse-error branch from b5aaa31 to a63f040 Compare March 2, 2023 03:54
@jrfnl
Copy link
Contributor Author

jrfnl commented Mar 2, 2023

Fixed by commit 9445108 in response to issue #3666, though this PR still contains some minimal differences (defensive coding, comment + minor difference in the fix) which may be worth having a look at.

Rebased to make the remaining changes mergable.

@jrfnl jrfnl added this to the 3.8.0 milestone Aug 6, 2023
@jrfnl jrfnl removed this from the 3.8.0 milestone Aug 6, 2023
@jrfnl jrfnl changed the title PEAR/FunctionCallSignature: bug fix - fixer creates parse errors PEAR/FunctionCallSignature: minor tweaks + extra test Aug 27, 2023
@jrfnl
Copy link
Contributor Author

jrfnl commented Dec 5, 2023

Closing as replaced by PHPCSStandards/PHP_CodeSniffer#134

@jrfnl jrfnl closed this Dec 5, 2023
@jrfnl jrfnl deleted the feature/pear-functioncallsignature-prevent-fixer-creating-parse-error branch December 5, 2023 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants