Clean Processor not listing entities with custom bundles

Looks good. Let's see what the testbot says.

Though I do think this could be considered a bug in ECK. This is what entity_get_info() does:

        // If no bundle key is provided, assume a single bundle, named after
        // the entity type.
        if (empty($entity_info[$name]['entity keys']['bundle']) && empty($entity_info[$name]['bundles'])) {
          $entity_info[$name]['bundles'] = array($name => array('label' => $entity_info[$name]['label']));
        }

so I would expect ECK to set a bundle key and therefore trigger this behaviour.

Turns out our tests are broken :(

index 1e34c53..c53d1be 100644
--- a/docroot/profiles/whistlepunk/modules/contrib/services_entity/plugins/services_entity_resource_clean.inc

Could you roll this from the module folder please?

+++ b/docroot/profiles/whistlepunk/modules/contrib/services_entity/plugins/services_entity_resource_clean.inc
@@ -175,7 +175,9 @@ class ServicesEntityResourceControllerClean extends ServicesEntityResourceContro
+    if (isset($entity_info['bundle keys'])
+      && (count($entity_info['bundles']) == 1 && key(reset($entity_info['bundles'])) == $entity_type)) {

There are coding standards being debated for this: #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines. Those would call for extra indentaion. Or you could put it all on one line. Either is fine with me :)

Attached is the patch re-rolled. I think I did it right.

edit: I just re-rolled to the module folder... not the second half of the post above.

I think there's a bigger problem in this code that we should fix here:

    // Default the bundle to the entity_type. This allows eck types that don't use custom bundles to not have to specify the bundle.
    $entity_info = entity_get_info($entity_type);
    if (isset($entity_info['bundle keys'])) {
      foreach ($entity_info['bundle keys'] as $bundle_key) {
        if (!array_key_exists($bundle_key, $values)) {
          $values[$bundle_key] = $entity_type;
        }

Compare with http://api.drupal.org/api/drupal/modules!system!system.api.php/function/...

- There should be only ONE entry in $entity_info['bundle keys']. Iterating over that is pointless.
- Much more major, the key name that is given by $entity_info['bundle keys']['bundle'] has **NO** business being present in entity values:

bundle: The name of the property of the bundle object that contains the name of the bundle object.

That means that it is the name of a key on something such as a 'node type' object. Nothing to do with what we have here.

I *suspect* what is really meant here is ['entity keys']['bundle'], but as I don't use the clean processor, I don't know.

$bundle_key has still absolutely no business being set in $values.

It's like saying that you have $node_type->foobar and therefore you expect $node->foobar. The two are simply totally unconnected.

I'm not entirely clear on what this function as a whole is meant to do. It's getting called for create, update, and index. Index won't need a bundle setting: if there is only one bundle, that's all you get. It's fairly likely that creating an entity that defines a bundle key will need this set: all sorts of thing in Entity API crash without a bundle key set.

This code is not going about it the right way though.

I agree with nhepner1, this code looks like it's compensating for something in ECK. If someone wants to request a specific type, they should do so explicitly. Here is another patch nuking that code all together.

setting to needs review

+++ b/plugins/services_entity_resource_clean.inc
@@ -192,16 +192,6 @@ class ServicesEntityResourceControllerClean extends ServicesEntityResourceContro
-    // Default the bundle to the entity_type. This allows eck types that don't use custom bundles to not have to specify the bundle.

I don't mind this code being there. It's nice DX sugar.

It just needs to be coded correctly! :)

The right logic is:

- is there only one bundle?
- get the bundle key (NOT the 'bundles keyS'!!)
- set it to the entity type (or, to be really sure, pop off the first and only key of the bundles array)

Attached patch is a revised version of that posted at #2068463: Improper use of 'bundle keys' in "clean" controller..

It implements the logic described in #13. Further, it removes this logic from transform_values() and puts it into create(). This is for 2 reasons:
1. We need to have the wrapped entity in order to check the bundle key, since the metadata property name might differ from the bundle key (e.g., for taxonomy, "vocabulary" vs "vocabulary_machine_name"), so we can't just check for it in the provided values.
2. transform_values() is called in 3 places - on "create", "update" or "index" -- for "index" we certainly don't want to but an invisible filter by bundle into the query (this was causing a related issue at [#1890068}, and for "update" we probably don't want to be changing the bundle anyway.

+++ b/plugins/services_entity_resource_clean.inc
@@ -193,16 +203,6 @@ class ServicesEntityResourceControllerClean extends ServicesEntityResourceContro
-    // Default the bundle to the entity_type. This allows eck types that don't use custom bundles to not have to specify the bundle.

Could we get this info back in please?

Please see also #1890068: "No entities found." on index call. This patch should resolve that issue as well.

re #17 - well, we do have:

          // If the entity supports only a single bundle, then use that as a
          // default. This allows creation of such entities without specifying
          // the bundle.
          $entity->{$bundle_key} = reset($entity_info['bundles']);

Do you think we have to mention eck explicitly? This little bit allows any single-bundle entity to be created this way, not just eck.

> This little bit allows any single-bundle entity to be created this way, not just eck.

Most single-bundled entities don't have a bundle key, so it's an ECK peculiarity I think!

We can word is so it's '... such as eck'. But it's nice to have some justification for what will look odd in future when we've forgotten about this.

Made the wording change above and committed, along with a test which fails without the patch.