Handle parse error in File::getMethodProperties()

[fredden]

Description

While investigating the cause of a PHP Fatal error while processing some input in the PSR12.Functions.ReturnTypeDeclaration sniff, I noticed that the File::getMethodProperties() method did not return sensible values in for some parse errors. The input file in question was
src/Standards/Generic/Tests/Arrays/DisallowLongArraySyntaxUnitTest.2.inc, which contains an intentional parse error. The File::getMethodProperties() method claimed that the return type of the test function was << HEAD, which is nonsense.

The change in this commit safeguards from such parse errors and resolves the issue that was being encountered downstream in the sniff.

Suggested changelog entry

Handle parse error in File::getMethodProperties()

Related issues/external references

Relates to #152

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.
• I have opened a sister-PR in the documentation repository to update the Wiki.

[jrfnl]

@fredden Thanks for creating this PR. I'm currently looking at the conflict you identified and I'm not so sure this is the correct solution as it changes a return value which is valid (null is the return type, even though there is a parse error).

• Did you consider strengthening the "end of function definition" condition (first one in the for loop) with checking for an open curly ? And if so, why did you decide against that ?
  => Strengthening that check would prevent the method from walking beyond the function signature to find the return type.
• Did you consider adding defensive coding to the PSR12/ReturnTypeDeclaration token against not finding the colon ?

[fredden]

• Did you consider strengthening the "end of function definition" condition (first one in the for loop) with checking for an open curly ? And if so, why did you decide against that ?
  => Strengthening that check would prevent the method from walking beyond the function signature to find the return type.

Reading the code with fresh eyes, it seems like this is a sensible suggestion. I think I dismissed this due to the need to skip over T_USE blocks, but with fresh eyes that doesn't make sense. I will review this approach and post an update.

• Did you consider adding defensive coding to the PSR12/ReturnTypeDeclaration token against not finding the colon ?

Yes, that was where I started. When I realised that the sniff was relying on the output from File::getMethodProperties() and that method was returning nonsense, I switched to fixing that instead.
Fixing this particular sniff instead of the helper method seemed like work that would need to be repeated. I did not look for where else this helper method is used in the codebase, but given it is a public method in the PHP_CodeSniffer API it seemed reasonable to assume that external parties may also be using this. Fixing the root cause of the issue seemed more efficient than the individual sniff.

It should be easy to add some validation to the sniff to cover the case where $colon is false instead of a valid token array reference.

[fredden]

I have opened #1354 to add some defensive coding to the sniff that started this journey. For the File::getMethodProperties() fix / changes, I am struggling to create a good test case to trigger the bug that won't mess up other tests in the same file. @jrfnl please can you help me with an example of how to split parse errors into their own files for internal tests?

[jrfnl]

I am struggling to create a good test case to trigger the bug that won't mess up other tests in the same file. @jrfnl please can you help me with an example of how to split parse errors into their own files for internal tests?

The most straight-forward way to add these would be by adding more test classes with their own .inc file, similar to https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/4.x/tests/Core/Files/File/GetMethodParametersParseError1Test.php

Mind - PHPCSUtils has a shadow-method for the File::getMethodProperties() method and it looks like it already has an extra parse error test: https://github.com/PHPCSStandards/PHPCSUtils/blob/develop/Tests/BackCompat/BCFile/GetMethodPropertiesParseError1Test.inc

Might be worth porting that one over from Utils to PHPCS before adding more parse error tests for this method.