Charset definition missing in XML-RPC

Had a problem with the missing charset definition. My regex on my client crashed often by transmitting ä,ö,ü ect. over XML-RPC. After adding the missing code in XML-RPC everything worked fine.

Looks good to me, but while we're at it we could also fix a long standing issue with some weak old parsers which don't like missing a space between an attribute and a PI termination.

Resetting to CNR to trigger the bot, but otherwise RTBC for me.

Is it possible to write a test for this?

Also, marking for backport to D7 and D6, per the OP.

Looks good to me, just some minor issues in coding standards. Once they are fixed, IMHO it is RTBC.
Nice to have better tests for the XML-RPC clients and server :-)

+++ b/core/modules/xmlrpc/lib/Drupal/xmlrpc/Tests/XmlRpcMessagesTest.phpundefined
@@ -62,4 +62,33 @@ protected function testAlterListMethods() {
+    //debug($headers, NULL, TRUE);

Remove debug statements not used. There are two of them.

+++ b/core/modules/xmlrpc/lib/Drupal/xmlrpc/Tests/XmlRpcMessagesTest.phpundefined
@@ -62,4 +62,33 @@ protected function testAlterListMethods() {
+    // we can't use the xmlrpc() function here,
+    // because we have to access the full response of drupal_http_request()

Capital w: "We". And the comment should end with a ".".

Thanks Markus!

My comments have been addressed, so catching other's attention.

Patch looks good but I think we can drop the custom assertion. Without that, if the test fails, it'll show you what the expected vs. actual value was anyway.

+    $this->assertIdentical(strpos($headers['Content-Type'], 'text/xml; charset=utf-8'), 0, 'Request Content-Type is "text/xml; charset=utf-8"');
+  }

Patch backported for 7.x

Hey @mkalkbrenner D8 patch in #13 needs a reroll ... no longer applies

$ curl https://drupal.org/files/ajax-number-1964098.pass_.patch | git apply
error: patch failed: core/modules/xmlrpc/xmlrpc.inc:560

After #1285726-48: Remove XML-RPC moving this to 7.x since it has a patch for that (in #14). It could also go to the contrib xmlrpc first, but I'm not sure.

This should now apply, based on the patch in #13, let's see the errors.

It might look like this : not sure this is the exact equivalent : specifically, I wonder whether the first assertIdentical() in XmlRpcMessagesTest::testRequestAndResponseEncodingDefinitions() has any chance of failing, ever.

Comment on the first assert in XmlRpcMessagesTest::testRequestAndResponseEncodingDefinitions() was not correct. No other change from #26.

Looks nice to me. Apparently test results did not came back, but https://qa.drupal.org/pifr/test/1099603 shows its green.

+++ b/src/XmlRpcTrait.php
@@ -4,11 +4,11 @@
  * Contains XmlRpcControllerBase.php.
  */

Minor: this comment is certainly not true :)

Fixed that and another minor whitespace.

Fails are unrelated. So postponing on #2628800: Tests fail on xmlrpc head with invalid !link placeholders..

Well, the guzzle fail is not unrelated. We can fix that here, while the !link fail fix is committed from #2628800: Tests fail on xmlrpc head with invalid !link placeholders..

Rerolled on current HEAD. One error appears within Guzzle in local tests.

One of the fails was just a typo, most (all ?) others related to the PSR-7 switch.

Yay, looks good to me, thanks for the updates. The coverage for setting the charset and for testing it all seems to be adequate. Also getting this in will help with starting the backports to Durpal 7 :)

Committed to 8.x-1.x HEAD. Thanks all.

Backport Drupal 7 anyone? Should we move this issue back to core or open another one?

Same topic, same issue, IMO. But not for tonight.

First step : to ease porting and especially later maintenance, test conversion to PSR-4 format. Setting to CNR to trigger bot, but is actually CNW. No actual change besides PSR-4 conversion, so no interdiff.

Oops, wrong patch : included renaming our PHP scripts/run-tests.sh to scripts/run-tests.php.

Very rough backport. Not crashing but many fails.

Everything should be fixed except the new encoding test being skipped fo rnow.

Forgot one short-form array... when can we require PHP 5.4 on D7 ?

All tests converted and passing locally, plus a pass of coding standards.

In case this is not clear, the conversion is finished and awaiting reviews...

+++ b/includes/xmlrpc.inc
@@ -594,17 +596,25 @@ function _xmlrpc($url, $args, $options = array()) {
-  $options['headers']['Content-Type'] = 'text/xml';
+  $options['headers']['Content-Type'] = 'text/xml; charset=utf-8';

+++ b/includes/xmlrpcs.inc
@@ -124,9 +124,9 @@ function xmlrpc_server_error($error, $message = FALSE) {
 function xmlrpc_server_output($xml) {
-  $xml = '<?xml version="1.0"?>' . "\n" . $xml;
+  $xml = '<?xml version="1.0" encoding="utf-8" ?>' . "\n" . $xml;
   drupal_add_http_header('Content-Length', strlen($xml));
-  drupal_add_http_header('Content-Type', 'text/xml');
+  drupal_add_http_header('Content-Type', 'text/xml; charset=utf-8');
   echo $xml;
   drupal_exit();
 }

These are the key changes, there are a bunch of other misc changes which are bringing the code in line with the Drupal 8 branch I guess. Those do not affect the tests or the issue here, so it would be good to break those out to its own issue IMHO.

For the changes in scope for this patch, I am not sure about the refactoring of the tests to Drupal 8's structure, that is not done in other modules I believe. I can see how it makes backporting fixes easier. Would be great to get committer feedback on it.

Basically, there are indeed 4 main changes here :

• the encoding change which Gábor pointed out, along with its associated test
• other tests backported from D8 because they had not been backported to D7 when XML-RPC was still in core in early D8 releases
• a conversion to PSR-4 for tests, to ease ongoing maintenance
• coding/commenting standards fixes to make all of this consistent with our current level on D8

I think those would be important to break into their own issues, since they make reviewing this change hard.

Split into 4 child issues and marked postponed on the child issues. We can reactivate it when they're done.