Skip to content

PEAR/FunctionCallSignature: minor tweaks + extra test#3667

Closed
jrfnl wants to merge 1 commit intosquizlabs:masterfrom
jrfnl:feature/pear-functioncallsignature-prevent-fixer-creating-parse-error
Closed

PEAR/FunctionCallSignature: minor tweaks + extra test#3667
jrfnl wants to merge 1 commit intosquizlabs:masterfrom
jrfnl:feature/pear-functioncallsignature-prevent-fixer-creating-parse-error

Conversation

@jrfnl
Copy link
Copy Markdown
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
Copy Markdown
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 jrfnl mentioned this pull request Sep 17, 2022
Closed
1 task
@jrfnl
Copy link
Copy Markdown
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.

GaryJones
GaryJones reviewed Sep 17, 2022
View reviewed changes
Copy link
Copy Markdown
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 jrfnl mentioned this pull request Sep 19, 2022
Closed
@jrfnl
Copy link
Copy Markdown
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
Copy Markdown
Member

gsherwood 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.

@jrfnl
Copy link
Copy Markdown
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
Copy Markdown
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.)

This was referenced Dec 4, 2022
Merged
Closed
@jrfnl jrfnl mentioned this pull request Dec 14, 2022
Closed
@jrfnl
PEAR/FunctionCallSignature: minor tweaks
a63f040 
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
Copy Markdown
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 mentioned this pull request Jun 7, 2023
Closed
@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 jrfnl mentioned this pull request Sep 21, 2023
Closed
@jrfnl jrfnl mentioned this pull request Nov 9, 2023
Merged
@jrfnl jrfnl mentioned this pull request Nov 24, 2023
Closed
10 tasks
@jrfnl jrfnl mentioned this pull request Dec 5, 2023
Merged
@jrfnl
Copy link
Copy Markdown
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
@jrfnl jrfnl mentioned this pull request Mar 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@GaryJones GaryJones GaryJones left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jrfnl @gsherwood @GaryJones