Cannot pass drupal_http_request options into xmlrpc() function

I've only done a visual inspection, but this is a welcome change if only for the timeout setting of drupal_http_request that can be passed through.

Method, the content-type header and data are required by the XMLRPC "specification", so it makes sense to not allow changing those settings.

Obviously an API change but I think it is worth it. Ideally, this would be re-factored a bit in Drupal 8. Would love to get other people's take on this.

Tough. I'd agree that being able to pass options to dhr() is needed (potentially having a requirement for that in contrib). However, this is quite an API change...

I wonder whether we could support backwards compatibility until D8...

Old:

$singlecall = xmlrpc($url, 'methodName', $arg1, $arg2, $arg3);
$multicall = xmlrpc($url, array(0 => array('firstMethod', $arg1, $arg2), 1 => array('secondMethod', $arg1, $arg2)));

New:

$singlecall = xmlrpc($url, array(0 => array('methodName', $arg1, $arg2, $arg3)));
$multicall = xmlrpc($url, array(0 => array(0 => array('firstMethod', $arg1, $arg2), 1 => array('secondMethod', $arg1, $arg2))));

Hm. So while the old $singlecall syntax could be easily supported, the new $singlecall syntax clashes with the old $multicall, they are identical.

However, even only ensuring BC for the old $singlecall syntax would be beneficial at this point in time, so we'd only need to announce an API change for multicalls (which are used rarely), i.e.

  $old_args = func_get_args();
  if (isset($old_args[1]) && is_string($old_args[1])) {
    array_shift($old_args);
    $args = $old_args;
  }
  ...

