WP_Http_ExtHTTP response code testing

[arena]

Sometimes http_request return informations that are just warnings and considered by class WP_Http_ExtHTTP has errors.

Testing response_code should be more accurate.

so in class WP_Http_ExtHTTP, line :

if ( false === $strResponse

! empty($infoerror?) )

becomes :

if ($info['response_code'] != 200 && ( false === $strResponse || ! empty($info['error']) ))

[arena]

not possible to edit the ticket

the original line of code is

if ( false === $strResponse || ! empty($info['error']) )

[arena]

content of $info

(
    [effective_url] => http://127.0.0.1/wptest/wp-cron.php?doing_wp_cron
    [response_code] => 200
    [total_time] => 0.486
    [namelookup_time] => 0
    [connect_time] => 0.004
    [pretransfer_time] => 0.004
    [size_upload] => 0
    [size_download] => 0
    [speed_download] => 0
    [speed_upload] => 0
    [header_size] => 165
    [request_size] => 251
    [ssl_verifyresult] => 0
    [filetime] => -1
    [content_length_download] => 0
    [content_length_upload] => 0
    [starttransfer_time] => 0.486
    [content_type] => text/html
    [redirect_time] => 0
    [redirect_count] => 0
    [connect_code] => 0
    [httpauth_avail] => 0
    [proxyauth_avail] => 0
    [os_errno] => 0
    [num_connects] => 1
    [ssl_engines] => Array
        (
        )

    [cookies] => Array
        (
        )

    [error] => Operation timed out after 1 seconds with 30492 out of 222394 bytes received
)

[jacobsantos]

The point of the code is to allow for the developer to see the error and handle it themselves. The WP_HTTP code is not to hold your hand completely, but to unify the retrieval of requests.

I'm saying invalid, however, with the error it does seem as if the time out should be increased.

[Denis-de-Bernardy]

to me, the timeout makes it look as if there was an issue in a plugin or something:

​http://127.0.0.1/wptest/wp-cron.php?doing_wp_cron

Operation timed out after 1 seconds with 30492 out of 222394 bytes received

WP outputs absolutely nothing when that file gets called. Might a plugin be outputting something?

[westi]

Moving to Future Release until we have more info

[arena]

Well the example with 'wp-cron' seems to be irrelevant.

see attached file with 3 relevant urls :

• ​http://www.e24.fr/hightech/?xtdate=12/14/09&service=rss&includeImages=true&endLoop=true
• ​http://www.lemonde.fr/rss/sequence/0,2-651865,1-0,0.xml
• ​http://feediz.01net.com/synd/2203.xml

giving

[error] => Operation timed out after 1 seconds with 0 bytes received

and having a correct response

see attached file result of following hack in http.php :

		// Error may still be set, Response may return headers or partial document, and error
		// contains a reason the request was aborted, eg, timeout expired or max-redirects reached.
print_r($info);
echo $strResponse;
		if ( false === $strResponse || ! empty($info['error']) )

[arena]

results

[Denis-de-Bernardy]

quick question, do we add an ignore_user_abort() call when we spawn a cron job?

[Denis-de-Bernardy]

nm the last comment: we do.

[hakre]

I think what arena is trying to say is, that the HTTP request acutally returned data but the class is reporting it returned 0 bytes which is wrong. Can you confirm arena?

[arena]

Replying to hakre:

I think what arena is trying to say is, that the HTTP request acutally returned data but the class is reporting it returned 0 bytes which is wrong. Can you confirm arena?

@hackre : i confirm

[arena]

patch

[dd32]

A non-200 response is not a error conditional...

[dd32]

Sorry, Didnt read the entire ticket completely.

