hook_xmlrpc_alter() cannot alter built-in methods

Committed.

I think what we want here is

GOOD: drupal_alter('xmlrpc', $services);
BAD: drupal_alter('xmlrpc_alter', $services);

If we leave as is, an implementation is too verbose. for example: poll_xmlprc_alter_alter()

Good catch, Moshe, and thanks for the extra documentation with the fix, fgm. Committed to CVS HEAD.

+++ includes/xmlrpcs.inc	12 Aug 2010 10:24:09 -0000
@@ -45,7 +45,12 @@ function xmlrpc_server($callbacks) {
+  // @link http://drupal.org/node/656266 @endlink

We only link to drupal.org issues in very complex cases.

+++ includes/xmlrpcs.inc	12 Aug 2010 10:24:09 -0000
@@ -45,7 +45,12 @@ function xmlrpc_server($callbacks) {
+  $callbacks = array_merge($defaults, (array)$callbacks);
+  drupal_alter('xmlrpc', $callbacks);

This means that we can revert the changes to xmlrpc.php.

Powered by Dreditor.

I've reviewed the changes and tested the patch. I was able to write a module using the hook, add a method to the list of xmlrpc methods, plus retrieve it from another site.

+1 for the patch in #14

Thanks!

+++ modules/simpletest/tests/xmlrpc.test	23 Aug 2010 13:39:53 -0000
@@ -211,4 +211,25 @@ class XMLRPCMessagesTestCase extends Dru
+  /**
+   * Ensure that hook_xmlrpc_alter() implementations can hide methods, even
+   * builtin ones like system.methodSignature().
+   */

phpDoc summaries should be on one line. See http://drupal.org/node/1354

We can shorten this to: Ensure that hook_xmlrpc_alter() can hide methods.

+++ modules/simpletest/tests/xmlrpc.test	23 Aug 2010 13:39:53 -0000
@@ -211,4 +211,25 @@ class XMLRPCMessagesTestCase extends Dru
+    variable_set('xmlrpc_hide_method_signature', FALSE);

+++ modules/simpletest/tests/xmlrpc_test.module	23 Aug 2010 13:39:53 -0000
@@ -64,6 +64,31 @@ function xmlrpc_test_xmlrpc() {
+function xmlrpc_test_xmlrpc_alter(&$services) {
+  if (variable_get('xmlrpc_hide_method_signature', FALSE)) {

The variable should at least be in the namespace of the xmlrpc_test module, i.e., starting with "xmlrpc_test_". Most often, we just re-use the hook implementation name for the variable in such situations, i.e., "xmlrpc_test_xmlrpc_alter" as variable name.

+++ modules/simpletest/tests/xmlrpc.test	23 Aug 2010 13:39:53 -0000
@@ -211,4 +211,25 @@ class XMLRPCMessagesTestCase extends Dru
+    // Invoke XML-RPC call to get list of methods.

This comment, in the middle of nothing, make no sense for me. It does not explain anything that wouldn't be visible from the code itself. I guess you rather meant to put one inline comment above each section/chapter:

// Ensure xmlrpc_test_xmlrpc_alter() is disabled and retrieve regular list of methods.
...

// Enable the alter hook and retrieve the list of methods again.
...

+++ modules/simpletest/tests/xmlrpc_test.module	23 Aug 2010 13:39:53 -0000
@@ -64,6 +64,31 @@ function xmlrpc_test_xmlrpc() {
+ * Implement hook_xmlrpc_alter().

s/Implement/Implements/

+++ modules/simpletest/tests/xmlrpc_test.module	23 Aug 2010 13:39:53 -0000
@@ -64,6 +64,31 @@ function xmlrpc_test_xmlrpc() {
+ *
+ * @return void

We do not document return values for hook implementations. See Doxygen coding standards I linked to above.

+++ modules/simpletest/tests/xmlrpc_test.module	23 Aug 2010 13:39:53 -0000
@@ -64,6 +64,31 @@ function xmlrpc_test_xmlrpc() {
+    if (!is_null($remove)) {

Should be isset(). Never use is_null().

Powered by Dreditor.

#19: xmlrpc_656266_19.patch queued for re-testing.

#19: xmlrpc_656266_19.patch queued for re-testing.

Please wait for the bot to come back green.

New hook = feature request.

Since this doesn't break BC, it's possible this is one of those things we could back-port from D8 to D7 later on.

Ah, ok. I saw the +drupal_alter() but missed the -drupal_alter() up above.

We should probably adjust the issue title so no one else makes that mistake.

Sigh. No comment on the last 10 comments.

Still RTBC.

#19: xmlrpc_656266_19.patch queued for re-testing.

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

Only feature requests and tasks can be assigned to D8 for now, because the Drupal 8 tree is not opened yet.

Committed to CVS HEAD. Thanks.