+++ includes/common.inc
@@ -6751,12 +6751,16 @@ function entity_form_submit_build_entity($entity_type, $entity, $form, &$form_st
- * @param ...
+ * @param $args
+ *   An array of arguments.
  *   For one request:
  *     The method name followed by a variable number of arguments to the method.
  *   For multiple requests (system.multicall):
  *     An array of call arrays. Each call array follows the pattern of the single
  *     request: method name followed by the arguments to the method.

+++ includes/xmlrpc.inc
@@ -530,12 +530,15 @@ function xmlrpc_base64_get_xml($xmlrpc_base64) {
- * @param ...
+ * @param $args
+ *   Array of arguments.
  *   - For one request: The method name followed by a variable number of
  *     arguments to the method.
  *   - For multiple requests (system.multicall): An array of call arrays. Each
  *     call array follows the pattern of the single request: method name
  *     followed by the arguments to the method.

I tried hard, but failed to understand how to pass a single method with or without arguments, and how to do a multicall. It was relatively clear with the previous variable amount of arguments.

Most probably, even after improving the param description for $args, we want to add example code to the PHPDoc of xmlrpc().

+++ includes/common.inc
+ * @param $options
+ *   Optional array of options for drupal_http_request().

@@ -6766,10 +6770,9 @@ function entity_form_submit_build_entity($entity_type, $entity, $form, &$form_st
+function xmlrpc($url, $args, $options = array()) {

+++ includes/xmlrpc.inc
@@ -530,12 +530,15 @@ function xmlrpc_base64_get_xml($xmlrpc_base64) {
+ * @param $options
+ *   Array of options for drupal_http_request().

@@ -544,9 +547,7 @@ function xmlrpc_base64_get_xml($xmlrpc_base64) {
+function _xmlrpc($url, $args, $options) {

1) Description for optional arguments should be prefixed with "(optional)". See http://drupal.org/node/1354 for detailed example code.

2) Technically, $options are also optional for _xmlrpc(). I'd recommend to keep both function signatures identically.

Powered by Dreditor.

Could we change the signature of $url?

if (is_array($url)) {
  $options = $url;
  $url = $options['url'];
  unset($options['url']);
}
else {
  $options = array();
}

... after all $options are for the transport, so it makes sense to have them close to (or in that case: inside) $url.

Stuffing everything in one call is never going to be clean, nor easy. In Drupal 8, I think we want to make this object oriented.

Pseudo-code:

  $xmlRpcClient = new XmlRpcClient($url);
  $xmlRpcClient->execute("method1", $params1);
  $xmlRpcClient->setTimeout(10);
  $xmlRpcClient->execute("method2", $params2);

I'm not sure it make sense to keep Drupal 8 in mind.

Somehow, the proposed new syntax looks wonky. Stuffing method name + arguments into a list was really only valid and "useful" for the old/current function signature.

When changing the syntax, the following would be more appropriate:

$singlecall = xmlrpc($url, array(
  'methodName' => array($arg1, $arg2, $arg3),
));
$multicall = xmlrpc($url, array(
  'firstMethod' => array($arg1, $arg2),
  'secondMethod' => array($arg1, $arg2)),
));

And using this syntax, we can retain full BC.

Just incorporated my review issues from #6 for now.

Trying to burn #12 now.

Burned it.

well, we can surely drop the BC code (though I'd disagree, it's simply too late for D7), but I'd highly prefer the $args structure as contained in #14.

The entire invocation of xmlrpc() is much more clean and clear with that.

EDIT: In case it's not immediately clear, #12 contained usage examples for the new syntax implemented in #14.

My point is that if we change the arguments, then we should use sane arguments. Compare:

$singlecall = xmlrpc($url, array(
  array('methodName', $arg1, $arg2, $arg3),
));
$multicall = xmlrpc($url, array(
  array(
    array('firstMethod', $arg1, $arg2),
    array('secondMethod', $arg1, $arg2),
  ),
));

$singlecall = xmlrpc($url, array(
  'methodName' => array($arg1, $arg2, $arg3),
));
$multicall = xmlrpc($url, array(
  'firstMethod' => array($arg1, $arg2),
  'secondMethod' => array($arg1, $arg2),
));

One of both makes sense.

It just failed because of a stupid variable name clash.

Still contains BC code. Whether we should or can break BC here should be decided by Dries/webchick. IMHO, we should not (D7CX frustration), if we don't have to, and we don't have to.

Remember that certain people started to build pretty large sites on D7 already. You can count on me on break ya APIs at this stage, but only if we really have to.

Personally, I think we should go with option 2 from #20.

@Dries: #22 is option 2 from #20. Question is whether to preserve backwards compatibility or not (latest patch in #22 does).

I think we can still break backwards compatibility -- we've done worse things. Once we're in beta, it will be an absolute "no go".

Removed. Also clarified usage in PHPDoc.

Alright. Committed to CVS HEAD. Thanks.

We need to document that in the upgrade instructions. Let's mark this fixed after.

The good news is that the new syntax can be backported, therefore allowing custom headers and a custom retry value in D6, too.

Tiny mistake for system.multicall in that BC code.

-function xmlrpc($url) {
+function xmlrpc($url, $args, $options = array()) {

I think that $args should be optional to be backported

@andypost: $args neither is nor was optional.

@sun, I mean that signature should be

-function xmlrpc($url) {
+function xmlrpc($url, $args = array(), $options = array()) {

Specially for d6 else be get a api incomputability and possible errro messages (php warnings or notices)

@andypost: See #34. $args has never been optional. D6 expects either a method name string or an array as second function argument. Not passing anything triggers an error in _xmlrpc().

A D6 module that depends on the new signature needs to check the Drupal version. Should this be mentioned in the doc block?

@Eric_A, No need because implementation is backward compatible... so I see no reason against ...

RTBC++

That's why I posted the usage examples:

D6:

$singlecall = xmlrpc($url, 'methodName', $arg1, $arg2, $arg3);
$multicall = xmlrpc($url, array(
  array('firstMethod', $arg1, $arg2),
  array('secondMethod', $arg1, $arg2),
));

D7:

$singlecall = xmlrpc($url, array(
  'methodName' => array($arg1, $arg2, $arg3),
));
$multicall = xmlrpc($url, array(
  'firstMethod' => array($arg1, $arg2),
  'secondMethod' => array($arg1, $arg2),
));

With the new argument, 1) $args is never a string and 2) $args[0][0] is never set, it could only be a string key, $args['firstMethod'][0].

Also having options is needed if you want to pass in additional headers (e.g. for web services that require authentication headers) for the http request.

If a D6 module depends on passing in additonal headers, it will depend on this new implementation. If the module doesn't require the correct version of Drupal, it will not function. Why should it not be needed to document this important information?

Pretty major D7 API breakage on this one (fgm discovered on XMLRPC Example in Examples project). And it never went to "fixed" so I never saw it - in those kind of cases let me know so I can announce.

Issue summary, please?

@rfay could you provide a link to issue?

@rfay could you provide a link to issue?

@andypost, #894972: XML-RPC test no longer passes, already fixed. But if it breaks xmlrpc_example, there is lots of contrib breakage.

I rerolled this with the requested comments about hook_requirements(). I would really love to get this committed, it would make our lives a lot easier in Services where we could use it for authentication purposes. I did some testing setting headers and it worked great.

New patch to remove trailing spaces.

@heyrocker. Do you happen to have the time to upload an interdiff from the patch in #30 to help me and other reviewers?

Confirmed that #49 is sun's patch from #30 with 4 lines added to xmlrpc() doc block. Patch in #50 is the same, but with a white space fix that was not in #30 and irrelevant to this patch.
The reroll was for these 4 lines. The one relevant commit to DRUPAL-6 since #30 (affecting common.inc) does not affect this patch.

Back to RTBC as per #39.

#50: 881536_xmlrpc_dhr_options.patch queued for re-testing.

Setting to RTBC should not trigger a test bot, right?
Well, even if the patch name does not end with -d6 or _D6, the current behaviour might be related to #961172: All D6 Core patches are failing.

Sorry, it can't be RTBC if it can't pass the testbot.

@rfay. This a D6 patch... Initially backported by sun. On November 3rd the status changed because of infrastructure hickups, it is presumed.

This is #50 with the name changed to -d6, no other change. It will factor the testbot out of the equation, if I'm not mistaken.

Sorry for my misunderstanding.

Uhm, I'm not sure this way of introducing a whole new way to call xmlrpc() is a good idea even if we keep the old ways of calling it intact with special hacks. Eg the new docs will not keep explanation for the old syntax, which I guess most existing Drupal code would still use. So if you'd like to look up why its invoked that way, you'll not even have docs anymore. I'm not convinced.