#18975 closed defect (bug) (fixed)
[E_STRICT] menu.php at line 218
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| 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)
Change History (30)
comment:1
SergeyBiryukov — 19 months ago
- Milestone changed from Awaiting Review to 3.3
- 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
SergeyBiryukov — 19 months ago
comment:8
SergeyBiryukov — 19 months 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:10
follow-up:
↓ 25
ryan — 19 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.
SergeyBiryukov — 19 months ago
Perhaps [18998] should also be altered (see 18975.3.patch).
comment:12
ryan — 19 months ago
In [19074]:
comment:13
scribu — 19 months 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.
comment:14
scribu — 19 months ago
And there's also key(), if needed.
comment:15
scribu — 19 months ago
I know it's not exactly related to this ticket, but as long as we're here...
comment:16
scribu — 19 months 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
scribu — 19 months ago
Or, we can just use list( ) = and avoid $var_by_ref altogether. Patch incoming.
comment:18
nacin — 19 months 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
scribu — 19 months 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
scribu — 19 months ago
Actually, I might be exaggerating how much extra work array_pop() and array_shift() do. Let me run some tests.
comment:21
scribu — 19 months 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
ryan — 19 months ago
In [19094]:
comment:23
ryan — 19 months ago
That's 18975.diff sans walkers. Those can wait.
comment:24
ryan — 19 months ago
- Owner set to ryan
- Resolution set to fixed
- Status changed from reopened to closed
In [19095]:
comment:25
in reply to:
↑ 10
hakre — 12 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 93We'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)

[18110] should probably be reverted.
Also, [18995].