WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 8 months 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 3 years ago.
18975.2.patch (1.1 KB) - added by SergeyBiryukov 2 years ago.
18975.diff (15.5 KB) - added by ryan 2 years ago.
18975.3.patch (2.3 KB) - added by SergeyBiryukov 2 years ago.
18975.4.patch (528 bytes) - added by scribu 2 years ago.
18975.5.diff (599 bytes) - added by jdgrimes 8 months ago.

Download all attachments as: .zip

Change History (33)

arena3 years ago

comment:1 SergeyBiryukov3 years ago

  • Milestone changed from Awaiting Review to 3.3

comment:2 nacin3 years ago

[18110] should probably be reverted.

Also, [18995].

comment:3 ryan3 years ago

In [18997]:

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

comment:4 ryan3 years ago

In [18998]:

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

comment:5 ryan3 years ago

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

comment:6 arena2 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

comment:7 arena2 years ago

another one :

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

SergeyBiryukov2 years ago

comment:8 SergeyBiryukov2 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.

comment:9 ryan2 years ago

In [19072]:

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

ryan2 years ago

comment:10 follow-up: ryan2 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.

SergeyBiryukov2 years ago

comment:11 SergeyBiryukov2 years ago

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

comment:12 ryan2 years ago

In [19074]:

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

comment:13 scribu2 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 really use $var_by_ref afterwards.

Version 0, edited 2 years ago by scribu (next)

comment:14 scribu2 years ago

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

comment:15 scribu2 years ago

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

comment:16 scribu2 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 ) ) )

comment:17 scribu2 years ago

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

comment:18 nacin2 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.

comment:19 scribu2 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.

comment:20 scribu2 years ago

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

scribu2 years ago

comment:21 scribu2 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.

comment:22 ryan2 years ago

In [19094]:

Avoid E_STRICT notices. see #18975

comment:23 ryan2 years ago

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

comment:24 ryan2 years ago

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

comment:26 follow-up: jdgrimes8 months 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.

jdgrimes8 months ago

comment:27 in reply to: ↑ 26 SergeyBiryukov8 months 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.