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

PEAR/Functions/FunctionDeclaration: add extra defensive coding #420

Merged
merged 3 commits into from
Apr 3, 2024

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Mar 29, 2024

Description

PEAR/Functions/FunctionDeclaration: minor efficiency tweak

Only call File::getMethodProperties() once.

PEAR/Functions/FunctionDeclaration: rename test case file

... in anticipation of adding additional test case file(s).

PEAR/Functions/FunctionDeclaration: add extra defensive coding

When the sniff is run during live coding, the sniff could encounter a situation where a function declaration does not have a scope opener, not a function body.

In that case, the "SpaceBeforeSemicolon" check would try to find a semi-colon, presuming the function is an abstract method or interface method, but would not find one, which would result in the if ($tokens[($end - 1)]['content'] === $phpcsFile->eolChar) condition throwing a "Undefined array key -1" notice.

Fixed now.

Includes test safeguarding the fix.

Note: this fix will, by extension, fix this same error for the PSR2.Methods.FunctionCallSignature sniff and the Squiz.Functions.MultiLineFunctionDeclaration sniff.

👉 this commit will be easier to review while ignoring whitespace changes.

Suggested changelog entry

PEAR/FunctionDeclaration: prevent a PHP notice during live coding

Related issues/external references

Loosely related to #152 as this issue would prevent the fixer conflict check from finishing its run.

Types of changes

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

Copy link
Contributor

@rodrigoprimo rodrigoprimo left a comment

Choose a reason for hiding this comment

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

LGTM!

I don't know if this is something that you might want to address in this PR as it is not related to the changes proposed here, but while reviewing the code, I found a redundant check in an if condition.

if (isset($tokens[$stackPtr]['parenthesis_opener']) === false
|| isset($tokens[$stackPtr]['parenthesis_closer']) === false
|| $tokens[$stackPtr]['parenthesis_opener'] === null
|| $tokens[$stackPtr]['parenthesis_closer'] === null
) {
return;
}

I don't think the code needs to explicitly check if parenthesis_opener or parenthesis_closer is null as this is already handled by the isset() checks. Happy to create an issue or submit a PR for this if you prefer.

@jrfnl
Copy link
Member Author

jrfnl commented Apr 2, 2024

I don't know if this is something that you might want to address in this PR as it is not related to the changes proposed here, but while reviewing the code, I found a redundant check in an if condition.

if (isset($tokens[$stackPtr]['parenthesis_opener']) === false
|| isset($tokens[$stackPtr]['parenthesis_closer']) === false
|| $tokens[$stackPtr]['parenthesis_opener'] === null
|| $tokens[$stackPtr]['parenthesis_closer'] === null
) {
return;
}

I don't think the code needs to explicitly check if parenthesis_opener or parenthesis_closer is null as this is already handled by the isset() checks. Happy to create an issue or submit a PR for this if you prefer.

Out of scope for this PR. This PR addresses a very specific issue (found during review of the fixer conflicts).

It's easy to get distracted from the task in hand and to try to fix everything you see in a sniff (and I sometimes do try), but most of the time, fixing the one thing you came to fix in a sniff and stopping there (possibly making a note of other things seen which need further investigation) is the better thing to do. Small incremental improvement keep the codebase moving forward, while trying to do everything at once, often means nothing ever gets finished.

Would be good to check why those conditions were added (via the history), though I agree with your assessment that they are probably not needed. Possibly something to address in combination with the code coverage check ? (which is a typical task where redundant code would be found and removed)

@rodrigoprimo
Copy link
Contributor

rodrigoprimo commented Apr 2, 2024

Would be good to check why those conditions were added (via the history), though I agree with your assessment that they are probably not needed. Possibly something to address in combination with the code coverage check ? (which is a typical task where redundant code would be found and removed)

Makes sense to me, thanks. I will add to my list to work on improving the code coverage for this sniff and then I will investigate if indeed we can remove those conditions.

Only call `File::getMethodProperties()` once.
... in anticipation of adding additional test case files.
When the sniff is run during live coding, the sniff could encounter a situation where a function declaration does not have a scope opener, not a function body.

In that case, the "SpaceBeforeSemicolon" check would try to find a semi-colon, presuming the function is an abstract method or interface method, but would not find one, which would result in the `if ($tokens[($end - 1)]['content'] === $phpcsFile->eolChar)` condition throwing a _"Undefined array key -1"_ notice.

Fixed now.

Includes test safeguarding the fix.

Note: this fix will, by extension, fix this same error for the `PSR2.Methods.FunctionCallSignature` sniff and the `Squiz.Functions.MultiLineFunctionDeclaration` sniff.

:point_right: this commit will be easier to review while ignoring whitespace changes.
@jrfnl jrfnl force-pushed the feature/pear-functiondeclaration-bugfix branch from d9cf33f to e5ce70a Compare April 3, 2024 00:10
@jrfnl jrfnl enabled auto-merge April 3, 2024 00:11
@jrfnl jrfnl merged commit e0fc8ef into master Apr 3, 2024
44 checks passed
@jrfnl jrfnl deleted the feature/pear-functiondeclaration-bugfix branch April 3, 2024 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants