Add support for controller per entity type

That's an interesting idea.

I think that if we're going to go down this route, I'd like to also have support for the Services Entity controller being set in the entity info, the same way Entity API sets various controllers.

I'd actually be inclined to have per-entity type controllers ONLY set in the entity info, and not extend the UI. But then I am perhaps not a typical use case, as I only use this module with my own custom entity types. For me, adding a that would be an extra line in hook_entity_info(). For users who use this with entity types defined by contrib modules, this would involve a custom module with hook_entity_info_alter() just to set the controller, which would be faffy.

1. +++ b/services_entity.module
   @@ -61,6 +80,24 @@ function services_entity_get_resource_info($resource = NULL) {
   +function servics_entity_get_resource_controller($entity_type) {
   

   Typo in function name: servics instead of services.

2. +++ b/plugins/services_entity_resource.inc
   @@ -58,7 +58,6 @@ class ServicesEntityResourceController implements ServicesResourceControllerInte
   -    services_remove_user_data($entity);
   

   This looks like an unrelated change.

It's funny - I was going to post a similar issue as soon as we get some of the pending patches committed.

I agree with @joachim - this should be in hook_entity_info(). It seems to me the primary use case would be writing a controller that does something entity-specific, in which case there would be no point in offering a choice via the UI. Further, you'd have to write the module to implement the controller anyway, so it wouldn't be any harder to implement hook_entity_info_alter() than to implement hook_services_entity_resource_info().

@mhrabovcin - can you provide a use case for giving a UI to select resource controller per entity?

I also think that individual resource controllers should be able to define the operations (and actions) they implement -- so I propose adding a ServicesResourceControllerInterface::resources() method, which would essentially implement hook_services_resources_info() on behalf of that controller. This would allow (among other things), adding custom arguments and actions for specific entities in a clean, object-oriented way. But perhaps that's a different issue?

@mhrabovcin - That use-case makes sense, but I still think it's better to keep it out of the UI -- as you say this is a developer focused module, and choosing a resource controller class isn't really a site-admin function.

This patch takes your idea and goes a step further along the lines outlined in #3. I think it's an improvement on the basic architecture, and will make it much easier to add custom arguments, actions, relationships, etc. when implementing custom controllers. It's a bit of a refactor, but doesn't change anything about how the resource controllers themselves work. It just makes the way they are invoked a bit more flexible and generic.

@joachim - what do you think?

Sorry for the silence from me -- I was taking a break of the holidays, and now I'm snowed under with work stuff :(

The general approach of this patch looks good, though there are a few things I really wasn't sure of when I looked at it a couple of weeks ago:

- the services callbacks are getting moved to the module file. can't they stay in the .inc file? They also seem to be being changed to be more generic. That's going to make debugging and maintenance harder.
- lots of unrelated changes like spacing in the readme

I think it would be a lot easier to tackle this in several issues with smaller patches -- one to introduce the controller set in the entity info, and then one to rework controllers and refactor them. As such, this patch is a bit overwhelming, sorry!

I moved the service callbacks to the module file bc they are now comprise only about 20 lines of code -- it seemed unnecessary to break them out into a separate file -- but we can do so, if necessary.

The changing of the service callbacks to be more generic is a core part of the refactor - it makes it possible for controllers to define new methods and new method signatures for actions (e.g. to add a "revision_id" parameter to GET requests, or handle paging of index queries differently, or provide some level of recusion into related resources) without having to override the resource callbacks. All the work is done in the controller - the resource callback simply passes the method and argument on.

I rewrote the readme to describe the custom controller stuff. I was adding hard-wrap to the new lines, and thought I'd add it to the existing text as well for consistency - but we can separate that out into a diffferent commit if you think it necessary.

If you really think it's necessary to split this into two issues, we can do so (I will try to find the time) - but I think the refactor is related to the custom controller issue in that it wouldn't have much utility wihtout it, and it makes it possible to do much more with your custom controllers.

I think I've got my head round this.

I see what you're doing with the abstracted service callbacks. It's saving the very repetitive stuff we have at the moment:

/**
 * Services resource callback.
 */
function _services_entity_resource_create($entity_type, $values) {
  $resourceclass = variable_get('services_entity_resource_class', 'ServicesEntityResourceController');
  $resource = new $resourceclass;
  return $resource->create($entity_type, $values);
}

/**
 * Services resource callback.
 */
function _services_entity_resource_retrieve($entity_type, $entity_key, $fields) {
  $resourceclass = variable_get('services_entity_resource_class', 'ServicesEntityResourceController');
  $resource = new $resourceclass;
  return $resource->retrieve($entity_type, $entity_key, $fields);
}

...and so on.

It is worth noting though that call_user_func_array() is pretty slow. I suppose it's just two uses of that (access and execution), whereas in hook invocations there's potentially dozens or hundreds of them...

I would like to keep those Services hooks in those inc files. It's still better for performance, and it's also better DX, as that's where developers will be expecting to find them.

I'll make an updated patch soon.

I'll fix #6, and also the variables used in t() (existing problem, separate issue: #2174293: variables used in t()), and a few typos I've spotted :)

1. +++ b/plugins/services_entity_abstract.inc
   @@ -0,0 +1,224 @@
   +  protected function getInfoElement($entity_type, $method, $id_arg, $values_arg) {
   ...
   +      'access arguments' => array(TRUE),
   

   Instead of this workaround, we could just pass in to getInfoElement() the string for the entity_access() $op.

2. +++ b/services_entity.module
   @@ -141,3 +155,72 @@ function services_entity_entity_query_alter($query) {
   +  // We need to translate the "retrieve" method name to "view" in order
   +  // to pass it on directly to entity_access(). For all other operations
   +  // we can just use the method name.
   +  $op = $op === 'retrieve' ? 'view' : $op;
   

   ... then we wouldn't need this special case here.

Let's see what the testbot says for this.

I still have typos and things to fix.

There's a sequential patch from my local git branch too, so you can see what I've changed bit by bit.

It's occurred to me that there's a potential problem here with orthogonality.

The original goal of this issue was to choose whether you want the regular or clean controller per entity type.

It's always been possible to extend the controllers for specialized behaviour, but up till now this would be across the board, for all entities.

Now we making it easier to add custom services to controllers. We're introducing the idea that you might want to subclass the controller to make a specific controller for entity type Foo.

Here's a good example of that: #2174269: user register / create service can't be used if site has CAPTCHA.

It's been suggested over on that issue that Services could have an extra 'create_raw' method. An alternative to that would be to have user entity services that go via that API rather than forms in this module.

Because users are a bit special, we'd need the login, logout, register, etc methods adding.

That's easy: we subclass the controller to make a ServicesEntityUserResourceController, which has those extra methods. Great!

Except... we actually have to have TWO user classes: one that inherits from ServicesEntityResourceController, and one that inherits from ServicesEntityResourceControllerClean.

One the one hand, the controllers control the *processing* of the input and output. And we've now extended their role so that they control the actual services on a per-entity basis.

I am wondering whether we should come at this from another angle, and have controllers that provide the services and access, and can be subclassed for entity types, and have the regular/clean handled by another class hierarchy, say, 'processors'. But I also think that could turn into horrendous over-engineering!

Seeing as the testbot likes the sequential patch, here's the new version just in that format.

I've moved Services hooks and callbacks back to .inc files.

Hopefully this will get tested against the new tests I've added in #2174533: add access to tests.

This UI string needs to be updated:

"Select which Resource Controller to use to process the entities.".

I've been thinking over my comment in #12, and I think I was overthinking things there! This may be a problem that comes back, but I think for now we can leave it.

Hey, wow, sorry I have been buried. I will look at all your comments and patches tomorrow. Thanks so much for taking this up!

re #12 - I see your point, and in fact I think that's the way RESTws does it, separate controller and formatter classes. I like that architecture, but it would be a big project to implement for this module. Also, I think the differences between the "clean" and "generic" controllers are not just format-related (and will be even less so once we remove the stripping of "field_" from property names). The "clean" controller exposes only properties defined by the entity metadata API, so the actual data each returns is different.

re #10 - very clever, I like the idea. But I think it's a bit misleading/restrictive to name that $entity_access_op. Let's say you define a new method for a relationship or targeted action. You may want to implement your own access routine which piggybacks or even ignores the standard entity_access() rules. For example, a user "cancel" method would want to check for either "cancel account" or "administer users" permission rather than calling entity_user_access(). So I've changed the naming and the docs a bit, and also swapped the order of the arguments so that $access_op can default to the method name.

Otherwise, everything else looks great. You're absolutely right about separate .services.inc and .resources.inc files.

Looks great!

All my custom module tests pass with it (once I've removed the workarounds I had before for 'create' services...).

Committing this, with just one change as per comment #14:

    '#title' => 'Default Entity Resource Class',
    '#description' => 'Select which Resource Controller to use to process the entities where the entity type does not define one.',

git commit -m "Issue #2159051 by wodenx, mhrabovcin, joachim: Added support for controller per entity type, refactored code to abstract base controller." --author="wodenx "

Now this is in, I'll take care of a few clean-up bugs I was holding off on, and make another alpha.

Awesome. What do you think is still remaining to block a beta?

I think #2112595: remove stripping of 'field_' prefix from clean controller and #1950244: remove services_entityreference_format_plural(). They're the only issues that change API.

Hello all,
I read most of the comments and got lost on how is it exactly one would enable an entity-specific controller. I can't find any documentation on the matter.

Should I implement hook_entity_info_alter() in a custom module?

Is there a specific hook I should use to alter/add/inherit from one of the controller classes?