Add function call parameter related utility functions and abstract class #815

jrfnl · 2017-01-22T17:48:13Z

Now #795 has been merged, it was possible to create an abstract function parameter class which builds upon the logic for determining function calls already contained in WordPress_AbstractFunctionRestrictionsSniff.

This PR introduces the abstract and a number of related utility functions.

Utility functions

I initially introduced these utility functions in the PHPCompatibility standard were they are also individually unit tested.
They have been in use there for a number of months and proven useful & accurate.

The functions are:

does_function_call_have_parameters( $stackPtr ) - returns bool
get_function_call_parameter_count( $stackPtr ) - returns int
get_function_call_parameters( $stackPtr ) - returns array, key: 1-indexed parameter position, value: array with start - int stack pointer of param start position, end - int stack pointer of param end position and raw - trimmed raw content for the passed parameter
get_function_call_parameter( $stackPtr, $param_offset ) - returns array or false if no parameter was found at the specified offset

The functions expect to be passed a T_STRING stack pointer to a function call.

And while the function name might not indicate this, each of these functions can also be used with array stack pointers (T_ARRAY, T_OPEN_SHORT_ARRAY). This is a feature which was introduced after the initial naming & merge of the functions and the function documentation reflects this.

It is my intention to introduce these functions upstream in PHPCS itself at some point in the future.

New `WordPress_AbstractFunctionParameterSniff` class

The new WordPress_AbstractFunctionParameterSniff extends the WordPress_AbstractFunctionRestrictionsSniff class for the yes/no function call determination, retrieves the parameters used by the targetted function call using the new utility functions and then passes processing on to the child class.

The child class is expected to implement a process_parameters() method to handle the actual testing of parameters.

Optionally a process_no_parameters() method can be added to the child class to handle cases where parameters are expected, but not found.

The abstract expects a $target_functions array, but is without opinion about how that array should look, except for the requirement that the top level array keys are the function names.
Further requirements of the array are not needed as the processing of the parameters is done in the child class, allowing for great flexibility in potential use of this class.

Start using the abstract

As a last step - and to prove that the same functionality is maintained, and in some cases improved by the new abstract - I've refactored three existing sniffs which were prime candidates (duplicate logic and such) to start using the new WordPress_AbstractFunctionParameterSniff.

Fixes #745

I initially introduced these functions in the PHPCompatibility standard were they are also individually unit tested. They have been in use there for a number of months and proven useful & accurate.

This extends the `WordPress_AbstractFunctionRestrictionsSniff` class for the yes/no function call determination, retrieves the parameters used in a function call and then passes processing on to the child class. The child class is expected to implement a `process_parameters()` method to handle the actual testing of parameters. Optionally a `process_no_parameters()` method can be added to the child class to handle cases where parameters are expected, but not found. The abstract expects a `$target_functions` array, but is without opinion about how that array should look, except for the requirement that the top level array keys are the function names. Further requirements of the array are not needed as the processing of the parameters is done in the child class, allowing for great flexibility in potential use of this class.

…rameterSniff

grappler

Thank you very much for this.This will be a great help! This fixes #745

One thought that can to me is what would happen if one of the parameters that we were checking was an anonymous function. A common place where anonymous functions are used are hooks.

grappler · 2017-01-22T18:09:52Z

WordPress/AbstractFunctionParameterSniff.php

+	 *
+	 * @var array The only requirement for this array is that the top level
+	 * array keys are the names of the functions you're looking for.
+	 * Other than that, the array can have arbitrary content depending on your needs.


Does the text not need to be all on the same height? e.g.

/** * @var array The only requirement for this array is that the top level * array keys are the names of the functions you're looking for. * Other than that, the array can have arbitrary content depending * on your needs.

I second this.

grappler · 2017-01-22T18:12:42Z

WordPress/Sniff.php

+			// Ignore anything within short array definition brackets.
+			if ( 'T_OPEN_SHORT_ARRAY' === $this->tokens[ $next_comma ]['type']
+				&& ( isset( $this->tokens[ $next_comma ]['bracket_opener'] )
+					&& $this->tokens[ $next_comma ]['bracket_opener'] === $next_comma )


Is there a reason why there is an extra tab before the code?

Yes: nested parenthesis.

grappler · 2017-01-22T18:20:02Z

WordPress/Sniffs/PHP/StrictInArraySniff.php

+	 * @return void
+	 */
+	public function process_no_parameters( $stackPtr, $group_name, $matched_content ) {
+		$this->phpcsFile->addError( 'Missing arguments to %s.', $stackPtr, 'MissingArguments', array( $matched_content ) );


I don't think we need this as if there are no parameters then there will be a PHP error. #770 (comment)

We do need this.
By far not all function calls have required parameters, so a function call without parameters can be perfectly valid code, no syntax errors, but might still need to throw an error under certain rules.

The three functions that we are checking have required parameters. in_array, array_search and array_keys. Sorry I did not mean generally, just for this check.

Oh sorry, I was thinking the comment was about the abstract.

And true, in this case "no parameters" would also result in a PHP error, though not one which would be picked up by the Generic.PHP.Syntax sniff (not a parse error).
I added the method and the error message in this case to maintain backward compatibility. The same message was already being thrown by this sniff and I saw no compelling reason to remove it.

jrfnl · 2017-01-22T19:02:38Z

One thought that came to me is what would happen if one of the parameters that we were checking was an anonymous function. A common place where anonymous functions are used are hooks.

That works just fine, you will receive the start and end parameter and the raw content, no problem ;-)

