Advanced REST service formatters + UI setting

Yes, at the moment you can only define JSON formatted REST services in the UI. If you define a REST service in code you can pass the formatter class in the settings ($service->settings['formatter']). See also #1087538: Support custom formatter to be exportable.

So we need a UI addition in wsclient_rest.module. Implement wsclient_rest_form_wsclient_service_form_alter() and add an option list to select either from JSON or XML. Add a submit handler that saves the corresponding formatter class to $service->settings['formatter'].

I tried to make this change in order to call a webservice which result is in XML format instead of JSON.

I did the same than Belette.

$this->client = new HttpClient(NULL, new HttpClientXMLFormatter(HttpClientXMLFormatter));
// $this->client = new HttpClient(NULL, new HttpClientBaseFormatter(HttpClientBaseFormatter::FORMAT_JSON));

However I got an error because i dont have the constant HttpClientXMLFormatter.
Notice: Use of undefined constant HttpClientXMLFormatter - assumed 'HttpClientXMLFormatter' in WSClientRESTEndpoint->client() (line 45 of sites/all/modules/wsclient/wsclient_rest/wsclient_rest.module).
Do u know how I can solve it? Thank you in advance.

Attached is a patch that does what @klausi said in #4.

+++ b/wsclient.module
@@ -243,3 +243,25 @@ function wsclient_map_type($service_name, $service_types, $type) {
+/**
+ * A special Rules data getter callback for lists derived from the XML formatter.

This has nothing to do with Rules directly, this an Entity property API callback.

+++ b/wsclient.module
@@ -243,3 +243,25 @@ function wsclient_map_type($service_name, $service_types, $type) {
+ *
+ * @param $data
+ * @param array $options
+ * @param $name
+ * @param $type
+ * @param $info

param/return documentation is missing? See http://drupal.org/node/1354#functions . But since this a Entity property API callback I think it is enough to add the appropriate @see reference that points to the callback docs.

+++ b/wsclient.rules.inc
@@ -97,6 +97,18 @@ function wsclient_type_info_populate(array &$type_info, $service_name, array $al
+      if (strpos($info['type'], 'list<') === 0) {
+        if ($service = entity_load_multiple_by_name('wsclient_service', array($service_name))) {

use entity_load_single() instead.

+++ b/wsclient_rest/wsclient_rest.module
@@ -38,7 +38,8 @@ class WSClientRESTEndpoint extends WSClientEndpoint {
       if (isset($this->service->settings['formatter'])) {
-        $this->client = new HttpClient(NULL, $this->service->settings['formatter']);
+        $formatter = $this->service->settings['formatter'];
+        $this->client = new HttpClient(NULL, new $formatter());

This will not work when the module is upgraded and the formatter does not exist. Please add an isset() call and a default fallback to JSON. Or you can add hook_update_N() and write a default formatter to all existing service definitions.

+++ b/wsclient_rest/wsclient_rest.module
@@ -82,6 +83,11 @@ class WSClientRESTEndpoint extends WSClientEndpoint {
+      // If returned a SimpleXMLElement, convert it to nested array.
+      if ($response instanceof SimpleXMLElement) {
+        $response = json_decode(json_encode((array) $response), 1);

why is this json encode/decode necessary? Please add a comment.

+++ b/wsclient_rest/wsclient_rest.module
@@ -113,6 +119,74 @@ class WSClientRESTEndpoint extends WSClientEndpoint {
+
+  $default_value = $form_state['wsclient_service']->settings['formatter'] ?: 'HttpClientBaseFormatter::FORMAT_JSON';

?: is only available in PHP 5.3, right? So that will break 5.2 installations.

+++ b/wsclient_rest/wsclient_rest.module
@@ -113,6 +119,74 @@ class WSClientRESTEndpoint extends WSClientEndpoint {
+ *
+ * @param $form

@param docs are not necessary on form handling functions, see http://drupal.org/node/1354#forms

You are not changing the test cases here, have you checked that they still work? I have not run them myself for a long time, so be prepared for failures ...

Ah, I just realized that there is already an isset() check around the formatter setting, so never mind that :)

Actually I don't think this is a stable release blocker right now.

Also, supporting configurable formatters seems a bit more work. They can have constructor variables, like the HttpClientXMLFormatter can use a $default_root variable to specify the XML root element. Should we invoke a wsclient_rest_formatter_info hook to collect formatter classes? Should service settings or even operation settings have entries for the constructor arguments of the formatter? And should we expose that in the UI as well?

Also a public example for wsclient_examples testing would be nice, so that we know XML works as we expect.

• Backward compatibility: if old-style formatter definition found in the settings it will be handled "old-stylish".
• Composite formatter: You could define different formatters for serializing request and parsing response. See related issue, that is going to be fixed by this patch: #1362820: Allow web service client to use different HttpClientFormatters for serialization and parsing.
• Per formatter settings UI: Each formatter could expose its own settings to UI. They will be appended to the formatter form. The formatter settings only appear if corresponding item is selected (using '#states').
  
• New hook: Formatters are collected via invoking a hook. It is fully defined in included API-file.
• Fully exportable: Just see the screenshot.
  
• No migration: Schema was not touched: all settings are stored inside $service->settings - so no updates are required, it works right after patching.
• Optimized a bit: As there were introduced new classes declarations, all classes (including WSClientRESTEndpoint) were moved into a separate file, which is defined in .info. It may give a bit performance due to lazy loading.

Looking at the screenshot, I've noticed the typo: the description of 'Adaptive root' is misspelled.
Here is the new patch, fixed.
Interdiff is also attached.
Please note: the patch is git-aware (i.e., was made using git format-patch), so it should be applied using git am. See https://drupal.org/node/1146430 for more details.

Hello PatchRanger, I just realized that my patch for $request_alter:
https://drupal.org/node/1934274#comment-7667253

... conflicts with yours. According to you, what is the best way to solve the problem? Should I post a new patch? Or maybe you can merge mine into yours, adding support for $request_alter too?

@tito.brasolin I am glad that you've posted your comment, because I am using both patches too and do have the same trouble. I've handled the conflicts manually, believing that patches will be committed soon. In general I wouldn't prefer to merge patches in order not to harden the reviewing process. But in this particular case it could be the option.
Here is the new re-rolled patch, which introduces merging with yours and some other changes (see interdiff).
Please review.

Fixing the warnings.

Thank you @PatchRanger, I just did a quick test and everything worked almost fine. My web service is defined in hook_default_wsclient_service() and it worked without any setting... till now.

The fatal error is: "Class name must be a valid object or a string in [...]wsclient_rest.inc on line 40"

Such a beviour may break existing installations, I had to explicitly add a few lines to my module to make it work again:

  $service->settings = array(
    'serializer' => array(
      'class' => 'WsclientRestJSONFormatter',
    ),
    'parser' => array (
      'class' => 'WsclientRestJSONFormatter',
    ),
  );

Thanks for feedback.
Fixed patch is attached: I had to add update hook - so the patch doesn't have "No migration" advantage anymore :)
Should work for everyone out-of-the-box after running update.

1. +++ b/wsclient_rest/wsclient_rest.api.php
   @@ -0,0 +1,57 @@
   +/**
   + * Alter the http request.
   + *
   

   Should be "HTTP".

2. +++ b/wsclient_rest/wsclient_rest.api.php
   @@ -0,0 +1,57 @@
   +  if ($service->name == 'myservice') {
   +    // remove empty parameters
   

   Comments should start upper case and end with punctuation.

3. +++ b/wsclient_rest/wsclient_rest.inc
   @@ -0,0 +1,282 @@
   +  /**
   +   * @return HttpClient
   +   */
   

   Missing function description. I think you can just use {@inheritdoc} here, see https://drupal.org/coding-standards/docs#inheritdoc

4. +++ b/wsclient_rest/wsclient_rest.inc
   @@ -0,0 +1,282 @@
   +    // Services module compliance: post fields should be in the $data array
   +    // instead of $arguments.
   +    if ($type == 'POST') {
   +      $data = array_merge((array) $data, $arguments);
   +    }
   

   errr, what? This has nothing to do with this issue, so this should not be introduced here. Let's stay focused on the REST UI formatter setting. Otherwise this patch becomes too complicated to review/commit. Other issues can depend on this patch, I think it is not far from being committed.

5. +++ b/wsclient_rest/wsclient_rest.inc
   @@ -0,0 +1,282 @@
   +  public function formAlter(&$form, &$form_state) {
   

   Doc block is missing. Please check all introduced methods and all classes.

6. +++ b/wsclient_rest/wsclient_rest.install
   @@ -0,0 +1,20 @@
   +function wsclient_rest_update_7100() {
   +  foreach (entity_load_multiple_by_name('wsclient_service') as $service) {
   +    foreach (array('serializer', 'parser') as $formatter) {
   +      if (empty($service->settings[$formatter]['class'])) {
   ...
   +      }
   +    }
   +    $service->save();
   +  }
   

   So this breaks existing service descriptions that have defined some other formatter? It will just overwrite it with WsclientRestJSONFormatter. I think we should not alter existing service definitions, we have the extra function anyway? Just mark it as "custom"?

7. +++ b/wsclient_rest/wsclient_rest.module
   @@ -18,98 +18,149 @@ function wsclient_rest_wsclient_endpoint_types() {
   + *
   + * @param array $settings
   + * @param string $formatter_type
   + *   Either 'serializer' or 'parser'.
   + * @return boolean
   

   Each @ annotation should also have a description, see https://drupal.org/coding-standards/docs#param . Also elsewhere.

8. +++ b/wsclient_rest/wsclient_rest.module
   @@ -18,98 +18,149 @@ function wsclient_rest_wsclient_endpoint_types() {
   + * @return boolean
   

   Should be "bool", see https://drupal.org/coding-standards/docs#types

9. +++ b/wsclient_rest/wsclient_rest.module
   @@ -18,98 +18,149 @@ function wsclient_rest_wsclient_endpoint_types() {
   +function wsclient_rest_has_old_formatter($settings) {
   +  return !empty($settings['formatter']) && (new $settings['formatter']() instanceof HttpClientFormatter);
    }
   

   Why do you have to instantiate the formatter? Just use class_implements()? Also elsewhere.

+++ b/wsclient_rest/wsclient_rest.inc
@@ -0,0 +1,282 @@
+        $formatters = array();
+        foreach (array('serializer', 'parser') as $formatter_type) {

serializer and parser? Why parser? That are bad names. Let's use "send_formatter" and "receive_formatter". We can also make the backward compatible check then, if "formatter" is set we just use that, otherwise the two send/receive formatters.

I find it annoying that we have to provide all that custom formatter classes that just overwrite their parent with one line. Is that really necessary? Why does http_client not provide us with that classes?

Other than that this looks great, good work!

Currently there is no option to exclude empty elements from the request body. I added modified this code to support this functionality. Not sure where this code should belong, so I created an issue for it: #2109963: Option to exclude empty elements from request body

I totally changed the example to go to the easiest use case I could. And so ease any helper's work :)

Updated patch from #17 to apply to latest head.

Darn.
I just came in from looking at a small fix to allow 'optional' arguments to truly be optional, similar to #2045847: The provided argument for parameter is empty and #2109963: Option to exclude empty elements from request body but now I see this behemoth looming here.

This rewrite is ... huge. Though all good I'm sure, and looks well-reviewed by everyone.

Klausi, apart from the important things in the docblocks like http vs HTTP - (which is obviously a release blocker) do I understand that you endorse this whole thing? Should I try to get it in?
If this is to happen at all (and the theory looks good to me), then it's sorta blocking any other further work on REST services at all until it goes in. ... and what seemed to be an easy, the short-and-sweet authentication support fix over in #2138617: OAuth2 authentication for http_client now looks like a terrible conflict I've created etc.
:-(

1,2,3 docblock punctuation : done
4 removed the noted lines, I guess someone else should put it in later?
5. docblock added.
6. I'm not sure, but as there is no UI for it currently, any existing code that uses a custom formatter already is probably calling it from code? As this checks update checks empty() before making any change to any remembered settings, I think it's probably OK. But I'm not sure of the use-case you are describing, so I can't see how to create a test case where this would fail. Leaving it as is.
7. If

 * @param array $settings

is unacceptable (it helps my IDE enough), and the choice is therefore between

 * @param array $settings
 *  An array of settings

or to have no annotation at all - then we can can follow the examples set in wsclient.module instead.
So, fixed.
8. fixed.
9. I can't answer that. unchanged.

10 - renamed serializer and parser as requested.

Re-Rolled on top of the current dev
(needed re-merge over a minor authentication issue that doesn't impact this one ... once the merge conflict (function jumped to a different file) was sorted #2138617: OAuth2 authentication for http_client )

So - all of Klausis requests apart from #9 that I can't do and doesn;t seem to stop it working are here now...

So far it still holds together OK (is not broken) but I don't have services to test all the new functionality on.

note, for testing this to update an existing system you need to run update.php, and (it seems) clear cache.

The problem with incorrect undefined (optional) parameters being sent is still a fatal flaw for the APIs (Google services) I'm trying to plug in to, and I need this stable before I can re-roll the fixes for that.

Golly. I find myself back at this issue again, on a different project this time after struggling through the conversion of a GET service to a POST service, and discovering that I needed to switch from the default JSONFormatter to the nice HTTPClientCompositeFormatter that lets me POST a query and get JSON back. goody.

But still, this can only be done in code, and lacks this UI exposure. Even if I *want* to set the formatter in code today, It still needs to be on the service description screen for diagnostics.

Re-running this all today, on a different task, I can re-confirm that
* the patch works great, the UI is there and the choices made do take effect.
* It fixes another issue I had with POST data not being transferred to the REQUEST body(and had my own patch to fix, but this one makes it redundant!)
* The 'upgrade' path/legacy support transition here works exactly as needed - it did not break an old-style WSClient service definition, and just improved it.

It's pretty awesome, and certain bits of what we would expect from the REST client (like being able to use POST method at all are not working without it.
I'm really keen to push this in.

Hello, I am having a hard time applying this patch to 7.x-1.x-dev. Is that the correct version?

The error I am getting is:

Patching file 'wsclient_rest.api.php'
Assertion failed, hunk, file patch.c line 321

I have applyed other patched in the past without issue. Is there something special about this?

BTW, that file wsclient_rest.api.php doesn't exist in my copy of 7.x-1.x-dev.

Any thoughts would be greatly appreciated.

Thanks

Not sure.
I tried a re-run, seems OK.

  cd sites/all/modules/patched/wsclient
  git checkout -- .
  git pull origin 7.x-1.x
  wget https://drupal.org/files/issues/wsclient-1280332-rest_formatter_ui-22.patch
  patch -p1 < wsclient-1280332-rest_formatter_ui-22.patch 

ran clean for me.
Yes, it's a big patch and adds some re-architecturing - including the creation of the wsclient_rest.api.php file to hold the now-more-structured code.

Thanks for your response.
Ok, I noticed that you get the module from git before you apply the patch, whereas I was simply downloading the module from the modules homepage then applying the patch via unixtools in windows.

Is there a difference in the module when retrieved from git rather then downloaded via the webpage?

Thanks.
Aaron

@arm75
Depends on timing, but usually they are the same within an hour (or a day for sandboxes IIRC)
today, the 7.x-1.x-dev tarball says 2014-Jan-10
which matches with the repository history, also with the last change at 2014-01-10 http://drupalcode.org/project/wsclient.git

So no, they *should* not be different.
However, as a matter of course, if you are testing a patch, the much more common workflow would be to be using git primarily. It's not required, no, but generally common.

I'm now on my third independent project where this patch is needed on a *totally different web service* provided from another direction.
The above patch - that allows selection of JSON or XML as supported response formats for HTTP REST queries - is necessary, and amazingly still applies clean! (though does need a cache clear when upgrading)

Still searching for an OPEN web service that will work for testing though... I just need someone to confirm that it doesn't seem to break existing functions. I have found that it still works with all my wsclient_tester examples, though know that's not complete. Please test it out against your existing REST service to see it's not messing up anything unexpected.
Can I get a RTBC on this?

still applies clean

Thank you! This patch saved my life.

@Klausi - I'm happy to put this in, it's working in real world sites for me with no regression.
But due to the size of it, can I get a thumbs-up from you?
It's backwards-compatible with old behaviours, and (though it requires a cache-clear) doesn't seem to break anything I can test for.

I don't have a project where I use wsclient right now, so no real world scenario to test this.

Patch looks good otherwise.

Put that in then, to clear the decks and start looking seriously at some of the smaller issues that have been straggling.

NOTE: YOU WILL NEED TO CLEAR CACHE AFTER UPGRADING.

Hi,
I am on my first project with ws_client and using latest dev versions of both this module and http_client.
My webservice specification are briefly explained by example in attached WS-fulfillment.jpg illustration.
So i gets to basic HTTP authentication using XML formatting for both request as response.
I would like to trigger my webservices as much as posible by rules.

In attached WS-test-result.png, you can witness the webservice replied with a 400 Bad Request - The XML is not valid according to the XSD.
In the last dpm'nd variable $this->formatter (inside HttpClient.inc) it is clear the HttpCleintCompositeFormatter has been used instead of the by wsclient_rest.module instantiated WsclientRestXMLFormatter. (see dpm: settings-formatter)

Looking into wsclient_rest.inc, i believe to understand that on line 51 the evaluation on wsclient_rest_has_old_formatter should have returned true :

      if (wsclient_rest_has_old_formatter($settings)) {
        $formatter = new $settings['formatter']();
        $this->client = new HttpClient($authentication, $formatter, $this);
      }
      else {
        $formatters = array();
        foreach (array('send_formatter', 'receive_formatter') as $formatter_type) {
          // Settings could be missing.
          $formatter_settings = !empty($settings[$formatter_type]['settings'])
            ? $settings[$formatter_type]['settings']
            : array();
          $formatters[$formatter_type] = new $settings[$formatter_type]['class']($formatter_settings);
        }
        $formatter = new HttpClientCompositeFormatter($formatters['send_formatter'], $formatters['receive_formatter']);
        $this->client = new HttpClient($authentication, $formatter, $this);
      }
      // Pass through additional curl options.
      if (!empty($settings['curl options'])) {
        $this->client->options['curlopts'] = $settings['curl options'];
      }

In other words, how can i fully enable HttpClient's XML formatter without understanding the logic behind this legacy supporting has_old _formatter build in deviation ?

PS. I've also attached my WS description as my applied WS operation settings.
PS2. For the basic authentication, i wrote a custom module on hook_wsclient_service_load(), which seem to work.