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

fix: Tokens::overrideRange() block cache pruning #8240

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Oct 5, 2024

critical fix #8228 as currently Tokens::overrideRange() might leave the block cache corrupted

needed for #8225

@mvorisek mvorisek marked this pull request as ready for review October 5, 2024 11:29
@mvorisek mvorisek force-pushed the fix_tokens_cache_8228 branch from 493d769 to 4092701 Compare October 16, 2024 10:02

Verified

This commit was signed with the committer’s verified signature.
devnexen David CARLIER
@mvorisek mvorisek force-pushed the fix_tokens_cache_8228 branch from 4092701 to 692622d Compare October 26, 2024 06:40
@kubawerlos
Copy link
Contributor

critical fix

can you explain how it is different from a regular fix? It seems to me like unnecessary dramaturgy.

@mvorisek
Copy link
Contributor Author

mvorisek commented Oct 27, 2024

Of course - crtical = core bug that leads to a code corruption. Here the cache was able to get out of sync and thus answering wrong,

I stress this mainly because it should deserve a patch release after it is merged.

@kubawerlos
Copy link
Contributor

Of course - crtical = core bug that leads to a code corruption

What code corruption? Fixer does not corrupt any file.

I stress this mainly because it should deserve a patch release after it is merged.

"Should"? Does some international law of bugfixing dictate this? I don't think so, then not "should", but "would be nice" or something. Every time you use the phrase "should" (or "must etc.) - and you do it quite often - you sound like someone ordering others (e.g. maintainers) what they are obligated to do, but, hello 👋🏼, this is open source, a place where all people are doing what they are doing in their free time, for free.

@mvorisek
Copy link
Contributor Author

I would love to get this PR integrated ❤

Copy link
Member

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

@mvorisek in our contribution guide you can see:

if you work on a bug fix, please start with reproducing the problem by adding failing test case(s). When you have failing test case(s), you can create pull request just to reproduce fail in the CI. Then you can provide fix in the subsequent commits, it will make code review easier. It's allowed to modify existing test cases in bug fix pull request, but only if current behavior is proved to be invalid.

Not only it proves there actually is a bug, but also saves reviewers' time 🙂.

Anyway, I checked it locally and confirmed the buggy behaviour. For those who like code representation more than tokens, the flow here is:

  • There's a code <?php $a= [ ];
  • Token 3 (whitespace before [) is replaced with [
  • Token 5 (whitespace inside brackets) is replaced with ]
  • The code now is <?php $a=[[]]; which is valid (syntax-wise), but right now if you want to get index of a block end for token 4 (opening bracket for inner []), it should result with 5, but results with 6, which comes from Tokens::$blockStartCache

Thanks for the fix 🍻!

@Wirone Wirone added kind/bug topic/tokenizer Converting code into set of tokens labels Oct 28, 2024
@Wirone Wirone merged commit c982d2c into PHP-CS-Fixer:master Oct 28, 2024
28 checks passed
@mvorisek mvorisek deleted the fix_tokens_cache_8228 branch October 28, 2024 09:01
@mvorisek
Copy link
Contributor Author

Thank you very much, it took me almost a day to figure what was going on!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug topic/tokenizer Converting code into set of tokens
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ordered_class_elements and statement_indentation produces wrongly indented result when run together
3 participants