PEAR/Functions/FunctionDeclaration: add extra defensive coding

[jrfnl]

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)

[rodrigoprimo]

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.

PHP_CodeSniffer/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php

Lines 67 to 73 in d9cf33f

	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]

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.

PHP_CodeSniffer/src/Standards/PEAR/Sniffs/Functions/FunctionDeclarationSniff.php

Lines 67 to 73 in d9cf33f

	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]

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.