Squiz/NonExecutableCode: simplify + improve handling of comments and closures

[jrfnl]

Description

Recreation of upstream PR squizlabs/PHP_CodeSniffer#3898:

Some tweaks for the sniff which I noticed while reviewing squizlabs/PHP_CodeSniffer#3770.

Squiz/NonExecutableCode: make sniff more code style independent

When determining whether a return statement is the last code token in a function body, comments should be ignored, but weren't.

Fixed now. Includes tests.

Squiz/NonExecutableCode: flag redundant return statements in closures too

A return statement which doesn't return a value at the end of a function body would be flagged as "not required" for named functions, but not so for anonymous functions.

Fixed now. Includes tests.

Squiz/NonExecutableCode: fold duplicate code

Follow up on commits 0e10f43 and 01754d9, which both deal with fixing bugs where the sniff would not handle if/elseif/else conditions without curly braces correctly.

This commit merges the two near duplicate code blocks, which the above mentioned commits introduced, each containing code doing essentially the same thing.

Also note that T_ELSE is handled separately now as else does not take parentheses and can therefore not be a parenthesis owner.

This change is already covered by pre-existing tests.

---

Discussion in the original ticket:

@fredden asked:

While reviewing this pull request, I noticed there is a false-negative in this sniff. With the following test file, I get a warning about Squiz.PHP.NonExecutableCode.ReturnNotRequired on line 9, but not on line 6. Even if I remove line 9, I still don't get a warning about line 6.

<?php

function test() {
    if (true) {
        echo 'yes';
        return;
    }

    return;
}

Is this something which is in scope here, or should I raise this separately so it can be fixed in a different pull request?

To which @jrfnl replied:

In my opinion that is outside the scope of this PR. The ReturnNotRequired check currently only checks for a return statement without a return value, which is directly followed by the function close brace.

Expanding that check to find other potentially redundant return statements could get complex and I think this might even be better off in a separate sniff.

Suggested changelog entry

• Squiz/NonExecutableCode: redundant return statements just before a function close brace will now be flagged more often.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)