Depending on the nature of the xml-rpc request, it might be important to set the options for drupal_http_request() such as the timeout or context (see #156582: drupal_http_request() should support timeout setting).

Fixing this should be really trivial.

Files: 
CommentFileSizeAuthor
#60 881536_xmlrpc_dhr_options_0-d6.patch4.56 KBrfay
#50 881536_xmlrpc_dhr_options.patch4.56 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 881536_xmlrpc_dhr_options_0.patch.
[ View ]
#49 881536_xmlrpc_dhr_options.patch4.57 KBheyrocker
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 881536_xmlrpc_dhr_options.patch.
[ View ]
#30 drupal-DRUPAL-6.xmlrpc-options.30.patch4.28 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-DRUPAL-6.xmlrpc-options.30.patch.
[ View ]
#29 drupal-DRUPAL-6.xmlrpc-options.29.patch4.27 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-DRUPAL-6.xmlrpc-options.29.patch.
[ View ]
#26 drupal.xmlrpc-options.26.patch12.42 KBsun
PASSED: [[SimpleTest]]: [MySQL] 22,850 pass(es).
[ View ]
#22 drupal.xmlrpc-options.21.patch12.81 KBsun
PASSED: [[SimpleTest]]: [MySQL] 22,868 pass(es).
[ View ]
#17 881536-drupal.xmlrpc-options.13.patch9.42 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 22,854 pass(es).
[ View ]
#14 drupal.xmlrpc-options.14.patch11.16 KBsun
FAILED: [[SimpleTest]]: [MySQL] 22,847 pass(es), 6 fail(s), and 0 exception(es).
[ View ]
#13 drupal.xmlrpc-options.13.patch9.71 KBsun
PASSED: [[SimpleTest]]: [MySQL] 22,852 pass(es).
[ View ]
#4 881536-request-options-4.patch8.31 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 22,825 pass(es).
[ View ]
#1 881536-request-options-1.patch3.13 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 22,820 pass(es), 4 fail(s), and 13 exception(es).
[ View ]

Comments

pwolanin’s picture

Status:Active» Needs review
StatusFileSize
new3.13 KB
FAILED: [[SimpleTest]]: [MySQL] 22,820 pass(es), 4 fail(s), and 13 exception(es).
[ View ]

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.

Heine’s picture

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.

pwolanin’s picture

hmm, looks like this will fail - need to convert the tests.

pwolanin’s picture

StatusFileSize
new8.31 KB
PASSED: [[SimpleTest]]: [MySQL] 22,825 pass(es).
[ View ]

with test fixes

Dries’s picture

Issue tags:+API change

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.

sun’s picture

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:

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

New:

<?php
$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.

<?php
  $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.

Damien Tournoud’s picture

Could we change the signature of $url?

<?php
if (is_array($url)) {
 
$options = $url;
 
$url = $options['url'];
  unset(
$options['url']);
}
else {
 
$options = array();
}
?>
Damien Tournoud’s picture

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

Dries’s picture

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:

<?php
  $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.

pwolanin’s picture

@sun - the code changes needed for this API change are pretty trivial - look at the tests. You just wraps the arguments in an array.

given that most modules that use it only make RPC calls in one or two places (including one I maintain), I really, really don't think it's worth making this more complex than it needs to be.

pwolanin’s picture

@Dries - if this is going to be OO in Drupal 8, then maybe we will just adopt a library?

sun’s picture

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:

<?php
$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.

sun’s picture

StatusFileSize
new9.71 KB
PASSED: [[SimpleTest]]: [MySQL] 22,852 pass(es).
[ View ]

Just incorporated my review issues from #6 for now.

Trying to burn #12 now.

sun’s picture

StatusFileSize
new11.16 KB
FAILED: [[SimpleTest]]: [MySQL] 22,847 pass(es), 6 fail(s), and 0 exception(es).
[ View ]

Burned it.

Status:Needs review» Needs work

The last submitted patch, drupal.xmlrpc-options.14.patch, failed testing.

pwolanin’s picture

No, I really, really don't think we should care about BC for such a rarely used function. That flies against our whole philosophy.

pwolanin’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new9.42 KB
PASSED: [[SimpleTest]]: [MySQL] 22,854 pass(es).
[ View ]

Let's keep this simple - I'm reposting the same changes as in #13 - I think this is RTBC. The upgrade from D6 can be done essentially automatically via regex replace.

sun’s picture

Status:Reviewed & tested by the community» Needs work

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.

pwolanin’s picture

Status:Needs work» Reviewed & tested by the community

The patch in #17 works - the one in #14 I am getting weird xmlrpc errors:

Fatal error: Unsupported operand types in /Users/Shared/www/drupal-7/modules/simpletest/tests/xmlrpc_test.module on line 46

I really don't think it's worth more effort to get a syntax that's just a little cleaner. I still assert #17 is rtbc.

sun’s picture

Status:Reviewed & tested by the community» Needs work

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

<?php
$singlecall
= xmlrpc($url, array(
  array(
'methodName', $arg1, $arg2, $arg3),
));
$multicall = xmlrpc($url, array(
  array(
    array(
'firstMethod', $arg1, $arg2),
    array(
'secondMethod', $arg1, $arg2),
  ),
));
?>
<?php
$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.

pwolanin’s picture

@sun - I agree the second form is "nicer", but they both make sense (i.e. conform to a reasonable logic). But apparently it does not work, and I don't think it's worth the effort at this point to debug to achieve "nicer" in order to fix the big: the bug is that we cannot pass options to the http request.

The time and effort to get the "nicer" syntax working is far better spent fixing a critical bug so we can get D7 out. Please!

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new12.81 KB
PASSED: [[SimpleTest]]: [MySQL] 22,868 pass(es).
[ View ]

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.

Dries’s picture

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

sun’s picture

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

Dries’s picture

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".

sun’s picture

StatusFileSize
new12.42 KB
PASSED: [[SimpleTest]]: [MySQL] 22,850 pass(es).
[ View ]

Removed. Also clarified usage in PHPDoc.

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community

Reviewed the patch and discussed with Sun in chat. Looks good to me.

Dries’s picture

Status:Reviewed & tested by the community» Needs work

Alright. Committed to CVS HEAD. Thanks.

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

sun’s picture

Version:7.x-dev» 6.x-dev
Status:Needs work» Needs review
Issue tags:+Needs Documentation
StatusFileSize
new4.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-DRUPAL-6.xmlrpc-options.29.patch.
[ View ]

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

sun’s picture

StatusFileSize
new4.28 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-DRUPAL-6.xmlrpc-options.30.patch.
[ View ]

Tiny mistake for system.multicall in that BC code.

pwolanin’s picture

Version:6.x-dev» 7.x-dev
Status:Needs review» Fixed
Issue tags:-Needs Documentation
pwolanin’s picture

Version:7.x-dev» 6.x-dev
Status:Fixed» Needs review

oops - cross post - see above for D7 API docs

andypost’s picture

Issue tags:+Needs Documentation
-function xmlrpc($url) {
+function xmlrpc($url, $args, $options = array()) {

I think that $args should be optional to be backported

sun’s picture

@andypost: $args neither is nor was optional.

pwolanin’s picture

Issue tags:-Needs Documentation

docs are already added

andypost’s picture

@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)

sun’s picture

@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().

Eric_A’s picture

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

andypost’s picture

Status:Needs review» Reviewed & tested by the community

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

RTBC++

pwolanin’s picture

Is this a robust check?

elseif (isset($args[0][0])) {
sun’s picture

That's why I posted the usage examples:

D6:

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

D7:

<?php
$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].

Eric_A’s picture


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?

pwolanin’s picture

Status:Reviewed & tested by the community» Needs work

I think the point of mentioning a hook_requirements check for VERSION is useful to add to the doxygen.

rfay’s picture

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?

andypost’s picture

@rfay could you provide a link to issue?

andypost’s picture

@rfay could you provide a link to issue?

rfay’s picture

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

pwolanin’s picture

The D7 api change is documented at http://drupal.org/update/modules/6/7#xmlrpc

heyrocker’s picture

Status:Needs work» Needs review
StatusFileSize
new4.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 881536_xmlrpc_dhr_options.patch.
[ View ]

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.

heyrocker’s picture

StatusFileSize
new4.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 881536_xmlrpc_dhr_options_0.patch.
[ View ]

New patch to remove trailing spaces.

Eric_A’s picture

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

Eric_A’s picture

Status:Needs review» Reviewed & tested by the community

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.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 881536_xmlrpc_dhr_options.patch, failed testing.

Eric_A’s picture

Status:Needs work» Reviewed & tested by the community
rfay’s picture

Issue tags:-API change

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

Status:Reviewed & tested by the community» Needs work
Issue tags:+API change

The last submitted patch, 881536_xmlrpc_dhr_options.patch, failed testing.

Eric_A’s picture

Status:Needs work» Reviewed & tested by the community

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.

rfay’s picture

Status:Reviewed & tested by the community» Needs work

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

Eric_A’s picture

Status:Needs work» Reviewed & tested by the community

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

rfay’s picture

StatusFileSize
new4.56 KB

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.

Gábor Hojtsy’s picture

Version:6.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Fixed

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.

Status:Fixed» Closed (fixed)
Issue tags:-API change

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