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.

CommentFileSizeAuthor
#13 drupal_http_337783.patch19.46 KBdrewish
#11 337783-drupal-http-request-arrayitize-D7.patch18.11 KBdave reid
#9 337783-drupal-http-request-arrayitize-D7.patch18.06 KBdave reid
#6 337783-drupal-http-request-arrayitize-D7.patch18.07 KBdave reid
#5 337783-drupal-http-request-arrayitize-D7.patch15.95 KBdave reid
#2 337783-drupal-http-request-arrayitize-D7.patch15.42 KBdave reid

Comments

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

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.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Category: task » feature
Status: Active » Needs review
StatusFileSize
new 337783-drupal-http-request-arrayitize-D7.patch15.42 KB

Arg...wasn't the latest patch. This one is good.

c960657’s picture

c960657 commented
Status: Needs review » Needs work

+ * (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/value rather than name => 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.

   // prefix were stored statically in a file or database variable.
+  global $db_prefix;

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. :-)

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

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.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new 337783-drupal-http-request-arrayitize-D7.patch15.95 KB

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.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review
StatusFileSize
new 337783-drupal-http-request-arrayitize-D7.patch18.07 KB

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()) {
...
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

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.

c960657’s picture

c960657 commented
Status: Needs review » Needs work 
+ *       An integer containting the response status code, or the error code if

Typo (containting).

Apart from that it looks good :-)

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review
StatusFileSize
new 337783-drupal-http-request-arrayitize-D7.patch18.06 KB

Hah! I guess that's a combination of containing and tainting. Also found a spelling error on "messsage." Revised and spelling-error free.

Status: Needs review » Needs work

The last submitted patch failed testing.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review
StatusFileSize
new 337783-drupal-http-request-arrayitize-D7.patch18.11 KB

Rerolled for HEAD.

Status: Needs review » Needs work

The last submitted patch failed testing.

drewish’s picture

drewish commented
Status: Needs work » Needs review
StatusFileSize
new drupal_http_337783.patch19.46 KB

re-rolling around the aggregator module changes.

dries’s picture

dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Thanks! :)

dries’s picture

dries commented
Status: Fixed » Needs work

Actually, marking this 'code needs work' until the upgrade documentation has been updated.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Fixed

Documentation has been updated. See http://drupal.org/node/224333#drupal_http_request_parameters

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.