I'm in search of the correct format a server should use for the arguments array.

I noticed the use case seems to differ from resource to resource,
certain ones work with an unnamed array:
$args[0] = value
others require a named array:
$args[key] = value

as amfserver handles them inconsistently, certain resource fail…
(services_views module only works with named arrays it seems)
(resources as node_resource seems to manipulate them further to eg. $args[0]->arg in the access callback, which is even more confusion)

so, what is the standard format of the arguments which should be fetched to a resource? is the named array the correct one?

Comments

g10’s picture

g10 commented
Title: Correct arguments array format? » Uniform service arguments declaration
Assigned: Unassigned » g10
Category: support » task

After testing the services module w/ d7 (+ services_views, services_menu and amf_server) I noticed an inconsistency on how the arguments are declared in resources, and how they are handled… this, in combination w/ a faulty implementation in the amf_server, resulted in some bugs.

I would like to propose a change related to the arguments array handling of a resource:

this by providing a method in the services.module which:
- serialize the arguments in an associative array
- validates if all required arguments are present (if not, throws a services_error)
- adds the default value of optional argument, if no value is provided

this method would be called in the services_controller_execute, before checking authentication (as authentication uses the arguments).

The args array in the declaration of a resource would be able to be declared in both ways: an associative/named array OR an indexed array

indexed array:
current implementation in services module for resources:

…
'args' => array(
      array(
        'name' => 'view_name',
        …
      ),
      array(
        'name' => 'display_id',
        …
      ),
…

associative array:
the services_views implementation (and proposed for services_menu)

…
'args' => array(
      'view_name' => array(
        'name' => 'view_name',
        …
      ),
      'display_id' => array(
        'name' => 'display_id',
        …
      ),
…

Benefits:
- server implementations don't have to worry about serialization and validation of arguments (no repeated code/functionality, responsibility is with the core services module who executes a resource)
- arguments are passed in a consistent way to the access callbacks of the resources (as one method handles all the calls in the same way, opposed to each server having another implementation, and as a result be prone to bugs)

As I've only recently switched to the 3.x branch, I'm still not 100% grasping the mindset and the intended structure. As a result my first thought was that there was an issue with the services_views module ( #1167354: $args empty in access callback ), after identifying a bug in the amf_server ( #1218070: correct argument parsing ) I assumed the services_views module was working correctly ( although, services_views declares the args array with an associative array, opposed to an indexed array as the services module ). And finally by optimizing the services_menu module ( #1222098: Invitation to merge with branch on GitHub ) I realized the issue is simply with the fact that there is no consensus on how a server implementation should serialize the passed arguments.

As I'm not testing on the rest_server, any feedback/insights on possible implications are welcome.

kylebrowning’s picture

kylebrowning commented
Status: Active » Postponed

Im not 100% sure how I feel adding this to services.module, but right now its up to the server to decide how the arguments are processed and worked out and we kind of like it that way.

REST server handles it like this.

<?php
  /**
   * Parses controller arguments from request
   *
   * @param array $controller
   *  The controller definition
   * @param string $path
   * @param string $method
   *  The method used to make the request
   * @param array $sources
   *  An array of the sources used for getting the arguments for the call
   * @return void
   */
  private function getControllerArguments($controller, $path, $method, &$sources=array()) {
    // Get argument sources
    $parameters = $_GET;
    if (isset($parameters['_method'])) {
      unset($parameters['_method']);
    }

    $data = $this->parseRequest($method, $controller);
    drupal_alter('rest_server_request_parsed', $data, $controller);

    $defaults = array(
      'path' => $path,
      'param' => $parameters,
      'data' => $data,
    );
    foreach ($defaults as $key => $data) {
      if (!isset($sources[$key])) {
        $sources[$key] = $data;
      }
    }
    // Map source data to arguments.
    $arguments = array();
    if (isset($controller['args'])) {
      foreach ($controller['args'] as $i => $info) {
        // Fill in argument from source
        if (isset($info['source'])) {
          if (is_array($info['source'])) {
            list($source) = array_keys($info['source']);
            $key = $info['source'][$source];
            if (isset($sources[$source][$key])) {
              $arguments[$i] = $sources[$source][$key];
            }
          }
          else {
            if (isset($sources[$info['source']])) {
              $arguments[$i] = $sources[$info['source']];
            }
          }
          // Convert to array if argument expected to be array.
          if (isset($info['type']) && $info['type'] == 'array' && isset($arguments[$i])) {
            $arguments[$i] = (array)$arguments[$i];
          }
        }
        // When argument isn't set, insert default value if provided or
        // throw a exception if the argument isn't optional.
        if (!isset($arguments[$i])) {
          if (isset($info['default value'])) {
            $arguments[$i] = $info['default value'];
          }
          elseif (!isset($info['optional']) || !$info['optional']) {
            return services_error(t('Missing required argument @arg', array(
              '@arg' => $info['name'],
            )), 401);
          }
          else {
            $arguments[$i] = NULL;
          }
        }
      }
    }
    return $arguments;
  }

At any rate, if we do implement something like this into services.module instead of at the services level, its not happening anytime soon as we still don't have a 3.x release.

g10’s picture

g10 commented

Thanks for the feedback, I realize there is some server specific argument handling to be done…
From the point of view of amf_server, there is no path/param/data differentiation, which led me to believe all of the code for serialization would be duplicated in each module.

The only part which actually has to be duplicated (in each server implementation) is the default value / missing arguments check:


        // When argument isn't set, insert default value if provided or
        // throw a exception if the argument isn't optional.
        if (!isset($arguments[$i])) {
          if (isset($info['default value'])) {
            $arguments[$i] = $info['default value'];
          }
          elseif (!isset($info['optional']) || !$info['optional']) {
            return services_error(t('Missing required argument @arg', array(
              '@arg' => $info['name'],
            )), 401);
          }
          else {
            $arguments[$i] = NULL;
          }
        }

This functionality is separate from the argument serialization, and should be performed in the same way for each call (independent of server implementation)

Handling this would prevent server implementations to pass through an argument array in a wrong format,
and avoid resources to have to implement this kind of tricks:

function _node_resource_access($op = 'view', $args = array()) {
  // Make sure we have an object or this all fails, some servers can
  // mess up the types.
  if (is_array($args[0])) {
    $args[0] = (object) $args[0];
  }
  elseif (!is_array($args[0]) && !is_object($args[0])) {  //This is to determine if it is just a string happens on node/%NID
    $args[0] = (object)array('nid' => $args[0]);
  }
…

(additionally the arguments could be cast to their proper data type)

IMHO, this is the responsibility of the services.module (to at least enforce an uniform argument array to be passed by the servers).

g10’s picture

g10 commented
Status: Postponed » Closed (fixed)

After a closer look on how rest_server handles the arguments, I realize now that this should be handled by the servers themselves (the needs of the rest_server are too specific to have a general approach to this)
The patch in #1218070: correct argument parsing brings the argument handling in line with the rest_server implementation