WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#8620 closed defect (bug) (fixed)

Fatal error with HTTP class

Reported by: momo360modena Owned by: westi
Milestone: 2.8 Priority: normal
Severity: major Version: 2.7
Component: HTTP API Keywords: has-patch commit
Focuses: Cc:

Description

I have a small error with a personal plugin when i use the new HTTP class.

Fatal error: Cannot use object of type WP_Error as array in /wp-includes/http.php on line 1215

I use this code in my plugin:

$reponse = wp_remote_get( $clean_url );
if( !is_wp_error($response) && $reponse != null ) {
 $code = wp_remote_retrieve_response_code($reponse);
 if ( $code == 200 ) {
  $image_raw = wp_remote_retrieve_body($reponse);
 }
}

This code is under a foreach loop. (20 items by 20 items)

I add a var_dump() into the function wp_remote_retrieve_response_code()
The ouput when i have an error is:

WP_Error::__set_state(array(
   'errors' => 
  array (
    'http_request_failed' => 
    array (
      0 => 'Operation timed out after 5000 milliseconds with 0 bytes received',
    ),
  ),
   'error_data' => 
  array (
  ),
))

I suggest a patch with this ticket.
My PHP version : PHP Version 5.2.6-2+b1

Attachments (11)

http.diff (2.1 KB) - added by momo360modena 5 years ago.
http.2.diff (2.2 KB) - added by momo360modena 5 years ago.
Props return WP_Error.
http.2.2.diff (1.7 KB) - added by momo360modena 5 years ago.
8620-http-php-api--method-improvements-docblocks-and-robustness.patch (1.7 KB) - added by hakre 5 years ago.
patch as describben in my comment. fixes 8620 in the current codebase and corrects return types (in docblocks)
8620.diff (4.0 KB) - added by DD32 5 years ago.
8620-isset-works-best-patch.patch (1.7 KB) - added by hakre 5 years ago.
isset() just checks and does not throw any warings. but the last patch can (because you did't check subitems or arrays). less code, more functionality. KISS by me.
8620-isset-works-best-patch-complete.patch (2.5 KB) - added by hakre 5 years ago.
Completed patch.
8620-isset-works-best-patch-less-complex-flow.patch (2.6 KB) - added by hakre 5 years ago.
8620-isset-works-best-patch-less-complex-flow-with-input-sanitization.patch (2.5 KB) - added by hakre 5 years ago.
Propper input sanitization added.
8620.patch (3.4 KB) - added by hakre 5 years ago.
Update against current trunk, DocBlocks extended to highlight WP_Error.
8620.docblocks.patch (2.4 KB) - added by hakre 5 years ago.
Docblocks Update against Trunk

Download all attachments as: .zip

Change History (51)

momo360modena5 years ago

comment:1 jacobsantos5 years ago

It is actually supposed to return the WP_Error class, so you can handle it yourself.

comment:2 jacobsantos5 years ago

Probably need to correct the patch.

comment:3 jacobsantos5 years ago

Check for WP_Error and return it, before getting to the array parts.

momo360modena5 years ago

Props return WP_Error.

momo360modena5 years ago

comment:4 DD325 years ago

  • Keywords has-patch added

comment:5 jacobsantos5 years ago

You are supposed to check for WP_Error before using those helper functions.

comment:6 jacobsantos5 years ago

Damn, the code upstream to the helper functions are being fixed to prevent them from having a WP_error in the body.

comment:7 jacobsantos5 years ago

  • Component changed from General to HTTP
  • Owner anonymous deleted

comment:8 westi5 years ago

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

I'm 50/50 on this change.

I think the most important improvement is to update the phpDoc to empasise that the main HTTP API functions will return WP_Error objects if an error occurs.

You could argue that these helper functions should not need to deal with the WP_Error objects as the code should test for that before calling them.

comment:9 westi5 years ago

(In [10359]) update phpdoc to hilight return of WP_Error objects on failures. See #8620.

comment:10 jacobsantos5 years ago

Sadly, the phpdoc is still wrong. Those functions return arrays instead of a string. I think that phpdoc was left over from the old patches that would have saved the object in an global and used that global for retrieving the body and such.

comment:11 hakre5 years ago

First off, naturally every public function should sanitize the input to the function. because this is all php4, it might look like a little bit overheaded, so w/o answering this question a fix like the suggested one to "fix it in the phpdocs" sounds valuable anyway.

i checked the source in /wp-includes/http.php for php doc comments and i saw those are _not_ in now. infact some comments look like just duplicated from one function to the other rendering them useless.

  • wp_remote_retrieve_response_code()
  • wp_remote_retrieve_response_message()

additionally related to this report, wp_remote_retrieve_response_code(), does not return WP_Error on failure any longer. It returns an empty string on failure. it might throw a warning, if there exists a response entry in the $response array but if there is no code. this should be checked (compareable to wp_remote_retrieve_header). a similar mistake is found in wp_remote_retrieve_response_message().

I created a patch that does make some minor changes to the code in the hope it gets mature. This is against the current SVN and contains modifed docblocks reflecting the correct return types. additionally further checks have been provided sothat no warning will be thrown if array indexes do not exist. those check were previously incomplete and could mostly be reduced to one isset command.

patch will follow.

hakre5 years ago

patch as describben in my comment. fixes 8620 in the current codebase and corrects return types (in docblocks)

comment:12 Denis-de-Bernardy5 years ago

  • Keywords tested added
  • Milestone changed from 2.7.2 to 2.8

comment:13 ryan5 years ago

I've lost where this thread is going. DD32, jacobsantos, should I be applying the latest patch?

comment:14 Denis-de-Bernardy5 years ago

hakre's patch is functionally the same as the current code base, only rewritten. (it's the one I looked into.)

