Since D6, many of our hooks have evolved to add a _alter version allowing after-the-fact altering of information.

One place where this is still missing is hook_xmlrpc().

The typical use case I envision for this is much the same as for hook_menu(), for instance implementing preprocessing features in front of existing xmlrpc web services without any need to alter their code. The implementation seems almost trivial:

Current, in xmlrpc.php

xmlrpc_server(module_invoke_all('xmlrpc'));

Possible change looks like it could be:

$services = module_invoke_all('xmlrpc');
drupal_alter('xmlrpc_alter', $services);
xmlrpc_server($services);
CommentFileSizeAuthor
#19 xmlrpc_656266_19.patch6.86 KBfgm
#17 xmlrpc_656266_17.patch6.73 KBfgm
#14 drupal.xmlrpc-alter.14.patch3.77 KBsun
#13 656266_13.patch928 bytesfgm
#12 xmlrpc_alter.patch737 bytesfgm
#7 xmlrpc_doc.patch1.78 KBfgm
#6 xmlrpc_alter.patch532 bytesfgm
#3 rpc_alter.patch592 bytesfgm
#2 rpc_alter.patch592 bytesfgm
rpc_alter.patch548 bytesfgm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
FileSize
rpc_alter.patch592 bytes

Rerolling against today's head.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Needs work » Needs review
FileSize
rpc_alter.patch592 bytes

Rerolling against today's head.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Committed.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Priority: Normal » Critical
Status: Fixed » Active

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

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Active » Needs review
FileSize
xmlrpc_alter.patch532 bytes

Oops, typo...

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
FileSize
xmlrpc_doc.patch1.78 KB

To compensate for the typo, enhanced patch including API documentation.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

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

Status: Fixed » Closed (fixed)

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

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Version: 7.x-dev » 6.x-dev
Priority: Critical » Normal
Status: Closed (fixed) » Patch (to be ported)

Any hope of seeing this backported ? It's exactly the same change on D6.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs work

Just found a limitation in this approach: default methods from the system package cannot be thus altered, because they are being injected in the call to includes/xmlrpcs.inc#xmlrpc_server(). The drupal_alter() needs to be performed after their injection instead of before as it currently is.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Needs work » Needs review
FileSize
xmlrpc_alter.patch737 bytes

Here is the D6 suggested patch, for a start

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
FileSize
656266_13.patch928 bytes

And here is the D7 patch.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: hook_xml_rpc is missing a matching hook_xml_rpc_alter » Missing hook_xml_rpc_alter()
FileSize
drupal.xmlrpc-alter.14.patch3.77 KB 
+++ 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.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented

Ooops. Sorry I forgot the xmlrpc.php when rolling the patch !

Your comments are better worded, too. RTBC for me, but maybe we want to add a test ?

sfyn’s picture

sfyn CreditAttribution: sfyn commented
Status: Needs review » Reviewed & tested by the community

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

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Needs review » Reviewed & tested by the community
FileSize
xmlrpc_656266_17.patch6.73 KB

@sfyn: thx for the review.

Here is a new version: same code, but adds a simpletest to hide a builtin method. The test would have failed with the patch from #3.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Reviewed & tested by the community » Needs work

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.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Status: Needs work » Needs review
FileSize
xmlrpc_656266_19.patch6.86 KB

Rerolled; thanks for your fast comments.

I kept the mention about builtin methods because this was precisely what failed in the previous implementation. Not sure whether it is useful or not ?

Regarding isset/is_null, I suppose this is for performance reasons (non-function vs function), but maybe there is something else ?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, xmlrpc_656266_19.patch, failed testing.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented

FWIW failure appears to be in a form API change committed this afternoon, not related with this patch.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review

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

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

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

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Please wait for the bot to come back green.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 7.x-dev » 8.x-dev
Category: bug » feature

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.

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Version: 8.x-dev » 7.x-dev
Category: feature » bug

@webchick: this is not a feature request, the hook has been in D7 core since Dries committed it in december, but included a bug which this new version fixes.

Accordingly, resetting to D7/bug. Hope I'm not misinterpreting your intent.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 7.x-dev » 8.x-dev
Category: bug » feature

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.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 8.x-dev » 7.x-dev
Category: feature » bug
fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented
Title: Missing hook_xml_rpc_alter() » hook_xml_rpc_alter() can not alter builtin methods

Adjusting title to match current situation.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: hook_xml_rpc_alter() can not alter builtin methods » hook_xmlrpc_alter() cannot alter built-in methods

Sigh. No comment on the last 10 comments.

Still RTBC.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

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

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Version: 7.x-dev » 8.x-dev

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

fgm’s picture

fgm
Primary language French
Location Paris, France
CreditAttribution: fgm commented

@sun: could you explain which rule applies ? This is a bug to an existing D7 feature. Are we only fixing bugs in D8 now ? And a rather bad one, because it removes most of the interest of the hook: if it is impossible to use HXA to alter/mask the builtin methods, the fact the system.multicall is not altered means any call to any method, even hidden, remains possible just by multicalling it in a single call array.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Version: 8.x-dev » 7.x-dev

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

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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