provide a unified way to retrieve result entities

+1 for this, would be a great help.

(Also, my name has no underscore. :P)

This sounds in general like a good idea. It could be used for the plugin_row_node for example.

One thing i'm wondering is what should be done with entities in relationships.
For example the node author.

Additional this method would have to find out whether the current base table has entities at all, and return an empty array if it's not.

Additional this method would have to find out whether the current base table has entities at all, and return an empty array if it's not.

I'd think that a base table that doesn't contain entities won't have any handlers that would use them, anyways. Also, normal handlers would need some information as to what the entity ID field is – if that information isn't present, they'd have to assume that the results aren't entities.

For that, I think it would make sense to "officially" support a 'entity type' in the views-table definition (like we have done it for the linked issue), so the lookup of the entity-type is easy and query-backends can use that to map their tables to entity-types.

But that doesn't help query backends figure out the ID field, does it? (OK, they could then use entity_get_info() with the known type to find it out, but the field name isn't guaranteed to identically map to a databasecolumn, so couldn't be completely relied upon.)

I just realized the method, as proposed, wouldn't allow one to access the entity associated with a result row from within a field handler, would it? For this, we'd probably need a method that returns an entity given a $values result row.
In my opinion, this should be equally possible. Or is there no use case for having this unified across query backends?

(Also: Isn't this a feature request, not a task?)

Hm, looked at the code in the Entity API issue again, and I see you solve this by calling entity_id() on the result row to get the ID (and then use that to access the corresponding loaded entity).
While this should work in most cases, I'm not sure we can rely on this being present, and in the same property that the entity object uses, when we call this on a result row instead of an entity.
However, it should work for my use case (as I have the ID anyways), so that's just a remark on something to consider.

+      $info = entity_get_info($entity_type);
+      $id_key = $info['entity keys']['id'];
+      // Assume the id has been added using the default-alias.
+      $id_alias = !empty($relationship) ? $relationship . '_' . $id_key : $id_key;

I don't think this is safe, since we can't really guarantee that the alias was constructed this way. I guess in the vast majority of cases it will be, and I might be being a little too conservative here, but I think we either:

1) need to cycle through the fields array looking for our field and testing if field and table actually match
2) have add_field create a secondary array that allows you to look up aliases by table + field. Something like $this->field_aliases[$table][$field] = $alias;

2 is the most efficient, isn't really that impactful and could be useful in other ways later on.

One small thing I noticed:

+++ b/plugins/views_plugin_query_default.inc
@@ -1455,6 +1455,50 @@ class views_plugin_query_default extends views_plugin_query {
+          $base_table_alias = $relationship->alias;

$base_table_alias isn't used afterwards.

Otherwise this looks good to me, apart from the issue Earl mentioned.

Powered by Dreditor.

Hm, yes, that problem is a bit of a puzzler. I'd vote for b) – I always wondered why that method wasn't in the base query class. After all, the vast majority of backends will use such a method, and there are already some methods (or parameters) that will have to be ignored by some backends.

Ah, great that this got resolved!

+++ b/handlers/views_handler_field_entity.inc
@@ -0,0 +1,54 @@
+    if (get_class($this->query) != 'views_plugin_query_default') {
+      return;
+    }

This doesn't look like a good idea to me. If someone used just a slight extension to the default query class, this would "inexplicably" stop working. You should at least use instanceof – or, better still, just leave it out altogether. Query backends not supporting this just shouldn't use the field handler, right?
E.g., the maintainer of the Search API (whoever that is) might also want to use this, without overriding the method. At the very least, overriding should be made easier by factoring out the generic part of the method.

+++ b/handlers/views_handler_field_entity.inc
@@ -0,0 +1,54 @@
+  /**
+   * Load the entities for all fields that are about to be displayed.
+   */
+  function pre_render(&$values) {

Should probably be "all rows", not "all fields". At least that's what I'd think.

Maybe I can come up with some code in the Search API to test this, too. I just have to provide the get_result_entities() method in the query plugin and then extend the views_handler_field_entity class for the field handlers, right?

Powered by Dreditor.

ok, I've improved the patch such that views_handler_field_entity::query() is reusable and uses instanceof for its check.

Like this, I'll still have to copy the whole method, only changing that single line. Why not just completely remove the line, as I argued above? Or use method_exists(). Or move the check (or the rest of the code) to a helper method so others don't have to override the whole method.

Version preferred by me.

Oops.

Stop - that all should be possible without having to override the entity-handler - else you won't be able to re-use existing handlers like the node-edit links again.

Ah, you're right – that was the whole point, wasn't it? OK, then let's do it with method_exists(). I'd say, the chances of someone providing the method with a completely different purpose are really slim. And even then, they'd just have to take care of that in their field handlers and couldn't use them as-is.

#22 was right, too, of course, overlooked that.

Oh, yes. Knowing the entity type of a views table would be great (especially for use cases such as VBO).

Nice work I've not had a chance to look at it all fully yet but attached is the patch re-rolled against latest dev.

Quick thought: is this a candidate for a break; after we've found the relationship?

+    if (!empty($relationship)) {
+      foreach ($this->view->relationship as $current) {
+        if ($current->alias == $relationship) {
+          $base_table = $current->definition['base'];
+          $base_table_alias = $relationship;
+        }
+      }
+    }
+    $table_data = views_fetch_data($base_table);

+++ b/handlers/views_handler_field_entity.incundefined
@@ -0,0 +1,53 @@
+
+  protected $entity_type, $entities;
+  protected $table_alias, $base_field;

I'm wondering whether this stuff stuff should be protected, perhaps this kind of informations can be used on custom theming etc.

Changed this and moved table_alias to views_handler as it is used there as well.

+++ b/plugins/views_plugin_query_default.incundefined
@@ -1421,6 +1438,49 @@ class views_plugin_query_default extends views_plugin_query {
+    if (!empty($relationship)) {
+      foreach ($this->view->relationship as $current) {
+        if ($current->alias == $relationship) {
+          $base_table = $current->definition['base'];
+          $base_table_alias = $relationship;
+        }
+      }

This is a quite common pattern, so it might be possible to abstract this, but this is out of scope of this patch.

subscribing

Thanks for working on this big task!
Another step to move d7 stuff into views, as there are still parts which aren't supported yet.

Commited to 7.x-3.x

subscribing

FYI we should really start collecting API changes like this somehow for developers. I created a change record (http://drupal.org/node/add/changenotice) for this issue which can now be seen at the top of the issue.

Perhaps things like this should be added to the changelog.txt again.

Let's keep the changelog dead. Drupal.org now has facilities for this (that Dave Reid just showed us), so we can and should use that.