To work with web services like twilio.com, the Services REST server needs to return XML that is not structured as RESTServerView::render_xml() currently does (i.e., recursively taking a StdObject and stuffing it into a DOMDocument, using "result" as the root).

This may well be the standard, but Twilio isn't standard. To get around this, I've patched 7.x-3.x to see if 'xml' response formatted data is of type SimpleXMLElement or DOMNode. If it is, the patched REST server will render these objects out as they are, w/o any further processing or wrapping.

CommentFileSizeAuthor
#1 issue-1924270-version-1.patch1.15 KBTorenware

Comments

Torenware’s picture

Torenware commented
Status: Patch (to be ported) » Active
StatusFileSize
new issue-1924270-version-1.patch1.15 KB

Uploading the first version of the patch.

Torenware’s picture

Torenware commented
Status: Active » Patch (to be ported)

Change status to patch and see what happens...

kylebrowning’s picture

kylebrowning commented
Version: 7.x-3.3 » 7.x-3.x-dev
Status: Active » Needs review
ygerasimov’s picture

ygerasimov commented
Category: feature » support
Status: Needs review » Fixed

I don't think we should do this change in the core of services as we might have people who depend on this functionality and changing XML structure we will do big break. We have API to change formatter (hook_rest_server_response_formatters_alter) and in this way we can overwrite the way XML is rendered. This should happen for this particular case with twilio.com.

Torenware’s picture

Torenware commented

The code in question should support anyone depending on the old behavior. Take a look at the patch -- it checks for the object formats and handles them, otherwise, it gives the old behavior.

I'll need to look at the formatter API; I'm not sure off hand if it will do what I need.

Torenware’s picture

Torenware commented

OK, I've looked at hook_rest_server_response_formatters_alter. Yes, it will work -- all I have to do is copy your implementation for RESTServerViewBuiltIn, since you folks have declared all the important methods as "private" -- so I have to copy over your implementation as not to break anything. This is unfortunate.

If your implementation is not "broken", can I ask how it is possible to render out the following bit of XML:

<?xml version="1.0" encoding="utf-8"?>
<Response><Sms to="+15555559576" from="+15555551212">We received your message, and will get back to you within 24 hours.</Sms></Response>

This is a real example; it's what is required to support the Twilio API. Your current code (which does not use standard XML libraries -- it parses it with regular expressions) does not appear to be able to do this.

You don't have to take my patch, of course. But if you want your rest server to be "best of breed", then use an implementation that actually generates well formed XML. Right now, it really doesn't.

Please consider using the standard PHP libraries for operating on XML.

ygerasimov’s picture

ygerasimov commented
Status: Fixed » Needs work

I have reviewed patch #1 once again and I agree that we should allow resources to construct SimpleXMLElement as they like and we should take it.

So I am fine with committing it.

Some comments about the code:
- Make sure we do not have trailins spaces (patch #1 has some).
- As we are using dom_import_simplexml() we need to have error handling.

I am thinking about something like:

<?php
if (is_object($data) and $data instanceof SimpleXMLElement) {
   $dom_import = dom_import_simplexml($data);
   if ($dom_import === FALSE) {
      return t('Error while converting XML');
   }
   // Recursively import the whole node.
   $root = $doc->importNode($dom_import, TRUE);
   $doc->appendChild($root);      
}
else {
   // If some other type of data, process it here.
  $root = $doc->createElement('result');
  $doc->appendChild($root);  
  $this->xml_recurse($doc, $root, $data);
}
?>
marcingy’s picture

marcingy commented
Category: support » task
marcingy’s picture

marcingy commented
Issue summary: View changes

Changed "result" from a tag, since the filters were removing it.

Status: Needs work » Needs review

kylebrowning queued 1: issue-1924270-version-1.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: issue-1924270-version-1.patch, failed testing.

kylebrowning’s picture

kylebrowning commented
Issue summary: View changes
Status: Needs work » Closed (won't fix)

Cleaning issue queue, if you want to work on this, re-open the ticket.