False Positive for Events Manager

[msykes]

Hello, a user reported this false positive with Events Manager 5.9.6:

FILE: /data/www/dev/www.evangelizerichmond.org/wp-content/plugins/events-manager/classes/em-events.php FOUND 1 ERROR AFFECTING 1 LINE 296 | ERROR | Since PHP 7.0, functions inspecting arguments, like func_get_args(), no longer report the original value as passed to a parameter, but will instead provide the current value. The parameter “$args” was changed on line 292.

FILE: /data/www/dev/www.evangelizerichmond.org/wp-content/plugins/events-manager/classes/em-taxonomy-terms.php FOUND 1 ERROR AFFECTING 1 LINE 215 | ERROR | Since PHP 7.0, functions inspecting arguments, like func_get_args(), no longer report the original value as passed to a parameter, but will instead provide the current value. The parameter “$args” was changed on line 211.

FILE: /data/www/dev/www.evangelizerichmond.org/wp-content/plugins/events-manager/classes/em-locations.php FOUND 1 ERROR AFFECTING 1 LINE 237 | ERROR | Since PHP 7.0, functions inspecting arguments, like func_get_args(), no longer report the original value as passed to a parameter, but will instead provide the current value. The parameter “$args” was changed on line 234.

I assume this is related to arguments passed by reference, which is not an issue in the enclosing functions where the above lines trigger these errors.

[jrfnl]

I assume this is related to arguments passed by reference, which is not an issue in the enclosing functions where the above lines trigger these errors.

It is not, but I can't seem to get to browse your code at wp.org at the moment to explain it properly.

As you are getting the error and not the warning, you will actually be changing the received parameter(s) between the start of the function and the call to func_get_args().

In PHP < 7.0 the call to func_get_args() would have ignored that assignment and would still have returned the original value of the passed parameter. In PHP 7.0 and higher, the current value of the parameter is returned by func_get_args(), including whatever you changed.

In other words, these are not false positives. The behaviour of your function will be different in PHP 5 vs PHP 7 and your code is therefore not cross-version compatibility.

[msykes]

Hi @jrfnl, thanks for explaining and the fast reply. I see your point, but I'm still convinced it's a false positive in my case. The three cases use very similar code with the same principle, I'll show one:

public static function output( $args ){
	global $EM_Event;
	$EM_Event_old = $EM_Event; //When looping, we can replace EM_Event global with the current event in the loop
	//get page number if passed on by request (still needs pagination enabled to have effect)
	$page_queryvar = !empty($args['page_queryvar']) ? $args['page_queryvar'] : 'pno';
	if( !empty($args['pagination']) && !array_key_exists('page',$args) && !empty($_REQUEST[$page_queryvar]) && is_numeric($_REQUEST[$page_queryvar]) ){
		$args['page'] = $_REQUEST[$page_queryvar];
	}
	//Can be either an array for the get search or an array of EM_Event objects
	if( is_object(current($args)) && get_class((current($args))) == 'EM_Event' ){
		$func_args = func_get_args();
		//...

As you predicted, $args is manipulated at the start of the function so that is an issue in most cases, however, it only happens if $args is an array. If it's an object, then the next if statement calls func_get_args() meaning both the assignment and modified PHP method are never called in succession.

I realize it'll be pretty difficult to detect this sort of false positive, and I don't expect you to fix this situation (maybe we'll adjust the code at one point to avoid the false positive), but I'd appreciate reaching a consensus on whether it's a false positive so I can reference this ticket to those that bring it up in the meantime :)