Improve services_error() - make more consistent and standards compliant

I'm fully in favor of doing this if we can make it happen quickly.

On this same issue, are the messages (first parameter) supposed to be wrapped in t() ? In the services/resources include files, some uses of services_error() wrap the messages in t() while most do not... what is correct?

Error codes are not translated currently, and I think they probably should be run through the translation system unless someone can come up with a reason why they shouldn't. Should we sanitize these strings too? Right now they are not being sanitized, and we shouldn't consider an submissions coming through services as being trustworthy (for instance, in the node resource we have

return services_error('Node type "'. $node->type .'" does not exist.', 406);

Which could be all sorts of problems depending on where it gets echoed. We can clean those when we run them through t()

I think if we run everything through t() it will work.

OK so here is a patch that does the following:

- Pushes all errors through t()
- Properly tokenizes messages instead of just concatenating in data
- When we implode form_set_errors() arrays, consistently do it with '/n' (instead of sometimes doing it with ' ')
- Made many error messages more detailed and useful ('Comment @cid not found' instead of 'Comment not found')

It still doesn't address the original issue of how we use reason phrases and whether or not we should directly throw exceptions, but at least they are consistent.

voxpelli points out that using line returns in the messages will totally mess up http headers (where these are sometimes used) so I changed them to all use spaces.

Patch looks great, have not tested it but does this affect Tests?

OK I've committed this. Moving back to needs work so we can revisit for the issues voxpelli originally brought up

Tests get broken because of introduced

return services_error(implode(" "("\n", $errors), 406);

Fixed that in all resources.

Committed in 7.x please port to 6.x

Heres the port to d6

Fixed and committed

how do send the error message in the body when using ServicesException class, or Exception for that matter. Do we do it the standard PHP way with header("Status: 500 Server Error"); and then the body is just printed?

As long as headers are not set on the services level, it would all be fine.
A service should implement the setting of specific headers

In the case of the amfserver, http headers for errors should not be set since that is not the standard way of handling errors for the amf protocol, which is basically a protocol over the http protocol.
In the amfserver, the status codes (like 500, 401 etc) are not used for setting headers, though they can be used there for sending that info to the actionscript client.

I was running into a problem returning meaningful errors via a services call and I found this issue. Having all the errors from "form_get_errors" concatenated into one long string was not very useful to me.

I wrote a patch that adds an optional "data" parameter to the "services_error" function. Seems if this is part of the "ServicesException" constructor then you should be able to set it via "services_error".

I also added this to the calls of "services_error" that are results of form errors in resources . I am passing the resulting error array from "form_get_errors" to this arguments in "services_error".

I have also changed the way the REST server is handling exceptions throw from "services_controller_execute". I am catching "ServicesException" here, adding the headers as before but also passing errors from ServicesException::getData on to the formatter so that they can be displayed in the body.

this breaks a bunch of tests but i like where this is going for sure.

kylebrowning, I ran all the Services tests locally on this and they all passed for me but I am new to Drupal's testing. Should I be running all of the site's tests?

While I figure out the test issues I have made some changes to the patch.

I have added info about types of errors:

if ($errors = form_get_errors()) {
    return services_error(implode(" ", $errors), 406, array('form_errors' => $errors));
  }

I also the ability to alter the errors in the rest_server:

try {
    $server = new RESTServer();
    return $server->handle($canonical_path, $endpoint_path);
  }
  catch (Exception $e) {
    $server->handleException($e);
  }

In my case I need to alter the error output to the application calling the Rest Server expects.

tests are passing, this looks good.

I guess the alter was actually not in my last patch

-    $result = services_controller_execute($controller, $arguments);
+    try {
+      $result = services_controller_execute($controller, $arguments);
+    } catch (ServicesException $e) {
+      $errors = $this->handleException($e);
+      drupal_alter('rest_server_exectue_errors', $errors, $controller, $arguments);
+      $result = $errors;
+    }

Patch in #22 needs to be ported to 6.x