Skip to:
Content

bbPress.org

Opened 6 weeks ago

Last modified 5 weeks ago

#3300 new defect

PHP 7.0 cross-version compatibility issue

Reported by: jrf Owned by:
Milestone: 2.6.4 Priority: normal
Severity: normal Version: trunk
Component: General Keywords:
Cc:

Description

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().

Attachments (2)

trac-3300-php-7.0-func-get-args-ok.patch (564 bytes) - added by jrf 6 weeks ago.
Add ignore flag to non-issue
3300.patch (3.6 KB) - added by johnjamesjacoby 6 weeks ago.

Download all attachments as: .zip

Change History (7)

@jrf
6 weeks ago

Add ignore flag to non-issue

#1 @jrf
6 weeks ago

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.

#2 follow-up: @johnjamesjacoby
6 weeks ago

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.

Last edited 6 weeks ago by johnjamesjacoby (previous) (diff)

#3 in reply to: ↑ 2 @jrf
6 weeks ago

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.

#4 @johnjamesjacoby
6 weeks ago

  • Milestone changed from Awaiting Review to 2.6.4
Note: See TracTickets for help on using tickets.