else, there is a slight complaint from jacob on phpdoc. westi patched the rest of the ticket, which should be closed as fixed.

comment:15 DD325 years ago

I'll test this and see if anything needs to be done. I have a feeling it might also need a check to see if the request WP_Error'd myself..

The PHPDoc does need updating though.

comment:16 ryan5 years ago

  • Keywords has-patch tested removed

DD325 years ago

comment:17 follow-up: DD325 years ago

  • Keywords has-patch added

attachment 8620.diff added

  • A few PHPDoc updates based on previous patches and comments
  • Added a WP_Error check on the helper functions (Note: The example code in the OP's description would never cause that error, due to the is_wp_error() check, But not everyone is going to do that check, as evident by that error message)
  • I didnt see the point of Denis's isset() changes, infact, I suspect they'd cause notices to be thrown (isset($arrone?two?) throws a one is unset i think under some conditions)

comment:18 ryan5 years ago

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

(In [11091]) phpdoc udpates and error checks for http. Props DD32. fixes #8620

hakre5 years ago

isset() just checks and does not throw any warings. but the last patch can (because you did't check subitems or arrays). less code, more functionality. KISS by me.

comment:19 in reply to: ↑ 17 ; follow-up: hakre5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to DD32:

attachment 8620.diff added

  • A few PHPDoc updates based on previous patches and comments
  • Added a WP_Error check on the helper functions (Note: The example code in the OP's description would never cause that error, due to the is_wp_error() check, But not everyone is going to do that check, as evident by that error message)
  • I didnt see the point of Denis's isset() changes, infact, I suspect they'd cause notices to be thrown (isset($arrone?two?) throws a one is unset i think under some conditions)

Added an additional patch to remove some faults in the code. First of all, I am pretty shure there is no collision between a valid $response and wp_error. So it was hardworking to add the checks everywhere, but those should not be really necessary because isset() does a propper job here.

Infact, the (In [11091]) patch will throw errors/notices/warnings if $responseresponse? is set and an array but does not contain a key called 'message'.

Isset() instead never notices on an unset variable or array by definition (otherwise please provide proof). Therefore my little patch adds it to the functions I had it in earlier. I might add another one soon cleaning out the other unnecessary and maybe incomplete checks.

hakre5 years ago

Completed patch.

comment:20 westi5 years ago

If we expect that a WP_Error maybe returned we should always do the test for it explicitly.

Also nesting the condition operators like that is very bad coding style.

So I am -1 to current patch.

comment:21 in reply to: ↑ 19 ; follow-up: DD325 years ago

Replying to hakre:

Added an additional patch to remove some faults in the code. First of all, I am pretty shure there is no collision between a valid $response and wp_error. So it was hardworking to add the checks everywhere, but those should not be really necessary because isset() does a propper job here.

You're correct, A valid $response (ie. not WP_Error) should be passed, Thats not always the case though. However, isset() does not work as you expect:

$a = (object) array();

isset($a['test']);

( ! ) Fatal error: Cannot use object of type stdClass as array in ...\1240904033.php on line ...

Infact, the (In [11091]) patch will throw errors/notices/warnings if $responseresponse? is set and an array but does not contain a key called 'message'.

Pretty useless there though, The assumption is, That if $response['response'] is set, Then its a valid HTTP return, The return will ALWAYS include the message and code, even if they're blank.

Isset() instead never notices on an unset variable or array by definition (otherwise please provide proof). Therefore my little patch adds it to the functions I had it in earlier. I might add another one soon cleaning out the other unnecessary and maybe incomplete checks.

My recolection (of notices that came up while jacob was writing the HTTP clases) was that under a certain version of (supported) PHP, isset($a['test']['test'] causes a notice when the first array indicy wasnt set... No specifics though. But eitherway, Its utterly pointless there given the previous mention on the fact of valid response values.

And, -1 to the extra complexity of the if statements, They dont gain anything, Just more complex code, Theres nothing to be afraid of with extra whitespace and 2 if's instead of 1, While the extra if adds an extra 0.01ms, Its not code thats being called a hundred times a pageload, optimizations can be left alone for easier readability.

comment:22 Denis-de-Bernardy5 years ago

close as fixed?

comment:23 in reply to: ↑ 21 hakre5 years ago

Replying to DD32:

You're correct, A valid $response (ie. not WP_Error) should be passed, Thats not always the case though. However, isset() does not work as you expect:

$a = (object) array();

isset($a['test']);

( ! ) Fatal error: Cannot use object of type stdClass as array in ...\1240904033.php on line ...

well, that fatal error occurs in that line:

$a = (object) array();

, right? this is not error message created by isset(), it is created by the cast.

Infact, the (In [11091]) patch will throw errors/notices/warnings if $responseresponse? is set and an array but does not contain a key called 'message'.

Pretty useless there though, The assumption is, That if $response['response'] is set, Then its a valid HTTP return, The return will ALWAYS include the message and code, even if they're blank.

Well if you even test for having a specific object in here (WP_Error), then you won't test for the right array item? That seems a pretty ambiguous argumentation to me.

And, -1 to the extra complexity of the if statements, They dont gain anything, Just more complex code, Theres nothing to be afraid of with extra whitespace and 2 if's instead of 1, While the extra if adds an extra 0.01ms, Its not code thats being called a hundred times a pageload, optimizations can be left alone for easier readability.

I will reduce the complexity sothat it's written more nice.

comment:24 follow-up: DD325 years ago

, right? this is not error message created by isset(), it is created by the cast.

No, Its caused by treating an Object as an array, Even when used with isset.

hakre5 years ago

Propper input sanitization added.

comment:25 in reply to: ↑ 24 hakre5 years ago

Replying to DD32:

, right? this is not error message created by isset(), it is created by the cast.

No, Its caused by treating an Object as an array, Even when used with isset.

True. Added sanitization now.

comment:26 westi5 years ago

  • Keywords needs-patch added; has-patch removed

Patch doesn't apply cleanly.

I would still prefer to see explicit check for wp_error to hilight that it is what is expected here if it's not an array()

comment:27 hakre5 years ago

well, then i would suggest to add that into the docblock comment. and not inside the code, since the check would be only within the function and would not change anything otuside of it.

hakre5 years ago

Update against current trunk, DocBlocks extended to highlight WP_Error.

comment:28 hakre5 years ago

  • Keywords has-patch added; needs-patch removed

comment:29 hakre5 years ago

fixed patch, incorporated westis suggestion to highlight WP_Error to the developer, implemented in the functions docblock.

comment:31 ryan5 years ago

  • Priority changed from high to normal

comment:32 westi5 years ago

  • Milestone changed from 2.8 to 2.9

Moving to the back burner.

Not removing the is_wp_error() calls

comment:33 hakre5 years ago

There is defenetly no need to have a is_wp_error() call inside the functions. It will have no difference in the API to have them in or not. It will only slow things down if you put them in

If you think there is need to have is_wp_errors() calls in there, please detail your arguments.

Also I see no problem to have this in 2.8.

comment:34 dd325 years ago

  • Milestone changed from 2.9 to 2.8
  • Resolution set to fixed
  • Status changed from reopened to closed

There is defenetly no need to have a is_wp_error() call inside the functions. It will have no difference in the API to have them in or not. It will only slow things down if you put them in

For code that is called once in a lifetime, A bit extra processing time is OK.

Valid responses from upstream are an array, or WP_Error object. These functions take the result from an upstream function, and return information about it. Therefor, They need to handle WP_Error objects as shown by the OP here, as not everyone is going to test for a error condition first.

Your patches add absolutely -nothing- over the current implementation, Infact, Casting to an array is a bandaid idea IMO. With keeping the current wp_error checks, I see absolutely no way that your proposed changes could speed it up at all, Testing variable type is faster than casting it anyway.

I'm closing this as fixed. Feel free to attempt to sway minds, But as you've seen above, No-one is interested in commiting it right now. Open a new ticket for a future release if you see fit.

comment:35 hakre5 years ago

Progressivly I would say it can make sense to merge the docblocks in. I can take care for that.

For Strictness I'm OK with your argumentation but therefore I would wish to see that everywhere in the core functions. But that is not the case. Good to read your words for this case.

I can not say if object testing is faster then object casting or the other way round. AFAIK casting is pretty fast in PHP often faster then doing a conversion. So I thought not that negativly about casting. But I have not benchmarked it. Are you shure about your assumptions?

My patch made the code more simple and extended the Docblock. That was what it was made for. I will patch the dockblock only against trunk now.

hakre5 years ago

Docblocks Update against Trunk

comment:36 hakre5 years ago

  • Keywords developer-feedback added

comment:37 dd325 years ago

You seem to be right about the cast vs is_array in PHP5, That wasnt the case under PHP4 though.

3 test cases, run 10,000 each over 2 valid, and 2 invalid cases.

Function1, as it currently is in core, 0.000035s /run
Function2, As you promosed it, 0.000014s/run
Function3, As below, 0.000009s/run

function wp_remote_retrieve_headers2(&$response) { 
 if ( is_object($response) || empty($response['headers']) ) 
  return array(); 
 return $response['headers']; 
}

is_wp_error() is the expensive part, it accounted for about 0.00002s of the total function time, which is about at a rough guess, is about 60% of the total function time. It doesnt matter what kind of input was given, checking if its an object, and then if the array indicy is empty, is the fastest method.

..But i'm still -1 to changing the damn thing to something more unreadable to pull out an extra 0.0002ms speed increase for a function which is rarely used, and even rarer used in a loop. Heck, The HTTP request parsing alone uses more than 10,000x the time i bet.. actually, its closer to 100,000x - 2seconds for the request itself to take place.

comment:38 dd325 years ago

(Yes, that function is called 2, but it was actually the 3rd test case)

comment:39 hakre5 years ago

dd32 thanks for benchmarking, it's always good to have some numbers. As said, I'm okay with having a strict var handling inside the function. It's not always this or that, let's take the best of both. Especailly if that function is not called that often.

Feel free to commit the docblock changes, that will improve for all devs. This might be really important because now, the function does not throw a fatal error any longer and therefore a developler might not be aware what is going wrong. Having a hint in the docblocks might save me and other devs some time in the future.

My 2 Cents.

comment:40 Denis-de-Bernardy5 years ago

  • Keywords commit added; developer-feedback removed

+1 for the docblocks. let's close this ticket, it has been open for too long.

Note: See TracTickets for help on using tickets.