More in-depth: The functions check for parenthesis at the correct nesting level and will skip over everything which is not at the right nesting level, so an anonymous function as a parameter will be returned and treated as one parameter.

And before you ask: The methods don't deal with parameters passed to anonymous functions, but don't need to either as that is a function declaration and you can use the PHPCS native getMethodParameters() for that.

grappler · 2017-01-22T21:12:32Z

WordPress/Sniff.php

+	 *
+	 * @param int $stackPtr The position of the function call token.
+	 *
+	 * @return array


Could we document the output too? Perhaps with something like:

/** * @return array { * Array of parameters. Empty array if there are no parameters. * * @type int $position Position of the parameter. { * @type int $start Start position of the call token. * @type int $end End position of the call token. * @type int $raw Raw parameter content. * } * }

or

/** * @return array { * Array of parameters. Empty array if there are no parameters. * * @type int $position => { Position of the parameter. * @type int $start Start position of the call token. * @type int $end End position of the call token. * @type int $raw Raw parameter content. * } * }

It already was documented a bit further up in the function documentation, but I've adjusted the format to a variant of what you suggested.

JDGrimes

💯 I've created similar functions before, though not as robust, and I know that they can be very useful.

JDGrimes · 2017-01-22T21:35:01Z

WordPress/AbstractFunctionParameterSniff.php

+	 *
+	 * @var array The only requirement for this array is that the top level
+	 * array keys are the names of the functions you're looking for.
+	 * Other than that, the array can have arbitrary content depending on your needs.


I second this.

jrfnl added 3 commits January 22, 2017 18:29

Introduce function call parameter/array utility functions.

cd5a64b

I initially introduced these functions in the PHPCompatibility standard were they are also individually unit tested. They have been in use there for a number of months and proven useful & accurate.

Refactor a few sniffs to start using the WordPress_AbstractFunctionPa…

888d79e

…rameterSniff

jrfnl added Status: Review ready Type: Cleanup Type: Enhancement labels Jan 22, 2017

jrfnl added this to the 0.11.0 milestone Jan 22, 2017

grappler reviewed Jan 22, 2017

View reviewed changes

JDGrimes suggested changes Jan 22, 2017

View reviewed changes

Minor documentation fixes.

44e3172

grappler approved these changes Jan 22, 2017

View reviewed changes

JDGrimes approved these changes Jan 23, 2017

View reviewed changes

GaryJones merged commit c418017 into develop Jan 23, 2017

GaryJones deleted the feature/function-parameter-abstract branch January 23, 2017 15:55

jrfnl mentioned this pull request Mar 6, 2017

Changelog for version 0.11.0 #866

Merged

GaryJones removed the Status: Review ready label Aug 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add function call parameter related utility functions and abstract class #815

Add function call parameter related utility functions and abstract class #815

jrfnl commented Jan 22, 2017 •

edited

Loading

grappler left a comment •

edited

Loading

grappler Jan 22, 2017

JDGrimes Jan 22, 2017

jrfnl Jan 22, 2017

grappler Jan 22, 2017

jrfnl Jan 22, 2017

grappler Jan 22, 2017

jrfnl Jan 22, 2017

grappler Jan 22, 2017 •

edited

Loading

jrfnl Jan 22, 2017

jrfnl commented Jan 22, 2017

grappler Jan 22, 2017 •

edited

Loading

jrfnl Jan 22, 2017

JDGrimes left a comment

JDGrimes Jan 22, 2017

Add function call parameter related utility functions and abstract class #815

Add function call parameter related utility functions and abstract class #815

Conversation

jrfnl commented Jan 22, 2017 • edited Loading

Utility functions

New WordPress_AbstractFunctionParameterSniff class

Start using the abstract

grappler left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grappler Jan 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Jan 22, 2017

grappler Jan 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JDGrimes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Jan 22, 2017 •

edited

Loading

New `WordPress_AbstractFunctionParameterSniff` class

grappler left a comment •

edited

Loading

grappler Jan 22, 2017 •

edited

Loading

grappler Jan 22, 2017 •

edited

Loading