Make InnerFunctionsSniff detect functions inside closures

[Daimona]

Fixes #3806

[jrfnl]

LGTM

[jrfnl]

I agree this is a good change, but looking at this sniff now, I see more things wrong with it.

@Daimona Would you be willing to make some additional fixes ?

Additional things which I believe should be addressed:

1. There is redundant code (the check for =). This is due to in the older day closures being tokenized as T_FUNCTION. That code will never be matched anymore now.
2. The part where it checks for methods in anonymous classes should probably be expanded to check for all OO structures.
   See: https://3v4l.org/c2Cbn
3. As more checking of the conditions is now needed, the sniff should probably check the conditions array itself instead of using getCondition().

Does that make sense ?

[Daimona]

1. There is redundant code (the check for =). This is due to in the older day closures being tokenized as T_FUNCTION. That code will never be matched anymore now.

Makes sense to me! In fact, I was a bit puzzled by that check given the existence of T_CLOSURE (that's not listed in register()).

I agree with the rest as well. I've updated my PR.

[jrfnl]

Thanks for making these updates @Daimona !

Looking good.

I wonder if the two loops walking the $conditions array can be combined ?

I'm thinking that logic like this may work (very much pseudo-code):

// Reverse the conditions array
foreach () {
    if ($condition === T_FUNCTION || $condition === T_CLOSURE) {
        break; // Error.
    }

    if (isset(Tokens::$ooScopeToken[$condition])) {
        return; // Found OO structure before function/closure, so this is an OO method.
    }
}

[jrfnl]

Extra code sample you may want to add to the tests (would currently not be detected/false negative):

function foo() {
    if (class_exists('Bar') === false) {
        class Bar {
            function foo() {
                function innerFunction() {} // Error.
            }
        }
    }
}

[Daimona]

I wonder if the two loops walking the $conditions array can be combined ?

I wanted to use separate loops so that we can bail out earlier if it's not a nested function. However, on second though that's simply untrue -- we'd loop through the whole array of conditions in the common case without nested functions. And I guess reversing the array of conditions is not terribly expensive, either, so I'm going to merge the loops, and make all the other suggested changes.

[jrfnl]

LGTM 🎉

Thanks for working with me to get this sniff into shape!

[jrfnl]

@Daimona I'm going to merge this PR. The changelog normally contains the full name of the contributor. Would you mind sharing yours ?

[Daimona]

@Daimona I'm going to merge this PR. The changelog normally contains the full name of the contributor. Would you mind sharing yours ?

Hi :) I don't use my real name online; at least not in connection with this username. Would it be possible to just credit me as "Daimona"?

[jrfnl]

Hi :) I don't use my real name online; at least not in connection with this username. Would it be possible to just credit me as "Daimona"?

Absolutely! Just wanted to give you the chance, but only if you want it.

[jrfnl]

FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).