8000
Skip to content

fix: make BinaryOperatorSpacesFixer work as pre-v3.47 - #7751

Merged
keradus merged 1 commit into
PHP-CS-Fixer:masterfrom
Wirone:codito/revert-7593
Jan 16, 2024
Merged

fix: make BinaryOperatorSpacesFixer work as pre-v3.47#7751
keradus merged 1 commit into
PHP-CS-Fixer:masterfrom
Wirone:codito/revert-7593

Conversation

@Wirone
@Wirone Wirone commented Jan 16, 2024
Copy link
Copy Markdown
Member

Revert of #7593

Fixes #7745
Fixes #7748

…ple shifts occurs in single line (PHP-CS-Fixer#7593)"

This reverts commit edce2ad.
@Wirone Wirone added the BLOCKER Extremely important changes required for all other ongoing changes label Jan 16, 2024
@Wirone
Wirone requested a review from keradus January 16, 2024 10:15
@Wirone Wirone self-assigned this Jan 16, 2024
@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 94.753% (-0.01%) from 94.764%
when pulling 04da3e4 on Wirone:codito/revert-7593
into bf48c81 on PHP-CS-Fixer:master.

@Wirone Wirone changed the title fix: make BinaryOperatorSpacesFixer work as pre-v3.47 fix: make BinaryOperatorSpacesFixer work as pre-v3.47 Jan 16, 2024
@keradus
keradus merged commit 66d2e15 into PHP-CS-Fixer:master Jan 16, 2024
@Wirone
Wirone deleted the codito/revert-7593 branch January 16, 2024 12:25
@wadakatu
Copy link
Copy Markdown
Contributor

@Wirone @keradus

Thank you for creating the revert PR and merging it. I apologize for the bug that I introduced.
I need to gain a better understanding of BinaryOperatorSpacesFixer to resolve the issue.

@Wirone
Wirone commented Jan 16, 2024
Copy link
Copy Markdown
Member Author

@wadakatu no problem. The issue there was that the logic you added was not based on tokens, but on single string that represented the line of code. I should catch it before, so I believe it's my fault it went live. The fixer just can't do a regex replacements on full string because - as reported snippets show - it may replace "operator" found inside a string. It must iterate through tokens and modify whitespace around actual operator.

@VincentLanglet
Copy link
Copy Markdown
Contributor

To avoid futur regression, is a PR planned (or is it already done) to add one of the two issues fixed in the tests ?

Fixes #7745
Fixes #7748

@Wirone
Wirone commented Jan 16, 2024
Copy link
Copy Markdown
Member Author

I did not think about it, to be fair, but it seems like a good idea. It isn't done yet, but how would you like to achieve it? These particular failures were strictly related to the invalid implementation which was reverted. Of course, we can add these reported snippets, but it does not cover every possible case.. 🙂

@VincentLanglet
Copy link
Copy Markdown
Contributor

how would you like to achieve it? These particular failures were strictly related to the invalid implementation which was reverted. Of course, we can add these reported snippets

This is what I had in mind.
Adding this will ensure you that the next PR which will try to solve #5605 won't reproduce again the same regression.
Introducing a bug once is human, introducing the same bug twice is silly ^^

I know it's done in PHPStan codebase, every single issue is added as a test case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BLOCKER Extremely important changes required for all other ongoing changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PHP-CS-Fixer 3.47.0 changes literal strings After update v3.47 binary_operator_space has changed

5 participants

0