Move xmlrpc includes into xmlrpc.module

.

Framework is our new tag for all of these issues.

2. Adds xmlrpc.module - this implements hook_menu() and registers xmlrpc.php as a callback.

Sleeping over this, there's one non-obvious consequence here: Without clean URL support, you cannot use xmlrpc.php anymore.

If we consider that to be a problem, we could provide the current/legacy xmlrpc.php within the module folder, so sites that cannot or do not want to enable clean URLs are able to copy the file into their site's root directory.

I think that clean urls should just be a requirement of using xmlrpc module. Another approach would be that we just completely remove the xmlrpc server and let contrib handle it with Services module (we've already removed the prime reason to even have an xmlrpc server, which was Blog API)

+1 for separate module, +1 for this not being in core (and the reason why I won't RTBC the patch)

> Another approach would be that we just completely remove the xmlrpc server and let contrib handle it with Services module (we've already removed the prime reason to even have an xmlrpc server, which was Blog API)

+1 for this option. I don't see why anyone would use core's xmlrpc server when there is Services module available.

Similarly, the code for making xmlrpc calls could be moved out to Clients module. There's no reason for core to be favouring xmlrpc over any other type of service call.

PS. Though isn't part of Services moving into core in D8 anyway? If so, won't that initiative be handling the cleanup of core's xmlrpc code?

Services provides a wrapper around the core xmlrpc functionality so I would be against this removal for the most part as a services maintainer. If xmlrpc leaves core I can pretty much say it will die (from a services prospective) because the vast majority of services maintainer are only interested in json servers.

@catch total. my comment was in response to the comments of remove xmlrpc from core.

+++ b/modules/xmlrpc/xmlrpc.infoundefined
+++ b/modules/xmlrpc/xmlrpc.infoundefined
@@ -0,0 +1,5 @@

@@ -0,0 +1,5 @@
+name = XML-RPC ¶
+description = Provides XML-RPC functionality.

Trailing whitespace, other docs seems to be ok

Powered by Dreditor.

> I have no idea whether the web services initiative will have anything to do with xmlrpc by the time it's done, currently that is 100% vapourware, so I have limited interest in having mess in the root directory waiting on that.

Yup, fair point. I see what you mean: clean up what we definitely can now, clean up more later if/when other things happen :)

Just a few points -- and TBH some of it is stuff that could be looked at later.

+++ b/modules/xmlrpc/xmlrpc.module
@@ -0,0 +1,54 @@
+  $items['xmlrpc.php'] = array(

I had my doubts about this, but I just looked at xmlrpc.php and it does drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL). How much do we lose going via the menu system rather than straight in?

+++ b/modules/xmlrpc/xmlrpc.module
@@ -0,0 +1,54 @@
+    'type' => MENU_SUGGESTED_ITEM,

Surely MENU_CALLBACK?

You won't get anything visible back from this with a GET, just a white screen (or a message saying 'only POST', I forget).

+++ b/modules/xmlrpc/xmlrpc.module
--- /dev/null
+++ b/modules/xmlrpc/xmlrpc.pages.inc

Why do we need both the xmlrpc.pages.inc and the xmlrpcs.inc?

Powered by Dreditor.

#28: xmlrpc_module.patch queued for re-testing.

Oops. First I saw CommentBlockFunctionalTest fails, and only after requesting the retest did I notice that lines have gone missing in the latest patch...

Those are some very weird tests that are failing.

A lot of them are to do with file_put_contents(). How can that have been affected by this?

More likely to be a testbot issue, perhaps out of disk space.

#28: xmlrpc_module.patch queued for re-testing.

It appears that the most recent patch is not creating the xmlrpc module, but removing all traces of the xmlrpc server and client from core. I'm okay with this, but I'm certain that's not what you were aiming for, so back to CNW.

Yeah, catch probably forgot to add new files on latest patch (#28)

subscribe

Blog module was the only thing in Drupal core using the XML-RPC system. Now that Blog module is no longer in core, there is no reason to have XML-RPC in there.

Also, I moved xmlrpc.module, what catch was working on, to http://drupal.org/node/1285602 . We can promote it to a full project if we want. I've given a bunch of you commit rights on there.

*cough*

:%s/Blog module/Blog API module/g

Please create a separate issue for removing xmlrpc altogether. I'll explain on that one why I'm going to mark it postponed.

#28: xmlrpc_module.patch queued for re-testing.

sub

Please create a separate issue for removing xmlrpc altogether. I'll explain on that one why I'm going to mark it postponed.

Here ya go: #1285726: Remove XML-RPC

#26 / #28 needs to be re-rolled.

The patch from #26 was not re-rollable at all, so I basically recreated it and also added the changes advertised in #28 but not really added to that patch :)

We've transferred this work to: http://drupalcode.org/sandbox/sun/1255586.git/shortlog/refs/heads/platfo...

Attached patch contains a change by @sun that renames xmlrpc.pages.inc to xmlrpc.server.inc.

A PHP file actually works better than a callback for fastCGI environments, since some headers are not passed through the URL rewrite process.

A separate file could actually have a different bootstrap? e.g. never need the theme layer?

I think sun had such a file for JS callbacks at one point.

I don't think we want more then one front-end controller (ie. a PHP entry point) for now. The are refactoring going on in #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel so let's stick with the proposed xmlrpc.inc routing entry for now.

Since the functionality was enabled by default, do you think we should retain the upgrade path for it?

/**
 * Move XML-RPC functionality to its own module.
 *
 * @see http://drupal.org/node/1033818
 */
function system_update_8005() {
  module_enable(array('xmlrpc'));
}

Yes, definitely. Thanks Rob :)

+++ b/core/modules/xmlrpc/xmlrpc.moduleundefined
@@ -0,0 +1,53 @@
+ /**
+  * @file
+  * Enables XML-RPC functionality.
+  */
+
+ /**
+  * Implements hook_menu().
+  */
+ function xmlrpc_menu() {
+   $items['xmlrpc.php'] = array(
+     'title' => 'XML-RPC',
+     'page callback' => 'xmlrpc_server_page',
+     'access callback' => TRUE,
+     'type' => MENU_CALLBACK,
+     'file' => 'xmlrpc.server.inc',
+   );
+   return $items;

There's some extra opening whitespace there. Attached patch doesn't have it. If it goes green, I'd consider this RTBC!

14 days to next Drupal core point release.

+++ b/core/modules/system/system.installundefined
@@ -1761,6 +1761,15 @@ function system_update_8004() {
+* Move XML-RPC functionality to its own module.
+*
+* @see http://drupal.org/node/1033818
+*/
+function system_update_8005() {
+  module_enable(array('xmlrpc'));
+}

I don't agree with this. This means that every single Drupal site that upgrades from 7 -> 8 will have xmlrpc.module enabled, even if they don't need it. I'd say let's leave it disabled, and then document an API change: if your modules depend on XML-RPC functionality, then you need to enable the xmlrpc module.

+++ b/core/modules/xmlrpc/xmlrpc.moduleundefined
@@ -0,0 +1,53 @@
+ function xmlrpc($url, $args, $options = array()) {
+   module_load_include('inc', 'xmlrpc');
+   return _xmlrpc($url, $args, $options);

Should this be named something else? Most of the other modules that we have in core don't have functions that are named the same as the module name. What about xmlrpc_request()?

I'd say let's leave it disabled, and then document an API change

Change notification it is!

What about xmlrpc_request()?

Sure!

Uses xmlrpc_call() instead of xmlrpc_request() since that's already defined.

Just a note: this issue, and others related to XMLRPC in D8, are part of the program for the Paris code sprint on 2012/04/28-29. More info at http://groups.drupal.org/node/223174

Re-rolled and adjusted for latest HEAD.

This code lives in the platform/platform-xmlrpc-1033818-sun branch now.

Note: I've intentionally reverted the unnecessary xmlrpc() API change.

#58: platform.xmlrpc.58.patch queued for re-testing.

Re-roll.

Got it! The problem was WSCCI fallout from the index.php change.

Excellent, thanks!

Thanks! Also noticed the system.api.php hook_xmlrpc() docs could be moved in too.

Awesome!

#66: 1033818.patch queued for re-testing.

Also, updated the issue summary.

+++ b/core/modules/xmlrpc/xmlrpc.module
@@ -0,0 +1,68 @@
+        '@xmlrpcapi' => 'http://drupal.org/node/44895',
+        '@xmlrpc' => url('xmlrpc.php', array('absolute' => TRUE)),

Minor, but the placeholders are defined in the opposite order they are used. Also, maybe a dash or two would make the placeholder names more readable.

+++ b/core/modules/xmlrpc/xmlrpc.module
@@ -0,0 +1,68 @@
+  $items['xmlrpc.php'] = array(

Is there a reason this is 'xmlrpc.php' vs. just 'xmlrpc'? At first I didn't want to mention this as I thought it would be bikeshedding, but then I noticed that if you don't have mod_rewrite, then that path looks like example.com/index.php/xmlrpc.php, which is super weird IMO.

Leaving at RTBC for now.

• Switching those declarations to be the right order makes sense to me!
• Switching to just "xmlrpc" for the menu callback should probably be a follow-up issue. This just tries to keep backwards compatibility with Drupal 7. I agree it's the right path though!

IMO, backwards compatibility shouldn't be a huge concern here - if a site is making use of the XML-RPC server and they're upgrading from D7->D8, they'd likely also update whatever system is using XML-RPC.

Also? Who really doesn't use mod_rewrite ;)

Created #1627496: Switch XML-RPC callback from "xmlrpc.php" to "xmlrpc" to get that fix in afterwards :-) . I'll stick it in the issue summary.

Updated issue summary.

RTBC++

http://drupal.org/node/1632592

Perfect!

Oh yeah, priority should change too.