Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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:
function drupal_http_request($url, array $options = array()) {
$options += array(
'headers' => array(),
'method' => 'GET',
'data' => NULL,
'retry' => 3,
);
Providing patch shortly.
Comment | File | Size | Author |
---|---|---|---|
#13 | drupal_http_337783.patch | 19.46 KB | drewish |
#11 | 337783-drupal-http-request-arrayitize-D7.patch | 18.11 KB | Dave Reid |
#9 | 337783-drupal-http-request-arrayitize-D7.patch | 18.06 KB | Dave Reid |
#6 | 337783-drupal-http-request-arrayitize-D7.patch | 18.07 KB | Dave Reid |
#5 | 337783-drupal-http-request-arrayitize-D7.patch | 15.95 KB | Dave Reid |
Comments
Comment #1
Dave ReidInitial 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.
Comment #2
Dave ReidArg...wasn't the latest patch. This one is good.
Comment #3
c960657 CreditAttribution: c960657 commented+ * (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:
The rationale behind "header"/"value" vs. "name"/value is described in #327269: drupal_page_cache_header() should compare timestamp. The notation
name/value
rather thanname => value
appears 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.
Aren't global declarations usually put at the first line of the method?
+ 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. :-)
Comment #4
Dave ReidThose 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.
Comment #5
Dave ReidRevised 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.
Comment #6
Dave ReidRevised 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:
Comment #7
Dave ReidMarked #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.
Comment #8
c960657 CreditAttribution: c960657 commentedTypo (containting).
Apart from that it looks good :-)
Comment #9
Dave ReidHah! I guess that's a combination of containing and tainting. Also found a spelling error on "messsage." Revised and spelling-error free.
Comment #11
Dave ReidRerolled for HEAD.
Comment #13
drewish CreditAttribution: drewish commentedre-rolling around the aggregator module changes.
Comment #14
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks! :)
Comment #15
Dries CreditAttribution: Dries commentedActually, marking this 'code needs work' until the upgrade documentation has been updated.
Comment #16
Dave ReidDocumentation has been updated. See http://drupal.org/node/224333#drupal_http_request_parameters