QA: remove unreachable code

[jrfnl]

Description

Tests: remove unreachable code

break after a return statement is redundant.

As a side-note: I did have to chuckle when I saw that the NonExecutableCode test file also included this 😂

Various: remove some redundant return statements

... when the return is at the end of the function without a value, and therefore not needed.

PHPCS: add the Squiz.PHP.NonExecutableCode sniff to the ruleset

... which is used for PHPCS itself.

Suggested changelog entry

N/A

[fredden]

Thanks for this @jrfnl. The changes here make sense to me. I have two additions to request:

1. Please can you add <rule ref="Squiz.PHP.NonExecutableCode"/> to phpcs.xml.dist so that this problem does not re-occur. (Note that until Respect all sniffs when reviewing PHP_CodeSniffer itself #3914 is merged, any warnings that the sniff produces will be masked/hidden/ignored.)

2. When reviewing the code in this branch, I noticed that there are three other files in violation of Squiz.PHP.NonExecutableCode.ReturnNotRequired. Is fixing those in-scope for this pull request?

........................................................W... 60 / 68 (88%)
...W..W.                                                     68 / 68 (100%)



FILE: src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php
----------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------
 215 | WARNING | Empty return statement not required here
     |         | (Squiz.PHP.NonExecutableCode.ReturnNotRequired)
----------------------------------------------------------------------------------------------------------


FILE: src/Sniffs/AbstractScopeSniff.php
----------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------
 151 | WARNING | Empty return statement not required here
     |         | (Squiz.PHP.NonExecutableCode.ReturnNotRequired)
----------------------------------------------------------------------------------------------------------


FILE: tests/Standards/AbstractSniffUnitTest.php
----------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------
 434 | WARNING | Empty return statement not required here
     |         | (Squiz.PHP.NonExecutableCode.ReturnNotRequired)
----------------------------------------------------------------------------------------------------------

Time: 2.42 secs; Memory: 6MB

[jrfnl]

Thanks for this @jrfnl. The changes here make sense to me. I have two additions to request:

1. Please can you add `<rule ref="Squiz.PHP.NonExecutableCode"/>` to `phpcs.xml.dist` so that this problem does not re-occur.

2. When reviewing the code in this branch, I noticed that there are three other files in violation of `Squiz.PHP.NonExecutableCode.ReturnNotRequired`. Is fixing those in-scope for this pull request?

@fredden Thanks for looking this over.

I didn't use the Squiz.PHP.NonExecutableCode sniff to create this PR. This PR focused solely on the "break after return"-pattern in the test files, which was typically still there in the test files for some of the older sniffs.

Having said that, I agree that adding those three extra fixes and adding the sniff to the ruleset makes sense.

I'm generally cautious about making additions to the CS ruleset used by the repo (or any repo which is not owned/controlled by me), but this seems like a trivial addition.

[fredden]

I'm generally cautious about making additions to the CS ruleset used by the repo (or any repo which is not owned/controlled by me), but this seems like a trivial addition.

Yes, I agree with this approach. In this case, I see the rule-set adjustment as the "add tests to cover the change" part of the contribution. If the maintainer doesn't want the changes to the rule-set, then they probably also don't need the code changes. I also see you as a primary contributor to this repository.

[fredden]

👍

[jrfnl]

Closing as replaced by PHPCSStandards/PHP_CodeSniffer#96