Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Analysis of function calls with a callback parameter #123

Open
GaryJones opened this issue Mar 29, 2020 · 3 comments
Open

Analysis of function calls with a callback parameter #123

GaryJones opened this issue Mar 29, 2020 · 3 comments

Comments

@GaryJones
Copy link
Contributor

Is there a PHPCS / PHPCSUtils function for checking if a token is an array (either array( or [ that isn’t a list)?

I mean, I can see PHPCSUtils has an $arrayTokens which I could compare against, but being able to use \PHPCSUtils\Arrays\is_array( $stackPtr ) (or similar) would be cleaner than having to do an isset / in_array / array_has_key (and remembering that isset is generally the most performant of those approaches) within the external standard code against that token collection.

Extension: Perhaps if there are typical uses of the token collections (comparing against a $stackptr), then a new utility method should exist for them, so that external standards sniffs are more full of function calls than isset and === comparisons.

@jrfnl
Copy link
Member

jrfnl commented Mar 29, 2020

The idea of those token collections is that you can easily do the below, which is more performance efficient than using a function call to do the same:

if (isset(Collections::$arrayTokens[$tokens[$stackPtr]['code']]) === true) {
    // This is an array token.
}

However, that doesn't answer the "that isn’t a list" part of your question.

For that, you'd currently need to do:

if (isset(Collections::$arrayTokens[$tokens[$stackPtr]['code']]) === true
    && Arrays::isShortArray($phpcsFile, $stackPtr) === true
) {
    // This is the start of an array, definitely not a (short) list.
}

The questions is why are you doing this ? What is the next step in the sniff logic ?

If you want to skip over an array, you can use Arrays::getOpenClose() to get the array closer (or false if this turned out not to be an array).

If it is to get all the array items, you can call PassedParameters::getParameters() or PassedParameters::hasParameters().
Or perhaps even better: use the AbstractArraySniff as a base class.

Those functions already contain all the defensive coding to return either false or throw an exception if this would turn out to not be a token handled by the function.
And the abstract would just bow out if the token isn't an actual array.

Perhaps if there are typical uses of the token collections (comparing against a $stackptr), then a new utility method should exist for them, so that external standards sniffs are more full of function calls than isset and === comparisons.

That's the idea behind the functions I mention above (as examples, there are more of course).

However, having a function call to just do an isset() on one of the token arrays is unnecessary overhead.

So, what is the use case ?

I'd like to see if there are more typical use cases for which utility functions could be written.

@GaryJones
Copy link
Contributor Author

In this case, we're looking at at an add_filter() call, particularly the second argument, which could be various callables:

add_filter( 'my_filter', '__return_true' ); // string - name of callable
add_filter( 'my_filter', function() { // closure
	... 
} );
 // Arrays (for classes and objects), in short and long form.
add_filter( 'my_filter', [ \Foo\My_Class::class, 'my_class_method' ] );
add_filter( 'my_filter', [ $my_foo_object, 'my_object_method' ] );
add_filter( 'my_filter', array( \Foo\My_Class::class, 'my_class_method' ) );
add_filter( 'my_filter', array( $my_foo_object, 'my_object_method' ) );

The logic I'd like to be able to simplify is to when I get to the second parameter is to say "Is this an array?" since handling of that array would be done differently than if it was a string or a closure.

The existing logic isn't too unwieldy, but doesn't support long array syntax for a start.

As you can see, the focus of this sniff isn't about the array specifically so the AbstractArraySniff wouldn't likely make sense.

What I need to look into, is whether calling PassedParameters::get_parameter() on the add_filter() call token will reveal whether it's an array or not (docs suggest not - maybe it could?).

@jrfnl
Copy link
Member

jrfnl commented Mar 29, 2020

Ah, ok, so we're talking callback. I'm already tentatively working on a CallBackTrait with helpers for that. Will keep your question in mind.

@jrfnl jrfnl changed the title Array:isArray() Analysis of function calls with a callback parameter Mar 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants