Doc cleanup for xmlrpcs.inc and add missing doc headers [#855420]

Comment	File	Size	Author
#22	855420-22.patch	5.81 KB	jhodgdon

#17	855420-17.patch	5.61 KB	jhodgdon

#15	855420-15.patch	5.5 KB	jhodgdon

#12	855420-12.patch	5.52 KB	jhodgdon

#11	855420-11.patch	5.23 KB	jhodgdon

#8	855420-8.patch	5.22 KB	jhodgdon

#6	newline_xmlrpcs_inc_v2.patch	2.49 KB	aspilicious

	newline_xmlrpcs_inc.patch	565 bytes	aspilicious

Comment #1

she/her

English

CreditAttribution: jhodgdon commented 15 July 2010 at 17:39

The text just after that needs fixing:

 * @return
  *   An array of types representing the method signature of the
  *   function that the methodname maps to. The methodSignature of

Should be using $methodname, and probably SomeObjectTypeName::methodSignature ?

Log in or register to post comments

Comment #2

aspilicious CreditAttribution: aspilicious commented 15 July 2010 at 19:18

Title:	Fix newlines in xmlrpcs.inc	» Fix newlines in xmlrpcs.inc and add missing doc headers
Priority:	Minor	» Normal
Status:	Needs review	» Needs work

this is the function

/**
 * XML-RPC method system.methodSignature maps to this function.
 *
 * @param $methodname
 *   Name of method for which we return a method signature.
 *
 * @return
 *   An array of types representing the method signature of the
 *   function that the methodname maps to. The methodSignature of
 *   this function is 'array', 'string' because it takes an array
 *   and returns a string.
 */
function xmlrpc_server_method_signature($methodname) {
  $xmlrpc_server = xmlrpc_server_get();
  if (!isset($xmlrpc_server->callbacks[$methodname])) {
    return xmlrpc_error(-32601, t('Server error. Requested method @methodname not specified.', array("@methodname" => $methodname)));
  }
  if (!is_array($xmlrpc_server->signatures[$methodname])) {
    return xmlrpc_error(-32601, t('Server error. Requested method @methodname signature not specified.', array("@methodname" => $methodname)));
  }
  // We array of types
  $return = array();
  foreach ($xmlrpc_server->signatures[$methodname] as $type) {
    $return[] = $type;
  }
  return $return;
}

maybe you can make a better sentence ;)

There are also some undocumented function headers, so setting this to needs work.

Log in or register to post comments

Comment #3

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 15 July 2010 at 20:26

How about:

Returns the method signature of a function (XML-RPC method system.methodSignature).

A method signature is an array of the input and output types of a method. For instance, the method signature of this function is array('array', 'string'), because it takes an array and returns a string.

@param string $methodname
Name of method to return a method signature for.

@return
An array of types representing the method signature of the function that $methodname maps to.

Log in or register to post comments

Comment #4

aspilicious CreditAttribution: aspilicious commented 15 July 2010 at 20:43

First sentence exceeds 80 characters

Log in or register to post comments

Comment #5

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 15 July 2010 at 21:00

Dang.

How about

Returns the method signature of a function.

This is the function mapped to the XML-RPC method system.methodSignature.

A method signature is an array of the input and output types of a method. For instance, the method signature of this function is array('array', 'string'), because it takes an array and returns a string.

Log in or register to post comments

Comment #6

aspilicious CreditAttribution: aspilicious commented 15 July 2010 at 21:26

File	Size
newline_xmlrpcs_inc_v2.patch	2.49 KB

1) fixed some doc errors

2) following functions still need a header

function xmlrpc_server_output($xml) {
  $xml = '<?xml version="1.0"?>' . "\n" . $xml;
  drupal_add_http_header('Content-Length', strlen($xml));
  drupal_add_http_header('Content-Type', 'text/xml');
  echo $xml;
  drupal_exit();
}

and

function xmlrpc_server_multicall($methodcalls) {
  // See http://www.xmlrpc.com/discuss/msgReader$1208
  $return = array();
  $xmlrpc_server = xmlrpc_server_get();
  foreach ($methodcalls as $call) {
    $ok = TRUE;
    if (!isset($call['methodName']) || !isset($call['params'])) {
      $result = xmlrpc_error(3, t('Invalid syntax for system.multicall.'));
      $ok = FALSE;
    }
    $method = $call['methodName'];
    $params = $call['params'];
    if ($method == 'system.multicall') {
      $result = xmlrpc_error(-32600, t('Recursive calls to system.multicall are forbidden.'));
    }
    elseif ($ok) {
      $result = xmlrpc_server_call($xmlrpc_server, $method, $params);
    }
    if (is_object($result) && !empty($result->is_error)) {
      $return[] = array(
        'faultCode' => $result->code,
        'faultString' => $result->message
      );
    }
    else {
      $return[] = array($result);
    }
  }
  return $return;
}

Log in or register to post comments

Comment #7

aspilicious CreditAttribution: aspilicious commented 15 July 2010 at 21:26

Status:

Needs work

» Needs review

Log in or register to post comments

Comment #8

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 15 July 2010 at 23:06

File	Size
855420-8.patch	5.22 KB

How about this? Fixed a few more errors, and adds function headers for those functions.

Log in or register to post comments

Comment #9

aspilicious CreditAttribution: aspilicious commented 16 July 2010 at 08:01

Title:	Fix newlines in xmlrpcs.inc and add missing doc headers	» Doc cleanup for xmlrpcs.inc and add missing doc headers
Status:	Needs review	» Needs work

+++ includes/xmlrpcs.inc	15 Jul 2010 23:04:58 -0000
@@ -218,6 +239,19 @@
+ *   An array of XML-RPC requests to make. Each request is an array with the following elements:

Needs wrapping, besides of that this looks good.

Summary:
- This is a total doc cleanup for xmlrpcs
- Adds missing headers
- Don't affect current rtbc critical patches

46 critical left. Go review some!

Log in or register to post comments

Comment #10

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 16 July 2010 at 12:49

Yeah. I was using a different computer, just upgraded, and the emacs window was too wide (seems to have forgotten my settings, ugh). So it needs a reroll...

Log in or register to post comments

Comment #11

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 16 July 2010 at 13:01

Status:

Needs work

» Needs review

File	Size
855420-11.patch	5.23 KB

Here goes...

Log in or register to post comments

Comment #12

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 20 July 2010 at 18:19

File	Size
855420-12.patch	5.52 KB

Here's a better version.

Log in or register to post comments

Comment #13

sun

German

Karlsruhe

CreditAttribution: sun commented 29 July 2010 at 01:05

#12: 855420-12.patch queued for re-testing.

Log in or register to post comments

Comment #14

29 July 2010 at 01:20

Status:

Needs review

» Needs work

The last submitted patch, 855420-12.patch, failed testing.

Log in or register to post comments

Comment #15

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 29 July 2010 at 15:16

Status:

Needs work

» Needs review

File	Size
855420-15.patch	5.5 KB

Here's a reroll.

Log in or register to post comments

Comment #16

sun

German

Karlsruhe

CreditAttribution: sun commented 29 July 2010 at 15:34

Status:

Needs review

» Needs work

Thanks!

+++ includes/xmlrpcs.inc	29 Jul 2010 15:15:55 -0000
@@ -7,9 +7,9 @@
- * The main entry point for XML-RPC requests.
+ * Performs XML-RPC requests.
...
 function xmlrpc_server($callbacks) {

xmlrpc_server() does not really "perform requests", it rather accepts + processes... how about:

"Invokes XML-RPC methods on this server."

(or similar)

+++ includes/xmlrpcs.inc	29 Jul 2010 15:15:55 -0000
@@ -99,12 +99,13 @@
+ * @param string $message
+ *   The description of the error. Used only if an integer error code was
+ *   passed in.
...
 function xmlrpc_server_error($error, $message = FALSE) {

Missing (optional) prefix.

+++ includes/xmlrpcs.inc	29 Jul 2010 15:15:55 -0000
@@ -122,10 +129,15 @@
+ * Stores a copy of an XML-RPC request temporarily.

@@ -135,21 +147,31 @@
+ * Dispatches an XML-RPC request and any parameters to the appropriate handler.

s/an/a/, no?

+++ includes/xmlrpcs.inc	29 Jul 2010 15:15:55 -0000
@@ -135,21 +147,31 @@
+ * Retrieves the latest stored XML-RCP request.

Typo in "RCP"

+++ includes/xmlrpcs.inc	29 Jul 2010 15:15:55 -0000
@@ -135,21 +147,31 @@
  * @param $xmlrpc_server
...
+ *   Object containing information about this XML-RPC server, the methods it
+ *   provides, their signatures, etc.
  * @param $methodname
...
  * @param $args
  *   Array containing any parameters that were sent along with the request.
...
 function xmlrpc_server_call($xmlrpc_server, $methodname, $args) {

Why no @param data types here?

Powered by Dreditor.

Log in or register to post comments

Comment #17

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 29 July 2010 at 15:42

File	Size
855420-17.patch	5.61 KB

Not sure what you meant here:

+++ includes/xmlrpcs.inc 29 Jul 2010 15:15:55 -0000
@@ -99,12 +99,13 @@
+ * @param string $message
+ *   The description of the error. Used only if an integer error code was
+ *   passed in.
...
function xmlrpc_server_error($error, $message = FALSE) {

Missing (optional) prefix.

Actually I just noticed that $message could be either a string or FALSE, so I took out the "string" in this line.

Regarding a vs. an for XMLPRC, I thought it was pronounced "ex em el are pee cee", so when I say it, I would say "an XML-RPC request".

I also fixed the RPC/RCP typo, and added a few more @param types. I think we have to omit the type when it could be more than one type?

Anyway, here's another go at the patch... thanks for your review!

Log in or register to post comments

Comment #18

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 29 July 2010 at 15:42

Status:

Needs work

» Needs review

Log in or register to post comments

Comment #19

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 29 July 2010 at 15:43

Assigned:

aspilicious

» Unassigned

Log in or register to post comments

Comment #20

sun

German

Karlsruhe

CreditAttribution: sun commented 29 July 2010 at 16:09

oh - I'm no native English speaker - is usage of "an" instead of "a" coupled to pronunciation? (I've no idea, thought it would be only used in conjunction with a following "e" or "o" - in written text.)

Function arguments that can be omitted are documented with a "(optional)" prefix in the @param description. Since $message defaults to FALSE, it's optional and can be omitted, so I think the description should have an "(optional)" prefix.

Yes, no data types if an argument accepts mixed types. (ok, I think that php.net actually documents "mixed" in such cases, but I've no idea what that is good for)

Log in or register to post comments

Comment #21

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 29 July 2010 at 20:18

Yes - a/an is based on pronunciation. And "an" goes before any vowel sound, not just e or o.

I think in our doc standards discussion about types on @param/@return, we decided not to do "mixed".

And you are right, our standard for optional args looks like this:

 * @param $third
 *   (optional) Boolean whether to do Third. Defaults to FALSE.
 *   Only optional parameters are explicitly stated as such. The description
 *   should clarify the default value if omitted.

So that could be added to the patch above. I will get to it eventually, unless anyone else beats me to it. I think that's the only thing that needs fixing in it -- to find the optional parameters and put (optional) at the beginning of their descriptions?