WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

#18975 closed defect (bug) (fixed)

[E_STRICT] menu.php at line 218

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

Description

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

Attachments (6)

#18975.patch (551 bytes) - added by arena 5 years ago.
18975.2.patch (1.1 KB) - added by SergeyBiryukov 5 years ago.
18975.diff (15.5 KB) - added by ryan 5 years ago.
18975.3.patch (2.3 KB) - added by SergeyBiryukov 5 years ago.
18975.4.patch (528 bytes) - added by scribu 5 years ago.
18975.5.diff (599 bytes) - added by jdgrimes 3 years ago.

Download all attachments as: .zip

Change History (33)

@arena
5 years ago

#1 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 3.3

#2 @nacin
5 years ago

[18110] should probably be reverted.

Also, [18995].

#3 @ryan
5 years ago

In [18997]:

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

#4 @ryan
5 years ago

In [18998]:

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

#5 @ryan
5 years ago

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

#6 @arena
5 years ago

  • 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

#7 @arena
5 years ago

another one :

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

#8 @SergeyBiryukov
5 years ago

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.

#9 @ryan
5 years ago

In [19072]:

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

@ryan
5 years ago

#10 follow-up: @ryan
5 years 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.

#11 @SergeyBiryukov
5 years ago

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

#12 @ryan
5 years ago

In [19074]:

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

#13 @scribu
5 years ago

  • 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 5 years ago by scribu (previous) (diff)

#14 @scribu
5 years ago

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

#15 @scribu
5 years ago

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

#16 @scribu
5 years ago

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

#17 @scribu
5 years ago

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

#18 @nacin
5 years ago

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.

#19 @scribu
5 years ago

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.

#20 @scribu
5 years ago

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

@scribu
5 years ago

#21 @scribu
5 years ago

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

#22 @ryan
5 years ago

In [19094]:

Avoid E_STRICT notices. see #18975

#23 @ryan
5 years ago

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

#24 @ryan
5 years ago

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

In [19095]:

Use key(). Props scribu. fixes #18975

#25 in reply to: ↑ 10 @hakre
4 years 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)

#26 follow-up: @jdgrimes
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Just found another one of these notices today:

PHP Strict Standards:  Only variables should be passed by reference in /wp-admin/widgets.php on line 185

It only shows itself when using widget accessibility mode. Attaching a patch.

@jdgrimes
3 years ago

#27 in reply to: ↑ 26 @SergeyBiryukov
3 years ago

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

Replying to jdgrimes:

Just found another one of these notices today

This ticket was closed on a completed milestone. Moved to #25225.

Note: See TracTickets for help on using tickets.