Fatal error with HTTP class

[momo360modena]

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

[jacobsantos]

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

[jacobsantos]

Probably need to correct the patch.

[jacobsantos]

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

[momo360modena]

Props return WP_Error.

[jacobsantos]

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

[jacobsantos]

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

[westi]

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.

[westi]

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

[jacobsantos]

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.

[hakre]

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.

[hakre]

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

[ryan]

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

[Denis-de-Bernardy]

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.

[DD32]

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.

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

[ryan]

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

[hakre]

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.

[hakre]

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.

[hakre]

Completed patch.

[westi]

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.

[DD32]

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.

[Denis-de-Bernardy]

close as fixed?

[hakre]

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.

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

[hakre]

Propper input sanitization added.

[hakre]

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.

[westi]

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

[hakre]

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.

[hakre]

Update against current trunk, DocBlocks extended to highlight WP_Error.

[hakre]

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

[Denis-de-Bernardy]

traction?

[westi]

Moving to the back burner.

Not removing the is_wp_error() calls

[hakre]

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.

[dd32]

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.

[hakre]

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.

[hakre]

Docblocks Update against Trunk

[dd32]

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.

[dd32]

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

[hakre]

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.

[Denis-de-Bernardy]

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