Posted by Dave Reid on November 22, 2008 at 4:33pm
5 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Dave Reid |
| Status: | closed (fixed) |
Issue Summary
I feel like drupal_http_request is a good candidate to have it's parameters array-itized like url/l/etc. It's new definition would be:
<?php
function drupal_http_request($url, array $options = array()) {
$options += array(
'headers' => array(),
'method' => 'GET',
'data' => NULL,
'retry' => 3,
);
?>Providing patch shortly.
Comments
#1
Initial patch. I ran the tests that could be affected by the changes: aggregator, xml-rpc, and drupal_http_request tests (there are no openid tests) and I had 100% pass.
Edit: removed patch.
#2
Arg...wasn't the latest patch. This one is good.
#3
+ * (optional) An array which can have any or all of the following keys:Isn't "one or more" instead of "any or all" better?
I know that you are just reformatting the existing Doxygen comment, but I suggest updating the description to be more in line with the terminology used in RFC 2616 now that you are touching the lines. I suggest something like this:
* - headers* An array containing request headers to send as name/value pairs.
* - method
* A string containing the request method. Defaults to 'GET'.
* - body
* A string containing the request body. Defaults to NULL.
* - max_redirects
* An integer representing how many times a redirect may be followed.
* Defaults to 3.
* @return
* An object containing the request, the response code, headers, and body,
* and, if redirected, the redirect status and location.
The rationale behind "header"/"value" vs. "name"/value is described in #327269: drupal_page_cache_header() should compare timestamp. The notation
name/valuerather thanname => valueappears to be a bit more widespread in core (though I personally prefer the name => value convention)."data" is renamed to "body".
"retry" is renamed to "max_redirects" - following a redirect can hardly be described as a retry.
The @return description would be more readable if it was formatted with bullets like the $options array and with mention of the actual property names.
// prefix were stored statically in a file or database variable.+ global $db_prefix;
+ foreach ($options['headers'] as $header => $value) {I suggest renaming $header to $name.
I hope you don't feel I am derailing the issue. I just got the impression that you were trying to do some general clean-up while array-itizing the function. :-)
#4
Those are some great suggestions c960657 and I'll work on improving the documentation. I always hated the $retry parameter and how it did not match it's purpose.
#5
Revised patch for peer review. I still need to finish the descriptions for the @return doc, so I'm still leaving this as code needs work.
#6
Revised patch. I'm not comfortable renaming 'data' to 'body' since we already use $response->data in the return value. Renaming that object parameter is going to cause a lot of backwards compatibility headaches for contrib, so I'd like to get webchick or Dries's opinion on renaming 'data' to 'body'. As of this patch, this is now the current documentation for drupal_http_request:
* @param $url* A string containing a fully qualified URI.
* @param $options
* (optional) An array which can have one or more of following keys:
* - headers
* An array containing request headers to send as name/value pairs.
* - method
* A string containing the request method. Defaults to 'GET'.
* - data
* A string containing the request body. Defaults to NULL.
* - max_redirects
* An integer representing how many times a redirect may be followed.
* Defaults to 3.
* @return
* An object which can have one or more of the following parameters:
* - request
* A string containing the request body that was sent.
* - code
* An integer containting the response status code, or the error code if
* an error occurred.
* - redirect_code
* If redirected, an integer containing the initial response status code.
* - redirect_url
* If redirected, a string containing the redirection location.
* - error
* If an error occurred, the error messsage.
* - headers
* An array containing the response headers as name/value pairs.
* - data
* A string containing the response body that was received.
*/
function drupal_http_request($url, array $options = array()) {
...
#7
Marked #64866: Pluggable architecture for drupal_http_request() postponed until this issue lands in HEAD. It will make that issue so, so much easier and nicer.
#8
+ * An integer containting the response status code, or the error code ifApart from that it looks good :-)
#9
Hah! I guess that's a combination of containing and tainting. Also found a spelling error on "messsage." Revised and spelling-error free.
#10
The last submitted patch failed testing.
#11
Rerolled for HEAD.
#12
The last submitted patch failed testing.
#13
re-rolling around the aggregator module changes.
#14
Committed to CVS HEAD. Thanks! :)
#15
Actually, marking this 'code needs work' until the upgrade documentation has been updated.
#16
Documentation has been updated. See http://drupal.org/node/224333#drupal_http_request_parameters
#17
Automatically closed -- issue fixed for two weeks with no activity.