To be more consistent with the way the node services work, the services should accept the entity type as the name of the entity value instead of "values as well as accept an array instead of a struct.

For example:

(See lines 24 in services_entity.services.inc)

          array(
            'name' => 'values',
            'optional' => FALSE,
            'source' => 'data',
            'description' => 'The values used to create the entity',
            'type' => 'struct',
          ),

Preferably be written as

          array(
            'name' => $entity_type,
            'optional' => FALSE,
            'source' => 'data',
            'description' => 'The values used to create the entity',
            'type' => 'array',
          ),

There are a few places where this would need to be changed.

This would also allow this module to work with the Backbone module for entities in general (see the backbone_services module sub module in the project). It would also allow them to be used with existing modules which load other entity types in Drupal (e.g. node, user, taxonomy).

CommentFileSizeAuthor
#2 1867238-2-services_entity-backbone-compat.patch2.15 KBspotzero
#1 1867238-services_entity.patch2.33 KBminoroffense

Comments

minoroffense’s picture

minoroffense commented
StatusFileSize
new 1867238-services_entity.patch2.33 KB

Here's a patch which has the necessary changes. Going to be doing some testing this afternoon to see of any negative affects.

spotzero’s picture

spotzero commented
Title: Change entity callbacks to better match existing node services configuration » Change entity callbacks to be compatible with backbone
StatusFileSize
new 1867238-2-services_entity-backbone-compat.patch2.15 KB

This patch fixes issues with the previous patch, and works with backbone.

minoroffense’s picture

minoroffense commented
Status: Active » Needs review
joachim’s picture

joachim commented
Status: Needs review » Needs work

Hmmm... I don't actually like the parameter being variable, as it makes each 'create' resource different, and therefore adds to complexity.

Ideally, I'd change it to 'entity' for everything, which is clearer than 'values'.

But Services module does this too:

        'create' => array(
          'file' => array('type' => 'inc', 'module' => 'services', 'name' => 'resources/node_resource'),
          'callback' => '_node_resource_create',
          'args' => array(
            array(
              'name' => 'node',
              'optional' => FALSE,
              'source' => 'data',
              'description' => 'The node data to create',
              'type' => 'array',
            ),
          ),
          'access callback' => '_node_resource_access',
          'access arguments' => array('create'),
          'access arguments append' => TRUE,
        ),

And I guess, as much as I don't like it, we should try to stay consistent with Services core, when it makes sense.

One thing though:

+++ b/plugins/services_entity_resource.inc
@@ -27,6 +27,7 @@ class ServicesEntityResourceController implements ServicesResourceControllerInte
+    $values = _services_arg_value($values, $entity_type);

@@ -43,6 +44,7 @@ class ServicesEntityResourceController implements ServicesResourceControllerInte
+    $values = _services_arg_value($values, $entity_type);

Could you explain why we need these?

minoroffense’s picture

minoroffense commented

Those two lines are to extract the actual values from the passed element.

See services.runtime.inc


/**
 * Extract arguments for a services method callback, preserving backwards compatibility with #1083242.
 *
 * @param array $data
 *  original argument passed to a resource method callback
 * @param string $field
 *  name of the field where arguments should be checked for
 * @return array
 */

// Adds backwards compatability with regression fixed in #1083242
function _services_arg_value($data, $field) {
  if (isset($data[$field]) && count($data) == 1 && is_array($data[$field])) {
    return $data[$field];
  }
  return $data;
}

Otherwise when Services goes to parse the data, it's in an unexpected format (i.e. an array instead of the object itself). You can comment that out to see the difference. In our case, it goes and finds the entity object keyed by its type within the values array.

I realize calling a function which is marked internal isn't ideal, but arguably it's better than copy/pasting the function into services_entity. Could open an issue with Services to have the function signature changed.

joachim’s picture

joachim commented

It's also a function whose docs appear to state that it's only there for backwards compatibility, which is a bit worrying.

joachim’s picture

joachim commented

Okay, having now read #1083242: Fix for argument source handling in REST Server, we should definitely NOT be using that.

And we should have it that you don't need to post data with a sub-array, because that's the old behaviour which that helper function is there to support.

This comment explains it more: http://drupal.org/node/1083242#comment-4503570

joachim’s picture

joachim commented

Hence to create a new node, you post:

{
"type":"article",
"title":"This is my new article ",
"language":"und",
"body":{"und":{"0":{"value":"This is the body of my node"}}}
}

and not:

{"node":{
"type":"article",
"title":"This is my new article ",
"language":"und",
"body":{"und":{"0":{"value":"This is the body of my node"}}}
}}
joachim’s picture

joachim commented

(well, you do now I've just found and fixed #1960108: 'create' resource is broken for core entities!!)

wodenx’s picture

wodenx commented

@joachim - So it looks like this should be closed as "won't fix" -- am I right?
The name of the argument is more or less irrelevant if it's not used in the post data.

joachim’s picture

joachim commented
Status: Needs work » Closed (won't fix)

Yup,