PHP 7.0 cross-version compatibility issue

[jrf]

Follow up on ​https://bbpress.trac.wordpress.org/ticket/3294#comment:4

PHPCompatibility

An initial run with that throws up 11 issues.

• 8 will need review of the code (which I will do in a bit).

I've just reviewed these and aside from 1 which was a warning, the other 7 are actual PHP cross-version compatibility issues, though they would need a specific set of circumstances for this to become problematic.

I'll upload a patch to add an ignore comment for the 1 warning which isn't an issue.

Issue list

FILE: I:\000_GitHub\WP_bbPress plugins\bbpress\WP-SVN\trunk\src\includes\users\template.php
------------------------------------------------------------------------------------------------------------------------------------------------------------

FOUND 7 ERRORS AFFECTING 7 LINES
------------------------------------------------------------------------------------------------------------------------------------------------------------

  538 | 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 "$user_id" was changed on line 532.
      |       | (PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed)
  628 | 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 "$user_id" was changed on line 622.
      |       | (PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed)
  987 | 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 "$user_id" was changed on line 981.
      |       | (PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed)
 1178 | 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 "$user_id" was changed on line 1172.
      |       | (PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed)
 1617 | 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 "$user_id" was changed on line 1611.
      |       | (PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed)
 1670 | 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 "$user_id" was changed on line 1664.
      |       | (PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed)
 1723 | 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 "$user_id" was changed on line 1717.
      |       | (PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed)
------------------------------------------------------------------------------------------------------------------------------------------------------------

The problem

All of these concern code following the below code pattern:

<?php
        function bbp_function_name( $user_id = 0, ... other args ) {

                // Use displayed user ID if there is one, and one isn't requested
                $user_id = bbp_get_user_id( $user_id );
                if ( empty( $user_id ) ) {
                        return false;
                }

                // Bail if intercepted
                $intercept = bbp_maybe_intercept( 'bbp_pre_function_name', func_get_args() );
                if ( bbp_is_intercepted( $intercept ) ) {
                        return $intercept;
                }

If the call to bbp_get_user_id() changes the $user_id, the return value of func_get_args() will be different between PHP < 7 and PHP 7.0+, which in turn may influence the return value of the call to bbp_maybe_intercept().

Also see: ​https://www.php.net/manual/en/migration70.incompatible.php#migration70.incompatible.other.func-parameter-modified

I can't currently gauge what would be the best way to solve this, but these are some avenues which can be explored:

• Move the call to bbp_maybe_intercept() to above the user id check.
• Don't use func_get_args(), but pass in the actual parameter names wrapped in an array() in the function call to bbp_maybe_intercept().

[jrf]

Add ignore flag to non-issue

[jrf]

Note: the patch fixes all violations against the PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.NeedsInspection error code. See #3294

The remaining issues are for the PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue.Changed error code.

[johnjamesjacoby]

Suggested patch does a few things:

• Parses known arguments against function arguments, allowing them to be filtered via bbp_parse_args
• Explicitly allows local variables (such as $user_id) to override the function arguments (vs this being fuzzy)

The unexpected behavior change is still present between PHP versions, however I doubt anyone will notice, as these interceptors are designed to allow for a complete and total override regardless of their values.

This patch generally serves to solidify the current PHP7+ behavior by making the code intentional.

If all this doesn't seem necessary, the original patch is also perfectly fine. This was just my quick attempt at adding clarity to the code.

[jrf]

Replying to johnjamesjacoby:

The unexpected behavior change is still present between PHP versions, however I doubt anyone will notice, as these interceptors are designed to allow for a complete and total override regardless of their values.

This patch generally serves to solidify the current PHP7+ behavior by making the code intentional.

As you already surmised, with the code in the patch, the issue is still present, so IMHO this patch does nothing to mitigate the situation and isn't really necessary.

In my original report, I mentioned this as a possible avenue to solve this:

Don't use func_get_args(), but pass in the actual parameter names wrapped in an array() in the function call to bbp_maybe_intercept().

That would involve a simple code change like the below example.

That change:

1. Does actually solve the PHP cross-version difference.
2. Also solidifies the PHP 7 behaviour as the desired behaviour.

<?php
// From:
$intercept = bbp_maybe_intercept( 'bbp_pre_get_user_profile_url', func_get_args() );

// To:
$intercept = bbp_maybe_intercept( 'bbp_pre_get_user_profile_url', array( $user_id, $user_nicename ) );

If all this doesn't seem necessary, the original patch is also perfectly fine. This was just my quick attempt at adding clarity to the code.

The original patch only whitelists the warning, it doesn't address the errors.

[johnjamesjacoby]

Related: #3033.

[johnjamesjacoby]

Moving open issues from 2.6.4 to 2.6.5, for 2.6.4 release today.

[johnjamesjacoby]

Patch moves Intercept calls to above variable assignments.

[johnjamesjacoby]

I've attached the alternative approach discussed above.

After a more thorough review of the problem, I'm not sure it's ultimately the way to go though.

Hypothetically, this could introduce the chance for breakage for anyone who calls these functions relying on bbp_get_user_id() to default to the currently displayed user ID.

So, I'm going to suggest that the original patch from @jrf is best correct way to address this problem.

I think it's fine for this inconsistency to exist between PHP versions. The silence of no one having noticed this quirk yet leads me to believe no one's noticed but us. 👀

[jrf]

@johnjamesjacoby Eh.. my patch doesn't address the problem. It only whitelists the one function call which *isn't* a problem.