Make WordPress Core

Opened 14 years ago

Closed 12 years ago

Last modified 12 years ago

#13757 closed defect (bug) (fixed)

Passing functions as call by ref parameter should be avoided

Reported by: tobiasbg's profile TobiasBg Owned by: westi's profile westi
Milestone: 3.3 Priority: normal
Severity: normal Version:
Component: Warnings/Notices Keywords: has-patch
Focuses: Cc:

Description

I recently had a bug report for a plugin, where script execution was stopped at an avoidable warning.

The problem was a call similar to

$my_array = array( ... );
$last_element_index = array_pop( array_keys( $my_array ) );

The reason for the issue is that the parameter for array_pop() is passed as a call by reference parameter. This leads to a problem here, because array_keys( $my_array ) is not a variable that could be changed during the call by reference process (in this case, the last element can not be removed from the array).
This raises a warning by PHP (also documented at www.php.net/array_pop), and there are hosts that seem to be stopping PHP execution at such warning.

Fixing the issue is very easy by adding a variable:

$my_array = array( ... );
$_my_array_keys = array_keys( $my_array );
$last_element_index = array_pop( $_my_array_keys );

As I had this issue in my plugin, I also grep'ed WP core and found two instances of array_pop( array_keys() ):

  • admin-ajax.php (line 893)
  • ms-edit.php (line 107)

With array_pop( explode() ):

  • wp-app.php (line 226)

With array_pop( split() ):

  • atomlib.php (lines 151 and 230)

There are more functions (especially within array_*) that are affected, and that should be checked on whether a function call is passed where a call by ref parameter is expected.

Attachments (1)

13757.patch (2.3 KB) - added by TobiasBg 14 years ago.
Patch to replace functions as arguments with temporary variables, to prevent warning

Download all attachments as: .zip

Change History (21)

#1 @scribu
14 years ago

array_pop() should be replaced with end()

array_shift() should be replaced with reset()

but a temporary variable is still needed.

#2 @TobiasBg
14 years ago

  • Keywords has-patch added; needs-patch removed

I attached a first pass for a patch.

Using the regexp array_(pop|push|shift|unshift)+\(\s?\w.+\s?\) I grep'd for calls of array_pop, array_push, array_shift, array_unshift which do not have a variable as its argument.

This lead to one more instance of such a case in wp-db.php, additionally to the above mentioned.

In the patch, I replaced all the calls with temporary variables (the names might not be the best...).

I intentionally left out the two findings in atomlib.php, as that seems to be an external library we are using.

@TobiasBg
14 years ago

Patch to replace functions as arguments with temporary variables, to prevent warning

#3 @scribu
14 years ago

  • Component changed from General to Warnings/Notices
  • Owner set to westi

Looks good.

#4 @TobiasBg
14 years ago

Just noted that mentioning array_push and array_unshift was actually nonsense, because they don't even make sense without a variable passed as the argument, so ignore those.

However, other functions might still be affected.

#5 @nacin
14 years ago

I understand the variable name case is a good faith effort, but it's kind of confusing.

Patch looks fine so far though.

#6 @scribu
14 years ago

Just realized that instead of array_shift( array_keys( $var ) ) you could simply use key( $var ).

Too bad there's no equivalent for array_pop( array_keys( $var ) ).

#7 @TobiasBg
14 years ago

I guess, key( array_reverse( $var, true ) ) would work for that, but it's a whole lot more confusing, I guess :-)

#8 @westi
14 years ago

  • Milestone changed from 3.0 to 3.1

I don't think this is a blocker for 3.0 release.

Moving to 3.1 (we can always back port into 3.0.1 if required)

#9 @westi
14 years ago

  • Milestone changed from Awaiting Triage to 3.0.1

It turns out this may be more of an issue that I realised.

It appears that PHP since 5.1 may have a bug related to functions which take references being called with function results as arguments.

Moving into 3.0.1 for consideration

#10 @nacin
14 years ago

Related, #14160.

#11 @westi
14 years ago

  • Milestone changed from 3.0.1 to 3.1

I think we need to soak test this in trunk before backporting to the 3.0 branch and ensure we don't introduce any other subtle bugs.

We are changing the code from returning values to references with the current patch I think due to the implementation details of end and reset.

So I guess it might be better to re-invent in another way.

#12 @TobiasBg
14 years ago

Maybe we should not change the functions that are used for now (i.e. don't replace array_pop() with end()), but just use the temporary locale variable part of the suggested solution. After all the issue is raised by the call by reference in some versions of PHP and not the used functions.

#13 @nacin
14 years ago

  • Milestone changed from 3.1 to 3.0.1

Currently running some tests on a 5.0.5 install, where this causes a fatal error. Thus, this the same bug as #14160.

The easiest fix, as noted in one of the PHP bugs linked to from #14160, is just assigning a variable first. Thus,

$last_element_index = array_pop( array_keys( $my_array ) );

becomes:

$last_element_index = array_pop( $var_by_ref = array_keys( $my_array ) );

I'm going to look through the instances cited in this patch and try to fix. (Pretty much what TobiasBg said.) We can then soak the rest in trunk come 3.1 development.

#14 @nacin
14 years ago

(In [15470]) More curses on PHP 5.0.5. see #13757, fixes #14160. for 3.0.1.

#15 @nacin
14 years ago

(In [15471]) More curses on PHP 5.0.5. see #13757, fixes #14160. for trunk.

#16 @nacin
14 years ago

  • Keywords early added
  • Milestone changed from 3.0.1 to 3.1

Okay, that should suffice for now. Moving to 3.1 early.

#17 @nacin
13 years ago

  • Keywords 3.2-early added; early removed
  • Milestone changed from 3.1 to Future Release

Happy with this for 3.1.

#18 @nacin
13 years ago

  • Keywords 3.2-early removed

See [18109], end() isn't always equivalent.

#19 @TobiasBg
12 years ago

  • Resolution set to fixed
  • Status changed from new to closed

Closing this as "fixed somewhere along the road".

I grep'd again, and the only find was something like this in atomlib.php:

$tag = array_pop(split(":", $name));

But since that's an external lib, I'm not sure if we'll mess with it at all. And as the split() (deprecated in PHP 5.3.0) would need to be replaced by explode() anyway, this can then be dealt with together.

New occurances of PHP warnings/notices related to function calls being used as a call-by-ref parameter should get a new ticket.

P.S.: Someone could change the Milestone again, if necessary, probably to 3.2. (I seem to lack permissions for that.)

#20 @SergeyBiryukov
12 years ago

  • Milestone changed from Future Release to 3.3
Note: See TracTickets for help on using tickets.