Improve the XML-RPC example to include multicall example

@fgm, thanks for your work on this! Please fix the test in this issue though (HEAD status seems OK: http://qa.drupal.org/pifr/test/26934) because we can't commit until your patch is green.

A core commit seems to have broken form example, so I'm trying to solve that. Thanks for the good work!

HEAD is fixed now. I'll get this reviewed and committed in the next week. Thanks so much for your work on this.

Unfortunately, the spelling is in fact "subtract", so the changes here are wrong.

I'm also opposed to taking a very simple example and making it require an alter. Is there a way this can be done while *adding on* the alter? If people can understand it without understanding the alter, then I'm OK with it. But if they have to understand the basic approach *and* al alter, I'm not.

Thanks very much for the work and for sorting out the core API change.

+++ xmlrpc_example/xmlrpc_example.module	27 Aug 2010 14:05:53 -0000
@@ -85,6 +88,28 @@ function _xmlrpc_example_subtract($num1,
+    'xmlrpc_example.substract',
+    '_xmlrpc_example_substract',

substract misspelled

+++ xmlrpc_example/xmlrpc_example.module	27 Aug 2010 14:05:53 -0000
@@ -102,7 +127,7 @@ function xmlrpc_example_menu() {
+ * Present a form that uses xmlrpc to add or substract.

misspelled again.

Powered by Dreditor.

I like the idea of enabling the alter based on a module setting. I think it would be better to conditionally alter the existing behavior than to alter a nothing-behavior like in #8. This will mean we have to explain a bit more in the xmlrpc_example_demo_form() so that people can discover the setting. And they don't have to grok the alter to grok the basic usage of xmlrpc.

It has to be obvious and easy to understand what's going on. The two techniques you suggest are confusing to everybody but the author (I use them often, but sometimes confuse even myself :-)

Perhaps only alter depending on a value in the payload?

Sorry, but I'm going to bump seriously into this example:
- D6 and D7 examples of xmlrpc are very different
- D6 version does not have an UI to verify this example

Here is my proposal:
- xmlrpc example (for both D6 and D7) will include
- xmlrpc server code (with a settings form)
- xmlrpc client code (with a testing form)

The xml client part (basically it is a form)
- will include an endpoint field (where is the xmlrpc located, to localhost by default)
- a function field (probably grabbed automatically using xmlrpc:system.listMethods)
and fields to call the selected service.

The xml example server part
- A set of defined services (very simple) (addition and substraction of two values)
- add (arg1, arg2)
- sub (arg1, arg2)
- the hook_xmlrpc_alter can be used (with some values in the settings page) to insert a third argument (a session key or something like) in all the methods.. resulting in add( key, arg1, arg2) and sub(key, arg1, arg2)

the hook_xmlrpc_alter can't be backported to D6

May this be clear enough?

Apologies, after reading the hook_xml_alter declaration I've come to the conclusion that it can't be used in that way. fgm, this _alter hook can only be used by the server to alter the input from the client before processing it, it can't be used to include/modify current xmlrpc methods defined in the server.

So, a new proposal:

Actually, I love the way xmlrpc example is declared in D7, so I'm going to backport 'as is' to D6 (as long as D6 does not support hook_xmlrpc_alter).

For the D7 part, the way to introduce hook_xmlrpc_alter and put some 'learning' skills there is explain clearly that it may alter the input from the client.. Due to the current example implementation it can be done by:
- remove or add services being called,
- change the service being called,
- change the arguments of the service

Any ideas?

Oh, I see, you are right, the description in the API confused me a little bit: "This hook lets at module modify the callback definition for already declared XML-RPC methods, when they are being invoked by a client." So, it allow to alter the methods and is called on every call to the services. Ok, I get the point (the previous one).

So clearly we need to find an use case where to implement this hook, and adding a new method is not the answer definitely. Modifying an existing method is the right way to do so, but what part? the callback?, the number of arguments? I'll be working on this patch tonight, so if you have an idea we can discuss here or on irc.

Thanks for the correction!

Works for me.

So, I've separated client, server, and alterations part in the code, and documented each of them. I've updated the testcases to reflect all the changes. It is a major rewrite, I know, probably it is better to patch, and test, and then review the comments than trying to review the patch itself.

who will put some time on this review? ;)

ok:Detect invalid patch format, Ensure the patch only contains unix-style line endings.

I'd try to.. now!

once more and I give up..

uff.. apologies for this boring notifications, I have a devel environment just installed and I'm missing some configs to avoid all these situations..

arfm... I'll hate netbeans and tortoisecvs for the rest of my life! XD

Just changing the title according to the new patch and module code.

I have to fix a lot of issues pointed by chx on irc, so I'll be rewritting it.

Updated with chx review:
- clearly specify what is required and what is optional to create a XML-RPC server or client.
- Notice that error numbers are arbitrary, selected for our implementation.
- Spelling and grammar issues, of course
- Comments now are on the right place

So I guess it is ready for review again.

OK, here's my update with mostly quite minor changes, improving comments, improving UI text, etc.

I've also attached a diff of this against #32.

This does not include fgm's suggestion from #34

FYI: Since HEAD is broken by the render example patch, we'll have to look at the actual testing results to see this succeed/fail (It comes back green in my environment)

EDIT: Unfortunately with HEAD broken all testing gets deferred.

Green now, and HEAD is fixed.

Hmm. I guess I won't call patch-to-patch comparisons ".diff" any more. Don't know if I can set this to needs review or not.

I'll do the tests for this locally to verify it, I do have to take a look to the changes yet! leaving as needs review for now.
Thanks rfay,
ilo

I'm unable to apply the patch at #35, however I've been reviewing the .diff (as I know the rest of the code) and here are my comments:

the following line has two typos: implEmentation and fiRSt (they were my fault, of course)

// The following code is the XML-RPC implmentation of the server part. The fisrt

Edited:

Now I saw why I was unable to apply the patch, and solved it (the .install file was lost, things that happen with netbeans). By the way, there is another typo in the xmlrpc_example.install file: this is not the simpletest example ;)

  * Install, update and uninstall functions for the simpletest_example module

Please, rfay, can you fix these typos also and commit this change so I can start working with the multicall implementation?

Committed to HEAD: http://drupal.org/cvs?commit=430838 (and followed up with the typo fixes)

@ilo: You might be interested in the far easier git workflow for your patching as well: http://groups.drupal.org/node/91424. Not that I'm partial to it.

And of course, you're welcome to commit something like this that you approve of.

Ok, rfay, I'll consider next time!

so.. I've updated some documentation in the server and client also, fixed a wrong t(), and implemented two new buttons:
- Request methods: ask the xmlrpc server for available methods
- Add and Subtract: performs a multicall to add and subtract the numbers in the same request.

fgm, please review if this complies with what you suggested.

Thanks for the review fgm!, fixed all the coding and descriptions, and added an autoreplacement in the IDE for xml-RCP hehe.. Yes, you are right, description was confusing because I copied the wrong link. Anyway I've removed all of them.

Looks OK to me. Waiting for @fgm's RTBC.

Thanks fgm, I'll do the changes and update the patch. I'm not sure, but looks like I no longer receive any email from the examples issue queue.

Rerolled with @fgm's suggestions. No additional changes.

wow.. It passed all the tests locally :/

The change of info files went in today, so don't neglect the possibility that there may be something in that that affects this. #1007580: Remove unnecessary files[] declarations from .info file

I've seen that only the page tests are failing, it is the output of a drupal_set_message() that is being checked. Looks like that request is failing (not in my local setup), however everything else, I mean the method calls, even the multicall are working. If someone can test the patch locally to see if this is only my case I'd appreciate it. Otherwise I'll put a debug message to see how is the output in the d.o. test environment.

Remember the two major things different between you and the bot:

* Clean URLs are not on on the bot
* The bot executes the installed D7 in a subdirectory

I'll run the tests and report back

Yeah, thanks rfay, but the failing tests are just a Post submission result, and it is anoying me because the methods are all working, and other checks following the same procedure are working also. Perhaps the testbot has not the 'system.listMethods' method enabled? that is the only clue I have about these failings tests.

system.listMethods is already in the unpatched example - I added it the other day in #1004918: Improve xmlrpc_example by adding system.listMethods, and tests pass.

Doing a retest on HEAD: http://qa.drupal.org/pifr/test/26934

Edit: it passed, but there is no test that invokes it. So maybe that was kind of silly.

I do get the failures when running Drupal in a subdirectory and with clean URLs turned off.

Thanks so much for the test, rfay. The only difference with my setup was the clean urls, and as the test passed in #46 I think that it must be something in the patch that has to do with urls.. My bet is that fgm's suggestion about use url() instead of $GLOBALS has the key.

Thank you both!

IMO when fgm & ilo are good with this it can be committed.

then we are waiting for fgm's last word here.

Thanks fgm!

My comments (rfay, feel free to unbalance and resolve)

- Open question: we must not talk about xmlrpc, as long as we don't document php. We are teaching/explaining Drupal's XML-RPC system. I don't see any problem including resource links, but we are not explaining what XML-RPC is, just using it with Drupal.

- url() works fine for urls, and perfect for menu entires. It happens that xmlrpc.php is a file, not a menu link, and using url() when clean urls are disabled renders a final RPC endpoint with '?q=xmlrpc.php' that made the module fail (that was the reason for the latest failing tests).

Totally agree in the rest of comments. I will fix them, but wait for Randy to unbalance these issues before submitting the patch.

Thank you for being so rabidly picky! hehe

Thanks for the follow up on this, fgm. I'll rewrite the patch for the comments at #63 in the following days.

Looks like url() only considers the 'script' parameter when clean urls are disabled. So, I guess the only way to make it working redgardless the status of clean urls is submitting both: path and script. Luckly, xmlrpc.php ignores the 'q' argument without clean urls (the request is something like this: xmlrpc.php?q=xmlrpc.php).

Size has increased because of double quote changes.

ayyye.. the paths!

To be honest I don't like it either. We have been playing ping pong with that patch for many many issues, I'm pretty sure it can wait a few days untill we find a better way, fgm.

Sun helped me on irc to get a fix for this using url($GLOBALS['base_url'] . '/xmlrpc.php', array('external' => TRUE));. This way hook_url_outbound_alter() could still handle it, but 'script' parameter is only for alternative index.php when clean urls are disabled (phpDoc description for the script parameter is very clear on that.)

So.. fgm, final review, I only changed that in this last patch.

Finally!, thanks fgm, rfay.

committed as http://drupal.org/cvs?commit=481996

Now, lets backport this do DRUPAL-6--1 to keep both branches as close as possible.

#8: example_xmlrpc_alter.patch queued for re-testing.

Setting to correct status; this is waiting for a port to d6.

It's end-of-life time for the 6.x-1.x branch of Examples. See #2642596: D6, D7 Roadmap for Examples