Is it maybe possible to use !empty($info['response_code'] to cover the non-200 bases?

[arena]

Replying to dd32:

Sorry, Didnt read the entire ticket completely.

Is it maybe possible to use !empty($info['response_code'] to cover the non-200 bases?

what do you mean by "non-200 bases?" ? can you give some samples like in 11428.txt ?

[arena]

Replying to arena:

Replying to dd32:

Sorry, Didnt read the entire ticket completely.

Is it maybe possible to use !empty($info['response_code'] to cover the non-200 bases?

what do you mean by "non-200 bases?" ? can you give some samples like in 11428.txt ?

.
​http://fr2.php.net/http_request

[dd32]

what do you mean by "non-200 bases?" ?

I mean non-200 OK responses, From what i can see from your patch, basically you're saying that even if Error is set to true, but its a 200 response, then it shouldnt recieve an error?

What about a 404 status document that had the same issue?

I cant give any examples since i cant get the darn thing to work at all.

The more i look at this ticket.. The more i keep seeing, This cant be done reliably. In your example, The entire document has been recieved, but the Extension is returning a error condition?

If thats right, Then in your example, If only half the document was recieved, and the same error arrised, What would be the correct thing to do?

I'm thinking, might be best to compare Content-Length: 7185 and [size_download] => 7185 or.. alternativly.. just ignore the extension since its buggy?

What i'm trying to get at is, Your patch seems to me, to say, "Hey, It thinks an error has occured, but i've at least recieved a HTTP header, so everything must be alright!"

[arena]

Replying to dd32:

what do you mean by "non-200 bases?" ?

I mean non-200 OK responses, From what i can see from your patch, basically you're saying that even if Error is set to true, but its a 200 response, then it shouldnt recieve an error?

In the existing code, Error is set to true because $infoerror? (from php function http_request (​http://php.net/http_request)) is not empty ...
but in that case this is just a warning ...

What about a 404 status document that had the same issue?

I cant give any examples since i cant get the darn thing to work at all.

404 is not found, so that is an error !!!

The more i look at this ticket.. The more i keep seeing, This cant be done reliably. In your example, The entire document has been recieved, but the Extension is returning a error condition?

this is just a warning ... (see above)

If thats right, Then in your example, If only half the document was recieved, and the same error arrised, What would be the correct thing to do?

i am sure i would get another http error code (not 200).

I'm thinking, might be best to compare Content-Length: 7185 and [size_download] => 7185 or.. alternativly.. just ignore the extension since its buggy?

What i'm trying to get at is, Your patch seems to me, to say, "Hey, It thinks an error has occured, but i've at least recieved a HTTP header, so everything must be alright!"

all this stuff below is returned by php function http_request (​http://php.net/http_request)

(
....
    [response_code] => 200
....
    [error] => Operation timed out after 1 seconds with 30492 out of 222394 bytes received
)

[dd32]

{{{ (
....

[response_code] => 200

....

[error] => Operation timed out after 1 seconds with 30492 out of 222394 bytes received

)
}}}

Thats not a warning, Thats an error condition that it has only loaded 10% of the document in the timeout period.

If the error item cannot be trusted, then extra validation should take place, such as checking the content length matches the document before declaring the error invalid.

404 is not found, so that is an error !!!

Not in the eyes of the HTTP API, Whilst it may be an HTTP error, That error should still be passed to the calling function - as a HTTP Request object (Since the request was OK, just the content was missing) rather than a WP_Error (Error condition was hit)

[hakre]

Replying to dd32:

Not in the eyes of the HTTP API, Whilst it may be an HTTP error, That error should still be passed to the calling function - as a HTTP Request object (Since the request was OK, just the content was missing) rather than a WP_Error (Error condition was hit)

Please define "Error condition was hit". I do not see any error / exception handling in WP defined/documented so I hardly doubt that with a wishy-washy sentence like "Error condition was hit" anything is explained. I understood you that you're talking for or against a HTTP-Error condition was hit that should be reflected with an WP_Error or not?!?!

[dd32]

Please define "Error condition was hit". I do not see any error / exception handling in WP defined/documented so I hardly doubt that with a wishy-washy sentence like "Error condition was hit" anything is explained.

Sure, "A code line which is not supported by the function, And as such, will return a WP_Error or false to indicate failure of the function"

A "Error Condition" is a case where the function can no longer continue, In the event that [error] => Operation timed out after 1 seconds with 30492 out of 222394 bytes received is recieved, The function cannot continue as it has been given incomplete data. It is an Errro condition. Exception handling is a seperate issue and is not suported by WordPress.

I understood you that you're talking for or against a HTTP-Error condition was hit that should be reflected with an WP_Error or not?!?!

I am seperating the differences between a HTTP level error, and a WordPress error.

A 404 is a HTTP Status code which stands for an error, Nothing more, Nothing less.

That IS the response from the server, Therefor, its not an error where the function can no longer continue. In the event, that the server is unreachable, or returns a imcomplete data stream, That is a Error condition as there is no data which can be trusted as a response from the server.

From a users perspective, A 404 error in the browser is the UI choice. It has been decided that the Users Experience upon hitting a 404 page should present a Not found page and appropriate action taken. The underlying HTTP subsystem would've returned that Status from the server. The Browser would also handle the no-connection-present or no-server error conditionals (Which in WordPress, would be presented as a WP_Error object), Those are Application-level errors, which is distinctivly different from a HTTP-level error.

Looking at the patch, If someone can write a patch which ensures that the error condition is only ignored in the event that the data was 100% returned and valid, then i'm all for it.

But as the patch stands, It mearly saying:

• For any response which is not a 200 and the 'error element' is set, or the response was false
  • Then return a WP_Error
• In the event that the 'error element' is returned, If the headers said it was a 200 response, Ignore the error and return the data.
  • In the event that only "HTTP 200 OK\nHeader: Te" was returned, This says thats a Valid response. Its not. Thats incomplete data, as the 'error element' has said.

('error element': The error array key of the returned data)

[arena]

as far as this is a "Simple and uniform HTTP request API."

maybe it should support http standards (​http://www.w3.org/Protocols/rfc2616/rfc2616.html)

what about only testing ($inforesponse_code? == '200') ?

[dd32]

maybe it should support http standards

It does.

Its up to the implementor to determine how they wish to return HTTP level error conditions. The WP_HTTP class returns a Response object describing the HTTP response, and a WP_Error on failure - Thats it.

what about only testing ($inforesponse_code? == '200') ?

Will that prevent a result being returned with half-full data in the event of a network timeout during the recieve process?

[dd32]

Its up to the implementor to determine how they wish to return HTTP level error conditions. The WP_HTTP class returns a Response object describing the HTTP response, and a WP_Error on failure - Thats it.

A WP_Error upon failure of the WP_HTTP class to make a Successful HTTP request which returns a valid response.

[dd32]

I'm closing this as wontfix, due to my reasonings above.