Opened 19 months ago

Closed 19 months ago

Last modified 12 months ago

#18975 closed defect (bug) (fixed)

[E_STRICT] menu.php at line 218

Reported by: arena Owned by: ryan
Priority: normal Milestone: 3.3
Component: Menus Version: 3.3
Severity: normal Keywords: dev-feedback has-patch
Cc:

Description

PHP [E_STRICT] 2048 : Only variables should be passed by reference in ........wordpress\wp-admin\includes\menu.php at line 218

Attachments (5)

#18975.patch (551 bytes) - added by arena 19 months ago.
18975.2.patch (1.1 KB) - added by SergeyBiryukov 19 months ago.
18975.diff (15.5 KB) - added by ryan 19 months ago.
18975.3.patch (2.3 KB) - added by SergeyBiryukov 19 months ago.
18975.4.patch (528 bytes) - added by scribu 19 months ago.

Download all attachments as: .zip

Change History (30)

arena19 months ago

  • Milestone changed from Awaiting Review to 3.3

[18110] should probably be reverted.

Also, [18995].

In [18997]:

Avoid pass by ref warning. Props arena. see #18975

In [18998]:

Avoid 'Only variables should be passed by reference' warnings. Reverts [18110]. see #18975

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

I re open this ticket because

  • the code such as in post.php
$dir = array_shift($keys = array_keys($dirs));

still gives a E_STRICT PHP error
PHP [E_STRICT] 2048 : Only variables should be passed by reference in D:\webserver\htdocs\wordpress\wp-admin\includes\menu.php at line 218

  • same for following code
$last_menu_key = array_pop( $last_menu_key = array_keys( $menu ) );

PHP [E_STRICT] 2048 : Only variables should be passed by reference in D:\webserver\htdocs\wordpress\wp-includes\post.php at line 4036

another one :

[E_STRICT] 2048 : Only variables should be passed by reference in wordpress\wp-includes\general-template.php at line 1624

Apparently this doesn't work as expected:

$last_menu_key = array_pop( $last_menu_key = array_keys( $menu ) );

Here's what I see on Posts screen in 3.3-beta2-19067 on PHP 5.3.3 with E_ALL | E_STRICT:

Strict Standards: Only variables should be passed by reference in wp-includes/pomo/mo.php on line 198
Strict Standards: Only variables should be passed by reference in wp-includes/pomo/mo.php on line 198
Strict Standards: Declaration of Walker_Category_Checklist::start_lvl() should be compatible with that of Walker::start_lvl() in wp-admin/includes/template.php on line 52
Strict Standards: Declaration of Walker_Category_Checklist::end_lvl() should be compatible with that of Walker::end_lvl() in wp-admin/includes/template.php on line 52
Strict Standards: Declaration of Walker_Category_Checklist::start_el() should be compatible with that of Walker::start_el() in wp-admin/includes/template.php on line 52
Strict Standards: Declaration of Walker_Category_Checklist::end_el() should be compatible with that of Walker::end_el() in wp-admin/includes/template.php on line 52
Strict Standards: Only variables should be passed by reference in wp-admin/includes/menu.php on line 218
Strict Standards: Non-static method WP_MatchesMapRegex::apply() should not be called statically, assuming $this from incompatible context in wp-includes/class-wp.php on line 222

18975.2.patch fixes menu.php and mo.php. Doesn't touch the rest for now.

In [19072]:

Avoid 'Only variables should be passed by reference' warnings. Props SergeyBiryukov. see #18975

ryan19 months ago

comment:10 follow-up: ↓ 25   ryan19 months ago

That fixes all strict warning I see except for:

PHP Strict Standards:  Redefining already defined constructor for class WP_Widget in /.../trunk/wp-includes/widgets.php on line 93

We'll have to keep that PHP4 constructor around for a time.

Perhaps [18998] should also be altered (see 18975.3.patch).

In [19074]:

Avoid 'Only variables should be passed by reference' warnings. Props SergeyBiryukov. see #18975

  • Keywords needs-patch added; has-patch removed

It's more efficient to use reset() and end() instead of array_shift() and array_pop(), since we don't use $var_by_ref afterwards.

Last edited 19 months ago by scribu (previous) (diff)

And there's also key(), if needed.

I know it's not exactly related to this ticket, but as long as we're here...

Also, this line is rather silly:

array_map('wp_setup_nav_menu_item', array( get_post( $var_by_ref ) ) )

It can just be:

array( wp_setup_nav_menu_item( get_post( $var_by_ref ) ) )

Or, we can just use list( ) = and avoid $var_by_ref altogether. Patch incoming.

IIRC, reset() and end() don't work as you might expect, depending on the order items were added to the array. ryan seems to recall the same thing but we could both be off. end(), key(), and reset() all require references like array_shift() and array_pop(), so they don't eliminate the requirement for the variable. I'd love to see a patch that changes our understanding here.

reset() and end() don't work as you might expect, depending on the order items were added to the array.

Yes, in the sense that order matters, even for associative arrays:

array( 'a' => 1, 'b' => 2 ) vs. array( 'b' => 2, 'a' => 1 )

but array_shift() and array_pop() follow the same rule.

end(), key(), and reset() all require references like array_shift() and array_pop(), so they don't eliminate the requirement for the variable.

They don't eliminate the requirement for the variable, but they avoid all the work that array_shift() and array_pop() do, which can be significant, especially in the case of wp-db.php.

Actually, I might be exaggerating how much extra work array_pop() and array_shift() do. Let me run some tests.

  • Keywords has-patch added; needs-patch removed

Yep, the difference in performance between array_shift() and array_pop() vs. reset() and end() is unnoticeable, even for arrays with thousands of items.

So 18975.4.patch is just a single conversion to key(), since I think it makes the code easier to understand.

In [19094]:

Avoid E_STRICT notices. see #18975

That's 18975.diff sans walkers. Those can wait.

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from reopened to closed

In [19095]:

Use key(). Props scribu. fixes #18975

comment:25 in reply to: ↑ 10   hakre12 months ago

Replying to ryan:

That fixes all strict warning I see except for:

PHP Strict Standards:  Redefining already defined constructor for class WP_Widget in /.../trunk/wp-includes/widgets.php on line 93

We'll have to keep that PHP4 constructor around for a time.

The strict error can be cured while keeping the PHP 4 constructor: #20801 (and even giving deprecated notices)

Note: See TracTickets for help on using tickets.