WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#27656 closed defect (bug) (fixed)

PHP Strict Standards: Only variables should be passed by reference in wp-includes/class-oembed.php on line 139

Reported by: markjaquith Owned by: dd32
Milestone: 3.9 Priority: low
Severity: minor Version:
Component: Embeds Keywords:
Focuses: Cc:

Description

Can't do this all in one line:

if ( $html = wp_remote_retrieve_body( wp_safe_remote_get( $url ) ) ) {

Attachments (1)

27656.diff (549 bytes) - added by markjaquith 6 years ago.

Download all attachments as: .zip

Change History (7)

@markjaquith
6 years ago

#1 @ocean90
6 years ago

There is a similar line in wp-includes/class-wp-xmlrpc-server.php.

#2 @nacin
6 years ago

Yeah, I had noticed these the other day. It appears the byref stuff here is from an older era and should be removed from all wp_remote_retrieve_* functions. I asked dd32 and he concurred.

#3 @nacin
6 years ago

  • Owner set to dd32
  • Status changed from new to assigned

Let's just make a decision one way or another here. I'm fine with just papering over the notice now if we think removing the byref late in a release cycle might have unintended or unpredictable side effects.

These functions never actually modify $response, but $response can be more than just an object. It's WP_Error on failure but otherwise it's a big array on success. (Replacing this with an object with ArrayAccess could be interesting, at some point.)

#4 @dd32
6 years ago

I am almost 100% certain that removing these [the byrefs] will have no detrimental impact. The usage of byref appears to be due to an attempt to keep memory footprint small and avoid the (potentially large) HTTP response being copied multiple times.. that being said, I'm not even sure if that was a risk under PHP4..

If the $response was an object instead of an array (..which it ideally should've been) then we may have had object copying memory-related issues in PHP4 but it wasn't, so there's really no reason why to use a byref here.

I'm on the fence still, a) removing the refs, b) splitting the oembed calls, c) leaving it as-is and removing the refs at the start of the next cycle.. lets face it, this isn't a new warning, although I don't like it as much as the rest of you.

Last edited 6 years ago by dd32 (previous) (diff)

#5 @nacin
6 years ago

I created #27687.

#6 @nacin
6 years ago

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

In 27957:

Don't pass variables by reference.

props markjaquith.
fixes #27656.

Note: See TracTickets for help on using tickets.