Skip to:
Content

bbPress.org

Opened 22 months ago

Last modified 15 months ago

#3300 assigned defect

PHP 7.0 cross-version compatibility issue

Reported by: jrf Owned by: johnjamesjacoby
Milestone: 2.7 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 (3)

trac-3300-php-7.0-func-get-args-ok.patch (564 bytes) - added by jrf 22 months ago.
Add ignore flag to non-issue
3300.patch (3.6 KB) - added by johnjamesjacoby 22 months ago.
3300.2.patch (4.9 KB) - added by johnjamesjacoby 16 months ago.
Patch moves Intercept calls to above variable assignments.

Download all attachments as: .zip

Change History (13)

@jrf
22 months ago

Add ignore flag to non-issue

#1 @jrf
22 months 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
22 months 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 22 months ago by johnjamesjacoby (previous) (diff)

#3 in reply to: ↑ 2 @jrf
22 months 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
22 months ago

  • Milestone changed from Awaiting Review to 2.6.4

#6 @johnjamesjacoby
20 months ago

  • Milestone changed from 2.6.4 to 2.6.5

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

#7 @johnjamesjacoby
16 months ago

  • Milestone changed from 2.6.5 to 2.6.6

@johnjamesjacoby
16 months ago

Patch moves Intercept calls to above variable assignments.

#8 @johnjamesjacoby
16 months ago

  • Owner set to johnjamesjacoby
  • Status changed from new to assigned

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. 👀

#9 @johnjamesjacoby
16 months ago

  • Milestone changed from 2.6.6 to 2.7

#10 @jrf
15 months ago

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

Note: See TracTickets for help on using tickets.