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

Squiz/FunctionSpacing: bug fix - prevent fixer conflict with itself #93

Merged

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 11, 2023

Description

Recreation of upstream PR squizlabs/PHP_CodeSniffer#3905:

The Squiz.WhiteSpace.FunctionSpacing sniff demands # lines above and below a function declaration, but when determining the number of lines above or below, it does not care for additional code on the same line as the function declaration. I.e.: it does not demand that a function declaration starts on its own line or that the close brace is on its own line (there are other sniffs for that).

The sniff also tries to prevent "double" errors for the same issue, i.e. when two functions are each on their own line without blank lines between them, the sniff will only throw one error for "spacing after" the first function and will not throw an error for "spacing before" for the second function.

To determine whether the "spacing before" error needs to be hidden, the sniff tries to check whether the tokens on the previous line indicate the line contains a function declaration.

As things were, however, this check could bow out too early as it stopped at a scope opener for a wrapping construct (class), however, that wrapping construct could be declared on the same line as the function, which means that in that case, the sniff would not determine the $foundLines correctly, as it stops on the current line at the scope opener instead of on a non-blank line above the function line.

This commit fixes this bug by changing the $stopAt value to contain the wrapping scope opener applicable to the code before the function line. This allows the $foundLines to be determined correctly and prevents the fixer conflict.

Includes additional tests.

Suggested changelog entry

Fixed bug squizlabs/PHP_CodeSniffer#3904 : Squiz/FunctionSpacing: prevent potential fixer conflict

Related issues/external references

Fixes squizlabs/PHP_CodeSniffer#3904

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

The `Squiz.WhiteSpace.FunctionSpacing` sniff demands # lines above and below a function declaration, but when determining the number of lines above or below, it does not care for additional code on the same line as the function declaration.
I.e.: it does not demand that a function declaration starts on its own line or that the close brace is on its own line (there are other sniffs for that).

The sniff also tries to prevent "double" errors for the same issue, i.e. when two functions are each on their own line without blank lines between them, the sniff will only throw one error for "spacing after" the first function and will not throw an error for "spacing before" for the second function.

To determine whether the "spacing before" error needs to be hidden, the sniff tries to check whether the tokens on the previous line indicate the line contains a function declaration.

As things were, however, this check could _bow out_ too early as it stopped at a scope opener for a wrapping construct (class), however, that wrapping construct _could_ be declared on the same line as the function, which means that in that case, the sniff would not determine the `$foundLines` correctly, as it stops on the current line at the scope opener instead of on a non-blank line above the function line.

This commit fixes this bug by changing the `$stopAt` value to contain the wrapping scope opener applicable to the code _before_ the function line. This allows the `$foundLines` to be determined correctly and prevents the fixer conflict.

Includes additional tests.

Fixes 3904
@jrfnl jrfnl force-pushed the feature/3904-squiz-functionspacing-fixer-conflict-with-itself branch from 95f24e4 to 17e17ac Compare December 5, 2023 13:24
@jrfnl jrfnl merged commit 103a84d into master Dec 5, 2023
64 checks passed
@jrfnl jrfnl deleted the feature/3904-squiz-functionspacing-fixer-conflict-with-itself branch December 5, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant