Checking callbacks for restricted functions

[grappler]

fixes #611

I removed preg_replace_callback_array() from the list as the callback is not a parameter but part of the value. In the following example where foobar is a restricted function.

$subject = 'Aaaaaa Bbb';
preg_replace_callback_array(
    [
        '~[a]+~i' => 'foobar',
    ],
    $subject
);

We could make a separate issue for this.

I was not sure where to place the tests for the abstract so I ended up using the old FunctionRestrictionsSniff. If there is a better place to add the tests I can move them there.

[grappler]

@jrfnl @GaryJones Can this be merged?

[jrfnl]

@grappler Reviewing this now.

[jrfnl]

@grappler Thank you for your efforts on this.

I'm sorry, but I think this PR still needs quite a lot of work and possibly a rethink on the principle of it.

The original issue only mentioned sniffing for functions through the AbstractFunctionRestrictionsSniff, but the principle behind it can - and probably should - also be applied when checking for sanitization functions and such.

For that to be possible, we need to implement this with more abstraction.

First off, I'd like to suggest moving the $callback_functions list to the WordPress_Sniff as this list might also be useful for other sniffs in the future.
We may even want to split the list in two (or three) - there are callback functions which directly execute (the PHP ones) and callback functions which are used in specific circumstances and even functions which take a callback, but don't execute it. For different purposes we may want to use a different subsection of the list. Splitting the list up would allow for that, in combination with a retrieval function which could get them all:

protected function get_all_callback_functions() {
	static $list;
	if ( ! isset( $list ) ) {
		$list = array_merge( $array1, $array2, $array3 );
	}
	return $list;
}

In a similar vain, a is_callback_function() method in the WordPress_Sniff which would return false or an array of the names of the callback functions would allow other sniffs to use the logic as well.

At a later point we could add an is_callback_method() for class methods which take callbacks.

Apart from that I have left quite a lot of comments on the code itself. Main points:

• We're missing quite a lot of functions (I know you based it on a list I posted, but I did say that was a start, not complete).
• Array callbacks could be misidentified as function callbacks
• Another function parameter could be misidentified as a function callback.

Altogether, this PR is IMHO not ready for merge.

From #611 (comment):

Also - don't forget eval() and create_function()

I imagine this may need to be addressed separately. In that case, please open a new issue for it.

[jrfnl]

Comment seems incorrect.

[jrfnl]

Please remove this as - in the unlikely event that a theme/plugin would have a function prefixed with foobar, it would trigger.

If you need to add a function group for unit testing only, please use the setUp() method in the unit test file. See https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress/Tests/DB/RestrictedClassesUnitTest.php for an example.

[jrfnl]

function => functions

[jrfnl]

While these functions won't execute the callback, they do take a callback argument. Should these be checked as well ?

• has_action
• has_filter
• remove_action
• remove_filter

[jrfnl]

I'm missing these WP functions in the list (not claiming completeness by any means - this is just a summary of the result of the first two pages returned by https://wordpress.org/search/callable ):

• register_activation_hook()
• register_deactivation_hook()
• register_uninstall_hook()
• add_menu_page(), add_submenu_page() and all other variants of it
• register_setting() - subkey within argument array + old format?
• register_meta() - 2 x subkey within argument array + old format
• wp_add_dashboard_widget() x 2
• add_shortcode()
• add_meta_box()
• spl_autoload_register()
• add_custom_background() (deprecated)
• add_custom_image_header() (deprecated)

[jrfnl]

No need for the array() wrapper here.

[jrfnl]

Please pass the end position too to prevent incorrect results.

[jrfnl]

A callback can be an array. In that case, you want to pass the stack pointer of the T_ARRAY or T_SHORT_ARRAY_OPEN to the get_function_call_parameters() method again.

[jrfnl]

You could bow out early for closures.

[jrfnl]

Should the function return false or just return ?

[grappler]

Thank you @jrfnl for your comments. I think an extra level of abstraction is good.

When we move check_for_callback_matches()/is_callback_method() to WordPress_Sniff then we would need to move check_for_matches() too as it is being used by check_for_callback_matches().

My concern is that with the additional utility the functions WordPress_Sniff will become a really big class. My suggestions would be to create a Functions_Sniff class that extends WordPress_Sniff but will be extended by WordPress_AbstractFunctionRestrictionsSniff.

I have fixed a few of the things mentioned.

[jrfnl]

@grappler

When we move check_for_callback_matches()/is_callback_method() to WordPress_Sniff then we would need to move check_for_matches() too as it is being used by check_for_callback_matches().

Not if you split the logic for determining whether something is a callback off from the check_for_callback_matched function the way I suggested above.

The sniff specific logic - checking for excluded groups etc - should stay in the sniff.

[grappler]

@jrfnl It would be great if you could have a look at this PR.

The "Preliminary check" only checks for functions in the list but not for the callback functions which was causing the code to fail. So I had to disable it for the callback functions but I am not sure if that is the best way. You can find more info in the commits.

What is still missing is:

We may even want to split the list in two (or three) - there are callback functions which directly execute (the PHP ones) and callback functions which are used in specific circumstances and even functions which take a callback, but don't execute it.

[jrfnl]

Thank you @grappler for working on this and I'm sorry that it took us so long to get back to it.

As I mentioned in the comment in the original ticket which I left today, I think it would be better if we have some utility function with regards to callbacks in PHPCSUtils before we try to address this for WPCS, especially as the code which would be needed will need additional changes to be able to include handling PHP 8.0 named parameters etc.

With that in mind, I'm closing this PR. I'm really sorry about this and hope you may want to work with me on getting the utility functions sorted in PHPCSUtils.