Skip to content

Conversation

keradus
Copy link
Member

@keradus keradus commented Jul 10, 2025

closes #8828

interesting part is simply that utest work, just super slow if too many tokens with {$foo}

so utest not failing, but itest can run out of time

@coveralls
Copy link

coveralls commented Jul 10, 2025

Coverage Status

coverage: 94.795% (-0.009%) from 94.804%
when pulling ef83be8 on keradus:8828
into f247a10 on PHP-CS-Fixer:master.

@keradus keradus marked this pull request as ready for review July 10, 2025 10:04
@keradus keradus changed the title BracesPositionFixer - fix performance issue for massive files fix: BracesPositionFixer - fix performance issue for massive files Jul 10, 2025
…e handle all cases, not only those we remembered about
Copilot

This comment was marked as resolved.

@keradus keradus enabled auto-merge (squash) July 10, 2025 11:51
@keradus keradus marked this pull request as draft July 10, 2025 16:04
auto-merge was automatically disabled July 10, 2025 16:04

Pull request was converted to draft

@keradus keradus changed the title fix: BracesPositionFixer - fix performance issue for massive files fix: BracesPositionFixer - fix performance issue for massive files with CT::T_CURLY_CLOSE Jul 10, 2025
@keradus keradus marked this pull request as ready for review July 10, 2025 16:49
@keradus keradus enabled auto-merge (squash) July 10, 2025 16:49
Copy link
Member

Choose a reason for hiding this comment

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

What about instead of this file, which is 1.7MB, we add a test case like below?

        yield 'string with a lot of variables in braces' => [
            '<?php
                if (true) {
                    $foo = "'            . str_repeat(' text {$variable} text ', 50_000) . '";
                }',
        ];

Especially that the name of the test case is a spoiler for what is coming next: the current fix works for 👆🏼 , but not for 👇🏼 .

        yield 'string with a lot of variables' => [
            '<?php
                if (true) {
                    $foo = "'            . str_repeat(' text $variable text ', 50_000) . '";
                }',
        ];

Which would need another over 1MB if we go in the direction of explicit files.

Copy link
Member Author

Choose a reason for hiding this comment

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

tbh, i'm ok with both.

Copy link
Member Author

Choose a reason for hiding this comment

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

i think to move back to integration test.
CI is failing when code-coverage or mutation, and integration test is outside of them by default

Copy link
Member

Choose a reason for hiding this comment

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

I guess the same problem exists for concatenation of many variables..

        yield 'string with concatenation of a lot of variables' => [
            '<?php
                if (true) {
                    $foo = "'            . str_repeat(' text ".$variable." text ', 50_000) . '";
                }',
        ];

Copy link
Member Author

Choose a reason for hiding this comment

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

i think to move back to integration test. CI is failing when code-coverage or mutation, and integration test is outside of them by default

done

Comment on lines 254 to 255
$openBraceIndex = $tokens->getNextTokenOfKind($index, ['{', ';', [CT::T_CURLY_CLOSE], [CT::T_PROPERTY_HOOK_BRACE_OPEN], [T_CLOSE_TAG]]);

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$openBraceIndex = $tokens->getNextTokenOfKind($index, ['{', ';', [CT::T_CURLY_CLOSE], [CT::T_PROPERTY_HOOK_BRACE_OPEN], [T_CLOSE_TAG]]);
$nextToken = $tokens->getNextMeaningfulToken($index);
if (!$tokens[$nextToken]->equalsAny(['=', [CT::T_PROPERTY_HOOK_BRACE_OPEN]])) {
continue;
}
$openBraceIndex = $tokens->getNextTokenOfKind($x, ['{', ';', [CT::T_PROPERTY_HOOK_BRACE_OPEN], [T_CLOSE_TAG]]);

I was thinking about something like this:

  • first - quick "deal breaker" for having a property hook after the variable
  • second - proper check if the property hook is there

The $x could be either $index (for simplicity) or $nextToken - 1 (for microoptimization).

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure if I grasp your idea here, want to sent a PR with it ?

Copy link
Member

Choose a reason for hiding this comment

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

If the variable is not followed by = or T_PROPERTY_HOOK_BRACE_OPEN, it cannot be a property with hooks.
In the given test cases it would be a fast fail for all variables inside the string.
Only for the first $foo = ... variable it would still traverse through all tokens in the long statement to the statement end (;).

Co-authored-by: Kuba Werłos <werlos@gmail.com>
@keradus keradus disabled auto-merge July 11, 2025 07:37
@keradus keradus requested a review from kubawerlos July 11, 2025 07:53
@keradus
Copy link
Member Author

keradus commented Jul 14, 2025

I'm thinking to merge and release before my hols, so production setup is bit better.
further improvements can be raised as separated PRs

@keradus keradus merged commit 998cf2e into PHP-CS-Fixer:master Jul 14, 2025
31 checks passed
@keradus keradus deleted the 8828 branch July 14, 2025 15:40
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.

BracesPositionFixer very slow for big migration file
4 participants