I'm currently looking into overhauling the Search API's Views integration, and especially the field handlers. And since those aren't in any way specific to the Search API (just being passed entities and rendering their fields in some way), I thought it could be worthwhile to implement those in the Entity API.

Related:
- #1077148-31: add an entity-view row plugin (the last patch for this – was mixed in with another issue that eventually got the upper hand)
- #1209678: Add a Views field handler to display rendered entities (one single field handler – could probably be used here, too, though)
- #1089758: Make use of new flexibility for Views field handlers (Search API issue – although most of this is outdated now; subscribe for new developments, though)
- #1231512: Use real Relationships instead of level magic in Views integration (other Search API issue, about relationships)
- #1172970: provide a unified way to retrieve result entities (fixed Views issue that help us here, I think)

My current plan looks approximately like this:
- Define new tables (not base tables, only for "joining" to – I think this is possible?) for all entity types. (Open: Even for types without property information?)
- Create field handlers (based on ones from Views probably) for all data types supported by the Entity API, and a special one for Field API fields. (Haven't looked into Field API internals at all yet. Is there an easy way to get formatters and their options for fields?)
- For all entity types, add field handlers for all their properties into their new table. For entity-typed properties we could maybe use the handler from #1209678: Add a Views field handler to display rendered entities.
- For entity-typed properties (and structures), add a relationship to the other entity table (or to a new table for that structure – not quite optimal, but I can't see a better solution :-/). No idea at all how this is done, yet – I haven't even ever used relationships in the UI yet, let alone coded them.

As you might have gathered by now, I'm not terribly good with Views (especially with relationships and everything related to them), so I'd very much appreciate feedback (and further ideas) from people who understand more about this. Is what I described feasible? Does it make sense?
(And if you're really good with Views, you can help me support OR groups with my filters – but that's another story. ;))

Anyways, the tables could then be joined to (e.g., by my Search API base tables), and the field handlers also re-used for other purposes. Sounds reasonable to me.
I just hope the Views 3 documentation is in better shape than last time I looked. :-/

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

FileSize
82.57 KB

drunken monkey and me have discussed that issue a bit. Attached is a foto of the "very nice" drawing we made:

Idea:
* The entity API provides a table per entity-type which contains views-fields working solely with entity-objects, based on the metadata.
* Others (Search API, or a possible efq backend) would have to create their own base-tables but can re-use those base-tables for relationships to related entities.
* For the main base table others could incorporate the table provided by the entity API and add their stuff (filters, sorts, arguments).
* Generic entity views-fields that should be added to each entity-type related table (green on the drawing) could be added in via a separate hook. Ideally this hook would make it into Views so those fields are automatically available in the regular base tables too. Thus, I've created #1270890: provide a way to provide entity-related views fields for handling generic-views-fields (green on the drawing)

drunken monkey’s picture

* Others (Search API, or a possible efq backend) would have to create their own base-tables but can re-use those base-tables for relationships to related entities.

Clarification: Since the entity tables lack a query class, they wouldn't really be base tables. (Which is also good because we'd otherwise get two tables for each entity type in the „Add view“ dialog, confusing the shit out of users.)
Only problem there is that the entity type is currently defined in the ['table']['base'] array, thus making it impossible (I think) to define the entity type of non-base tables. This would have to be somehow solved in Views, I guess.

Other than that I think we have most of the problems figured out, only relationships turn out to become even uglier than expected. Anyways, I should have a first draft soon.

drunken monkey’s picture

Added a sandbox for development on this issue. (Congratulations fago, you are maintaining another module now!) Please keep discussions in this issue for now, though, and don't use the other issue queue.

marcoka’s picture

subscribing

drunken monkey’s picture

The code in the sandbox is now updated to my latest development status. Things are far from working, but at least I already figured some things out and was able to remove a few bugs that were even in this early version.
I'm now also starting Search API integration in parallel to this issue, see #1231512-9: Use real Relationships instead of level magic in Views integration for my current status. Actually, it would probably be best if this was tested by at least two third-party implementations before it is committed, but I'm not sure where we'd get the second one.
Anyways, already ran into a few problems regarding that, but nothing insurmountable thus far. Will probably allow a magic get_result_wrappers() method on query plugins to circumvent get_result_entities() in the case of non-entity data (paradox use case for an Entity API, but still …).

Pedro Lozano’s picture

Subscribing.

drunken monkey’s picture

@ fago: I remembered the main use case of the alter hook we discussed for the $field_handlers array: modules could add new handlers for specific entity types, to replace the default "entity" one. Otherwise they'd have to crawl through the whole definition and check for, e.g., taxonomy term references.
I'm OK either way, though. Don't remember however what exactly we in the end agreed on regarding the hooks for this, or if we even need any?

Current status (see repo): Rather good, more and more things are working.
Major TODOs (I know of):
- Lists
- Relationships

fago’s picture

ok, I did a first short review.

/**
 * Render a field as a numeric value
 *
 * Definition terms:
 * - float: If true this field contains a decimal value. If unset this field
 *          will be assumed to be integer.
 *
 * @ingroup views_field_handlers
 */
// Class contents are mostly copied from views_handler_field_entity.
class entity_handler_field_numeric extends views_handler_field_numeric {

I don't think we need to take over all the views-documentation of the handler, let's just say it's extending the views handler to what it does different, i.e. using data from the entity. Also the last comment line seems to be wrong, as this code is in the helper.
Then, all the handler class and everything should be in the namespace entity_views, i.e. I think we should do entity_views_handler_field_numeric.

@EntityFieldHandlerHelper:
I think that form of code-reuse is the best we can do - still it'd expect it to live in the handlers directory too.

  public static function get_value($handler, $values, $field = NULL, $default = NULL) {
    $field = isset($field) ? $field : $handler->real_field;
    if (isset($values->$field)) {
      return $values->$field;
    }

I'm not sure about default to $values->$field if its there. There might be situations where it is there, but we shouldn't use it as the data is not accessed via the metadata wrapper. That way it might look different.

I know this is something safe to do for the search-api, but not in general. I don't know of a good way for you to implement it in the search-api then, maybe you have an idea? At least you could extend each class and override and customize get_value()... (I said I can't think of a *good* way.. ;)

drunken monkey’s picture

Done, done, done, done and … no idea how to do. But I'll guess I'll have to find a solution – I see your point, in any case.
However, there is no way I'll first implement the field handlers for you and then override it for myself anyways. Maybe a "magic property" (great, now that we're finally getting rid of the $row->entity magic property …) like _entity_fields, containing the fields that can safely be accessed this way?

Anyways, thanks for the review and feedback!

(Oh, and regarding the "entity_views" namespace: the row plugin doesn't use that either and I used that for orientiation.)

jakonore’s picture

subscribe

tnightingale’s picture

subscribe

drunken monkey’s picture

And another major leap forward. Most things now work. I haven't tested thoroughly, but the only thing I know of that isn't working yet is adding „official“ Views relationships. There I run into the problem that when I add a relationship and then a field based on that relationship, the field's handler doesn't get the relationship property set, it remains NULL.
Anyone knowing a bit about Views relationship handling that can help me out? I have my own relationship handler which does nothing, but looking at the Views default relationship handler it doesn't seem like it is the one resposible for setting this. So anyone got an idea where this might go wrong? Or can give me, or point me to, a basic tutorial on the thing (Views relationships from a developer's perspective)?

drunken monkey’s picture

OK, I think I got it figured out. Just fighting with one of my own functions now (entity_metadata_extract_property_multiple() – even typing the name is a fight), expect an "RC1" type of patch soon.

drunken monkey’s picture

Status: Active » Needs review
FileSize
57.81 KB

OK, and here it is! From some clicking around, everything seems to work fine, but of course I haven't clicked on everything. So please help test and review!

For testing purposes, you should probably use the Search API and the patch in #1231512-16: Use real Relationships instead of level magic in Views integration, which works with this. Otherwise, there's nothing to test, I think – this is purely an API change.

@ fago: I now just included the adaption to #1270982: only base-tables can be assigned to entity types here (so people won't wonder why the row plugin for Search API stops working, etc.).

marcoka’s picture

hm? i am interested in helping testing. does that patch add support to use normal views handlers like image_styles etc with search api? so i install both patches and start testing.

drunken monkey’s picture

Ah, yes, that's one of the things that are then possible. All Field API fields now use the proper field formatters, like the default Views field handlers. Thanks for reminding me, should notify the other issue …
You can't directly use a normal Views field handler with these tables, though (or at least not for all base tables / query plugins).

marcoka’s picture

i applied both patches with patch -p1 to the latest versions of entity and search_api.
When i create a view on node_search and try to add a field "Broken handler.. "
I am not sure what i did wrong? any tips?

marcoka’s picture

no more errors with latest search api patch. some mismatch with the entity.info but resolved that by hand (only some includes).

drunken monkey’s picture

Revised patch attached.

  • Fixes click-sorting. This previously relied on the orderby() method being present for the query object, and also passed wrong values to it. We now use a new method on the query plugin, if it exists, that we define.
  • Fixes some possible problems with nested fields. I really don't even try to claim that I have any idea what all of the field handlers are doing in what scenarios. But I think that previously they would use wrong field values in some cases, and this should now be fixed. Hopefully. Don't know. Please test!
  • Added an example query plugin to describe the methods we are using/expecting on a query plugin.
joachim’s picture

+++ b/views/handlers/entity_views_handler_relationship.inc
@@ -0,0 +1,21 @@
+class entity_views_handler_relationship extends views_handler_relationship {
+
+  // @todo What the heck have we got to do here?
+

Heh ;) Here we have to do #1114454: Views integration: create an entity-entity relationship that limits by entity type if applicable, though given how big this patch is already, we should probably focus on getting this in first, and then look at making relationships more advanced.

joachim’s picture

Status: Needs review » Needs work
+++ b/views/entity.views.inc
@@ -27,15 +28,177 @@ function entity_views_data() {
+    if ($table) {
+      $data['entity_' . $type] = $table;
+    }

> - Define new tables (not base tables, only for "joining" to – I think this is possible?) for all entity types. (Open: Even for types without property information?)

I'm actually a bit confused about this part. I've only just seen this issue, so a bit late to the party. But what's the purpose of adding clone tables for things such as {users}? Won't that make it harder to work with what's already on the users table as Views sees it? Would it not be better to add extra things to the users table with hook_views_data_alter()?

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
62.02 KB

I'm actually a bit confused about this part. I've only just seen this issue, so a bit late to the party. But what's the purpose of adding clone tables for things such as {users}? Won't that make it harder to work with what's already on the users table as Views sees it? Would it not be better to add extra things to the users table with hook_views_data_alter()?

I don't think so. I'm in no way a Views expert so I could be totally wrong, but the problem I'd see with this is that we'd then have handlers for two totally different backends in that table. While the default field handlers from the users table are written for the default query plugin, and therefore (generally) won't work with other plugins, the Entity API field handlers are written in a way to work with all query plugins that are aware of entities and use them at least partly in what they do. Therefore, the latter can perfectly be used by other query plugins, while things would utterly blow up if using the former (unless your query plugin is very similar to Views' default one).
This is the whole reason for these handlers in the first place, so adding them to the existing Views tables really wouldn't make any sense (again, unless I'm mistaken).

Attached patch fixes the relationship handler documentation a bit, as per #20. And I agree, advanced stuff (especially if it won't need support from the query plugins) can wait until this patch gets in.

joachim’s picture

Status: Needs review » Needs work

I'm fairly knowledgeable on the Views API, though I'm not familiar with query plugins or what how different backends work, so I don't know what the aim of the entity_* tables is.

From the Views perspective, my concerns are:
- if you have users and entity_users, both providing the same fields, which one will the user pick in the admin UI?
- What if the user already has a view with user fields, and then adds fields from entity_user too? won't that mean the user table is joined to twice?
- though for that matter, I'm not seeing any of the fields you add showing as fields I can add in the UI. This seems to be because none of the entity_* table definitions give any join information, so they are not available for any base table.

> While the default field handlers from the users table are written for the default query plugin, and therefore (generally) won't work with other plugins, the Entity API field handlers are written in a way to work with all query plugins that are aware of entities and use them at least partly in what they do

Like I say, I don't know about query plugins. But if they're a system in Views 3, and the Views 3 handlers aren't able to work with plugins other than the default one, then that seems to me to be a problem to be fixed in Views itself.

drunken monkey’s picture

Status: Needs work » Needs review

Please don't change the status just because you have questions!

OK, I hope I'll be able to explain the purpose of this issue better.
In Views 3, every base table you define has to have a query plugin assigned to it. This is responsible for building and executing the query and returning the results. Views itself provides a default query plugin that does this with a simple database query, as usual, which can be used for most normal purposes. However, with query plugins, you are not restricted to getting results from the database, but can literally retrieve them from anywhere you want, as long as you follow the basic structure Views defines.

However, as query plugins are (or can be) different to one another, tables with different query plugins cannot usually be joined with each other. Likewise, as field/filter/argument/etc. handlers have to understand the basic structure of the query mechanism, those handlers usually are specific to one kind of query plugin. As both handlers and the query plugin are set on the table, this is in principle no problem – the one writing the Views table definitions just has to keep in mind to only use handlers that are compatible to the query plugin. Of course, being able to use those handlers with all query plugins would be great – but even if it is possible (I don't know, it might) without losing a lot of flexibility, it would require a major rewrite of Views. So while it would be cool to „fix“ this directly in Views, we really don't have a chance to do so in Views 3 and thus have to work with this for now.

So, handlers have to be written specific to query plugins in general. However, contrary to filter/argument/sort/etc. handlers, there is a good chance to provide generic field handlers: as there are several applications in which query plugins will work by retrieving entities as their results, the field handlers there don't need to add any additional information to the query but just need a way to retrieve these entities in a unified way. By defining such an interface for query plugins, we can create generic field handlers for all kinds of data related to entities, as is done in this issue. We also don't need to add any query-time data for relationships, so those handlers can likewise be provided in a generic way.

So, what does this buy us? As you have pointed out, adding this data to the normal Views tables would be pointless and confusing. That no such join data is specified for them is therefore by design, rather than a flaw or oversight. The benefit of these tables and handlers is that other modules providing Views integration with new query plugins can just use these tables to build their own Views integration upon. As the diagram in #1 shows, they can copy the base table, add their own filters, …, and then use relationships to join to the entity_* tables for nested fields. This is possible regardless of the query plugin, as long as it uses entities (or entity metadata wrappers) and conforms to our small additional interface.
I already had such a system in the Search API. However, as a rebuild was needed anyways, fago urged me to put that in a generic way into the Entity API module, so other modules (e.g., efq_views) could use them, too. So, here they are.

If you want to see them in action, you'll have to try them out with the Search API. Otherwise, this patch doesn't do anything that is visible to users.

joachim’s picture

Thanks for taking the time to write such a thorough explanation! That really helps!

In which case, the link I posted earlier to #1114454: Views integration: create an entity-entity relationship that limits by entity type if applicable isn't related to this, as that issue is for relationships provided by the EntityDefaultViewsController class, which this patch doesn't touch -- so I'll carry on with that issue as I was planning to.

Also, might it be an idea to put part of your explanation in either the module's README or the documentation for entity_views_data()?

das-peter’s picture

subscribing

jsacksick’s picture

subscribing

drunken monkey’s picture

Discovered two small bugs in the previous patch:
- 'field_name' definition fields of Field API field handlers had wrong values for nested fields (as had relationship definitions, though it really doesn't matter at the moment for them).
- Those field handlers that hand off rendering to their parents had inconsistent (and non-working) code for dealing with list values. I actually forgot to implement that little part. Should work now, though it looks kinda ugly. Avoiding code duplication has really been a mess in this issue.

Revised patch attached.

marcoka’s picture

i exposed the page items. if you do that with a view that uses "node_search" the view will be broken and you get a WSOD
applied the latest patches of entity and, search api on both dev versions.

fago’s picture

Status: Needs review » Needs work

ok, I've done another review.

+++ b/entity.module
@@ -1095,6 +1095,41 @@ function entity_metadata_wrapper($type, $data = NULL, array $info = array()) {
+ * @see entity_metadata_wrapper()
...
+function entity_metadata_wrapper_multiple($type, array $objects = array(), array $info = array()) {
+  $wrappers = array();
+  foreach ($objects as $id => $object) {

Hm, I don't see why we need that function. Using entity_metadata_wrapper() with a list data type would achieve the same.

+++ b/entity.module
@@ -1113,6 +1148,172 @@ function entity_metadata_site_wrapper() {
+ */
+function entity_metadata_extract_property(EntityMetadataWrapper $wrapper, $property, array $options = array()) {
+  $ret = entity_metadata_extract_property_multiple(array($wrapper), $property, $options);
+  return reset($ret);
+}
...
+function entity_metadata_extract_property_multiple(array $wrappers, $property, array $options = array()) {
+  $options['identifier'] = TRUE;
+  if (!$wrappers) {

Let's make use of existing terminology and call this "data selector", thus it should take an $selector argument.

+++ b/entity.module
@@ -1113,6 +1148,172 @@ function entity_metadata_site_wrapper() {
+function entity_metadata_extract_property_multiple(array $wrappers, $property, array $options = array()) {
+  $options['identifier'] = TRUE;
+  if (!$wrappers) {

I'm not sure about introducing a apply-data selector-multiple function. First off, it's really complex. Then I can't think of many use-cases for that.

The multiple logic really only makes sense in the cases of of entity-relationships. So maybe, we could add it to the relationship class? So the relationship class has to take care about getting the entity object (which then can make use of multiple logic) and from there we are using single, simple data selectors?

+++ b/entity.module
@@ -1113,6 +1148,172 @@ function entity_metadata_site_wrapper() {
+      $ret = array();
...
+    $t = reset($wrappers)->$field->type();

Variable names have to make sense *always*. We should never use abbreviations like that.

+++ b/includes/entity.property.inc
@@ -42,6 +42,21 @@ function entity_get_property_info($entity_type = NULL) {
+ * @see hook_entity_property_info()

@see should be below @return.

+++ b/includes/entity.property.inc
@@ -311,6 +326,20 @@ function entity_property_list_extract_type($type) {
+function entity_property_list_inner_type($type) {

+++ b/views/entity.views.inc
@@ -27,15 +28,176 @@ function entity_views_data() {
+  $type = entity_property_list_inner_type($property['type']);

Do we really have to support mutiple nested lists here? Usually, we don't really care about that.

+++ b/views/entity.views.inc
@@ -27,15 +28,176 @@ function entity_views_data() {
+    if ($table) {
+      $data['entity_' . $type] = $table;
+    }
+  }

Let's roll a separate patch for that, as we can commit that asap.

+++ b/views/entity.views.inc
@@ -27,15 +28,176 @@ function entity_views_data() {
+ * @param array $property
+ *   The property information for which to create a field definition.
+ * @param array $table

Please always use $property_info if it's actually info. Else one could be easily confused.

+++ b/views/entity_views_example_query.php
@@ -0,0 +1,65 @@
+abstract class entity_views_axample_query extends views_plugin_query {

I'm not so happy with requiring a separate class people have to use. We could add an interface for describing further added methods we'll use though.

+++ b/views/entity_views_example_query.php
@@ -0,0 +1,65 @@
+   */
+  public function add_selector_orderby($selector, $order = 'ASC');

Wow, is that working? Sorting without any database?

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,344 @@
+// Class contents are mostly copied from views_handler_field_entity.

Wrong comment style.

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,344 @@
+    // The normal orderby() method for this usually won't work here. So we need
+    // query plugins to provide their own method for this.
+    if (method_exists($handler->query, 'add_selector_orderby')) {
+      $selector = self::construct_property_selector($handler, TRUE);
+      $handler->query->add_selector_orderby($selector, $order);
+    }

Why isn't it possible to implement the regular orderby() method?

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,344 @@
+      if ($v) {
+        $vs[] = $v;
+      }

Ouch.

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,344 @@
+      // We LOVE our core bugs! (http://drupal.org/node/1057242)
+      if ($handler->entity_type === 'file' && !is_array($url)) {

That's fixed.

joachim’s picture

+++ b/views/entity.views.inc
@@ -10,8 +10,9 @@
-    // Only enable the integration by default, it we know the module providing
+    // Only enable the integration by default if we know the module providing

Since the patch needs work anyway... can you remove this fix to comments please, as my patch at #1282506: documentation improvements fixes and expands that comment :)

drunken monkey’s picture

@ joachim: Done.

@ Wolfgang:

Hm, I don't see why we need that function. Using entity_metadata_wrapper() with a list data type would achieve the same.

Hadn't thought of that, but that does seem to work. On first impulse, having a ListWrapper instead of an array seems to me more error-prone, though. But as it seems to work, OK, let's do it that way.

Let's make use of existing terminology and call this "data selector", thus it should take an $selector argument.

Do you mean I should replace „property selector“ with „data selector“ everywhere, or only the two parameters you quote?

I'm not sure about introducing a apply-data selector-multiple function. First off, it's really complex. Then I can't think of many use-cases for that.

The multiple logic really only makes sense in the cases of of entity-relationships. So maybe, we could add it to the relationship class? So the relationship class has to take care about getting the entity object (which then can make use of multiple logic) and from there we are using single, simple data selectors?

Moving it to the relationship handler (apart from the fact that I then would have to find out where to put it there and how to best use it) would just do that: move it. It would stay as complex, and if we then only extract the entities and use „single, simple data selectors“ from there on (in the field handlers?), we'd have to add additional logic („What is the entity object for a given selector?“), plus copy three quarters of the function to another, second place.
The way to make this simpler would be to use single data selectors – but this would lead to (I think significant) performance loss, as we would load each entity separately (except for the initial results, maybe).

Do we really have to support mutiple nested lists here? Usually, we don't really care about that.

Well, I opt for correct code over short one. At least in the Search API this does happen occasionally, and I also thought of using this function instead of my own Search API variant once it's there.

Let's roll a separate patch for that, as we can commit that asap.

I don't really understand what you mean there. Did you mean to quote the same part as joachim in #31 ?

I'm not so happy with requiring a separate class people have to use. We could add an interface for describing further added methods we'll use though.

They aren't required to use it – on the contrary, they really can't. As the namespace and everything else should have suggested, this is simply an example class for documenting these methods. If you feel like they should be documented with an interface, I could of course replace these two words. I wouldn't, however, require people to implement it in any case. (First of all, because they'd then have to implement all three methods, not just one.)

Wow, is that working? Sorting without any database?

Er, yes? E.g., with Solr? And most other query plugins will have some kind of sort mechanism, too, I guess, so we give them the chance to provide click sorting with those.

Wrong comment style.

Why? This is just an implementation note, not a doc comment. Or do you mean something else?

Why isn't it possible to implement the regular orderby() method?

Because the regular orderby() method receives completely different (and misleading) parameters.

That's fixed.

No, it isn't. But I see your point, it really does look like it might be committed soon. So I guess we can risk to remove this. (We can then also close #1280420: Work around core bug in entity_uri(), I guess … Should've thought of that a year ago.)

drunken monkey’s picture

Hm, I don't see why we need that function. Using entity_metadata_wrapper() with a list data type would achieve the same.

Hadn't thought of that, but that does seem to work. On first impulse, having a ListWrapper instead of an array seems to me more error-prone, though. But as it seems to work, OK, let's do it that way.

Hm, no, doesn't really work that easily, after all. The main reasons are that you can't combine such list wrappers with + and that you can't (as far as I know) create a list wrapper from a (maybe nested) array of wrappers. Also, there is no easy NULL value as far as I can see.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
63 KB

Updated patch.

ZuluWarrior’s picture

Hi All,

Is there any update on this work?

I notice this latest patch is only a few hours old, but I though I'd give it a try. I've applied the entity patch too. Nothing seems to have changed and I can't add a relationship from my search view to the actual node fields (and formatters)

I'm currently writing a views gallery with a text search as an exposed filter so obviously this module is almost exactly what I'm after, but I need to display the image field in order to create a gallery. I know you've come across this request before.

I just wonder if search_api is going to be an adequate solution? As time has become of the essence.

fago’s picture

Do you mean I should replace „property selector“ with „data selector“ everywhere, or only the two parameters you quote?

Everywhere - the terms we use should be consistent.

Moving it to the relationship handler (apart from the fact that I then would have to find out where to put it there and how to best use it) would just do that: move it. It would stay as complex, and if we then only extract the entities and use „single, simple data selectors“ from there on (in the field handlers?), we'd have to add additional logic („What is the entity object for a given selector?“), plus copy three quarters of the function to another, second place.
The way to make this simpler would be to use single data selectors – but this would lead to (I think significant) performance loss, as we would load each entity separately (except for the initial results, maybe).

The "What's the next entity?" question would be easily solved by "ask the relationship handler for the entity" or if there is none, the query class. That's it.
Then, it remains to apply non-entity related data-selectors which can be applied using the single variant, for which a simple loop does it.

So we should not only move that function, but re-factor it into smaller, simpler parts. I dislike adding such monsters to the entity api.

Well, I opt for correct code over short one. At least in the Search API this does happen occasionally, and I also thought of using this function instead of my own Search API variant once it's there.

ok.

I don't really understand what you mean there. Did you mean to quote the same part as joachim in #31 ?

Let's roll a separate patch for the entity-type-key-location fix.

Er, yes? E.g., with Solr? And most other query plugins will have some kind of sort mechanism, too, I guess, so we give them the chance to provide click sorting with those.

Oh, of course. (In which case solr is our "database")

Why? This is just an implementation note, not a doc comment. Or do you mean something else?

Then it should be inside the function. Anyway, I think it's fine to have in the doc comment.

Hm, no, doesn't really work that easily, after all. The main reasons are that you can't combine such list wrappers with + and that you can't (as far as I know) create a list wrapper from a (maybe nested) array of wrappers. Also, there is no easy NULL value as far as I can see.

Still, the function does not make much sense to me. NULL should be perfectly fine in list-wrappers.

joachim’s picture

Could we maybe add something to the handler class names to indicate what they are for?

I've just realized that if we have 'entity_views_handler_relationship' for this feature, and a core Views-style handler called say 'entity_handler_relationship_foobar' then it's going to look like they inherit -- but they don't.

drunken monkey’s picture

@ ZuluWarrior:

I notice this latest patch is only a few hours old, but I though I'd give it a try. I've applied the entity patch too. Nothing seems to have changed and I can't add a relationship from my search view to the actual node fields (and formatters)

Is this just the same problem as described in the other (Search API) issue? Or do you really have no new options, e.g., for adding a relationship to the node author? Displaying image fields should now also work smoothly (and you don't need a relationship for that, if the image field is directly on the node).
If really nothing changed, make sure you patched the right site's code and then you have cleared all caches.

@fago: Let's just discuss this in person.

mh86’s picture

Tested the patch with some search api views and it works pretty well for me. The only thing I have noticed so far is that the option 'retrieve result data from index' doesn't work any more as the entities are loaded in any case.

ZuluWarrior’s picture

Thanks for the reply DM,

Yea after patching I create a new view on a new database index, went to the relationships tag and still got the standard "There are no relationships available to add." message. I cleared the cache...

I will go back and give this another 'vanilla' test in a bit (deadlines delay me). I'm 'pretty sure' I didn't get my sites mixed up but think it would be prudent for me to have another crack :)

FYI, I don't know if file/image is different or if its all just based around a file entity, but I am using the media module (so as to list video, audio, external video). It may well be that Image works but the file entity given by media is different?

PS> '@fago: Let's just discuss this in person.' had to have a little lol at that, reading your posts almost made my head hurt! I sense a deep discussion with many flow diagrams....

drunken monkey’s picture

@ fago:
- What did we decide there: Should we keep entity_metadata_wrapper_multiple(), move it to the helper class or get rid of it completely?
- Should we still call it „data selector“, not (e.g.) „property selector“, now that we know they are not exactly the same (regarding list handling).

@ mh86: Ah, you're right. Should be fixed now.

@ ZuluWarrior: OK, then your problems could at least partially be due to to Media module's seemingly incomplete Entity API integration. Doesn't explain why you can't add, e.g., the „Author“ relationship, though … Or isn't the index one on nodes?

dawehner’s picture

Status: Needs review » Needs work
+++ b/entity.infoundefined
@@ -1,12 +1,23 @@
+files[] = views/handlers/entity_views_handler_field_boolean.inc

Just from convention it should be entity_handler_field_boolean.

+++ b/views/entity.views.incundefined
@@ -27,15 +28,176 @@ function entity_views_data() {
+  $tables = &drupal_static(__FUNCTION__, array());
+
+  if (!isset($tables[$type])) {
+    $info = entity_get_info($type);

I'm just wondering why you need a static cache here. Views itself should take care that this is just called once per cache reset, so a static shouldn't help.

+++ b/views/entity.views.incundefined
@@ -27,15 +28,176 @@ function entity_views_data() {
+  // @todo Some alter hook?

This would be indeed nice.

+++ b/views/entity.views.incundefined
@@ -27,15 +28,176 @@ function entity_views_data() {
+    $options = call_user_func($property_info['options list'], $field_name, $property_info, 'view');
+    if ($options) {
+      $type = 'options';
+      $additional_field['options'] = $options;
+    }

Alternative you could define a options callback and get the options there. See views_handler_filter_in_operator

+++ b/views/handlers/entity_views_field_handler_helper.incundefined
@@ -0,0 +1,337 @@
+      $w = $wrappers;
+      while (is_array($w)) {
+        $w = array_filter($w);
+        $w = reset($w);
+      }
+      if ($w) {
+        $type = $w->type();

Oh this is a neat way to find the first non-array element. But this should at least get a comment what it does. replacing $w with something else might help as well.

+++ b/views/handlers/entity_views_field_handler_helper.incundefined
@@ -0,0 +1,337 @@
+      $h = $handler;
+      $view = $h->view;
+      while (!empty($h->relationship) && !empty($view->relationship[$h->relationship])) {
+        $h = $view->relationship[$h->relationship];
+        $ret = $h->real_field . ($ret ? ":$ret" : '');

As above using a short name doesn't really help to make it more readable.

This is just a review of the first part.

drunken monkey’s picture

Ah, finally some Views expert in this issue! ;)
Thanks for reviewing, I'll post a revised patch tomorrow!
(Further comments of course still welcome.)

tnightingale’s picture

I get the following warning:

Warning: array_flip() [function.array-flip]: Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->load()

In /views/handlers/entity_views_handler_field_entity.inc render_entity_link(); entity_load_single() is getting called with $entity being boolean FALSE.

I'm not familiar enough with the code to know if this is the right place for a fix but the following change removes the warning:

  public function render_entity_link($entity, $values) {
    $type = $this->definition['entity type'];
-    if (!is_object($entity) && isset($entity)) {
+    if (!is_object($entity) && !empty($entity)) {
      $entity = entity_load_single($type, $entity);
    }
    if (!$entity) {
      return '';
    }
    ...

No other glaring issues when testing. Sorry I haven't had time to do a more thorough review.
I have attached an updated patch containing the change made above.

mh86’s picture

patches from #41 or #44 broke many of my existing views with errors like following:

Notice: Undefined index: message in views_handler_field_field->access() (Zeile 78 von views/modules/field/views_handler_field_field.inc).
Notice: Undefined index: message in views_handler_field_field->query() (Zeile 134 von views/modules/field/views_handler_field_field.inc).
Notice: Undefined index: entity keys in views_handler_field_field->query() (Zeile 146 von views/modules/field/views_handler_field_field.inc).
  
Debug:
'Exception: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near \'AS message_, \'\' AS field_data_field_photo__entity_type
FROM 
message message
INN\' at line 1'
in views_plugin_query_default->execute() (Zeile 1386 von views/plugins/views_plugin_query_default.inc).
fago’s picture

Just from convention it should be entity_handler_field_boolean.

Hm, I'd prefer having views integration under the entity_views namespace. handler is not a reserved keyword for views ;)

- What did we decide there: Should we keep entity_metadata_wrapper_multiple(), move it to the helper class or get rid of it completely?

Let's do away with it as publicly available helper as it doesn't make any sense for others. I don't care whether there is function in the helper class for it, as long as it is clearly entity-views specific stuff.

- Should we still call it „data selector“, not (e.g.) „property selector“, now that we know they are not exactly the same (regarding list handling

Imo it should still be data selector. Coming up with multiple, similar terminologies won't help anyone - even if there are some subtle differences. Those differences should be described in the docblock of the helper though.

Pedro Lozano’s picture

Does this patch implies that any other modules that does not uses the entity module and implements its own views integration has to start depending on entity and use the functions provided by this patch for its views integration to work?

Because ZuluWarrior and I have pointed to other modules' views integration (entityreference, relation, media) that do not work with views built using this patch (ex: search_api) and the answer so far has been that it's the other module's fault.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
57.79 KB

I'm just wondering why you need a static cache here. Views itself should take care that this is just called once per cache reset, so a static shouldn't help.

It takes care of entity_views_data(), but not the nested entity_views_table_definition(). The latter can also be used by other modules (like the Search API) to get a base for their own base tables.
See the „copy + change“ notes in the figure from #1.

Alternative you could define a options callback and get the options there. See views_handler_filter_in_operator

Ah, yes, that could be a nice improvement, too. We'd need to store all the arguments to the call, too, though, so would need more space for few options (conent type, user roles, …). On the other hand, there is a huge saving potential for fields with lots of options (like taxonomy terms – they are treated as entities, though, so have no option list anyways).
fago, what would you say?

I'm not familiar enough with the code to know if this is the right place for a fix but the following change removes the warning:

Thanks for spotting this! I don't think we can find a better place for it, either. However, the fix is not completely correct, as we have to allow IDs like 0 or ''. Using isset($entity) && $entity !== FALSE for now.

patches from #41 or #44 broke many of my existing views with errors like following:

Are you completely sure? This view obviously uses Views' default query plugin, so shouldn't be influenced at all by this patch. Or, maybe you could check whether we inadvertently override some other table definitions?

Does this patch implies that any other modules that does not uses the entity module and implements its own views integration has to start depending on entity and use the functions provided by this patch for its views integration to work?

Because ZuluWarrior and I have pointed to other modules' views integration (entityreference, relation, media) that do not work with views built using this patch (ex: search_api) and the answer so far has been that it's the other module's fault.

No, it doesn't imply that. These are basically just helpers for people to create special kinds of Views integrations – modules not wanting to use this don't have to care about it at all.
The problem with the other modules was their apparently flawed Entity API integration, which this Views integration uses. Nothing Views-specific.

@ fago, RE alter hook/hook layout: OK, how do we do this now? Did we reach a conclusion there – and, what was it? Or just tell me how it should be done, in your opinion.

Attached is an updated patch. The sandbox is still up-to-date, too. I also now added tags for the different patches I posted, so people tracking the sandbox can easily see the changes between them.

mh86’s picture

Yes, this patch definitely breaks views that list messages. The Message module is based on the EntityDefaultViewsController, so maybe something has changed?

das-peter’s picture

Just posted a patch in the issue queue of the sandbox - I hope this is the right place to do so.
#1293198: Notice: entity_key_array_by_property() must be an array

fago’s picture

Status: Needs review » Needs work
+++ b/includes/entity.property.inc
@@ -311,6 +326,20 @@ function entity_property_list_extract_type($type) {
+ * Extracts the innermost type for a list type string like list<date>.

Maybe we should give an example with multiple nested lists here, as this is when you need the function.

+++ b/views/entity.views.inc
@@ -35,10 +36,171 @@ function entity_views_data() {
+  // Add generic (non-base) tables for all entity types.

Let's point to entity_views_table_definition() for more info about that using @see.

+++ b/views/entity.views.inc
@@ -35,10 +36,171 @@ function entity_views_data() {
+ * Helper function for getting the entity Views table definition for a type.
+ *
+ * @param $type
+ *   The entity type whose table definition should be return.
+ *
+ * @return
+ *   An array containing a generic Views table definition for the entity type.

Let's document what the views integration is about and how it works (using the objects...).

+++ b/views/entity.views.inc
@@ -35,10 +36,171 @@ function entity_views_data() {
+  $tables = &drupal_static(__FUNCTION__, array());

Let's comment why we statically cache it.

+++ b/views/entity.views.inc
@@ -35,10 +36,171 @@ function entity_views_data() {
+  // @todo Some alter hook?

Ok, why not.

+++ b/views/entity.views.inc
@@ -35,10 +36,171 @@ function entity_views_data() {
+    // Others might have unset the relationship handler in the alter hook.
+    if (isset($field_handlers['relationship'])) {

If they do, it's there fault. Let's don't check for silly stuff like that.

+++ b/views/entity.views.inc
@@ -35,10 +36,171 @@ function entity_views_data() {
+    $additional_field['entity_tables'] = array();
+    $additional_field['entity type'] = $table['table']['entity type'];

I guess this is there because the handler needs it. Imho the entity type is dupcliated here, so let's comment we only add this because the handler requires it.

+++ b/views/entity.views.inc
@@ -35,10 +36,171 @@ function entity_views_data() {
+  // Copied from EntityMetadataWrapper::optionsList()
+  elseif (isset($property_info['options list']) && is_callable($property_info['options list'])) {

Yep, if feasible I think calling the options-list via an callback would be better.
Not sure though, whether it's simple to get the context of the handler into the options callback.

+++ b/views/entity.views.inc
@@ -35,10 +36,171 @@ function entity_views_data() {
+ * Helper function for creating valid Views field identifiers out of property accessors.

data selectors?

+++ b/views/entity.views.inc
@@ -35,10 +36,171 @@ function entity_views_data() {
+  // If we re-use aliases (tables might be different), we have to make sure we
+  // never return the same alias for two different fields (even this is highly
+  // unlikely).

How would that be possible? Do we really need that?

+++ b/views/entity.views.inc
@@ -103,6 +265,7 @@ class EntityDefaultViewsController {
+      $data[$table]['table']['entity type'] = $this->type;

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+      $form['list']['glue'] = array(
+        '#type' => 'textfield',
+        '#title' => t('List seperator'),
...
+  /**
+   * Return a Entity API data selector for the given handler's relationship.
+   *
+   * @param $handler
+   *   The handler for which to construct the selector.
+   * @param $complete

Label and machine-name should basically use the same name.

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+    $handler->base_field = ltrim(substr($handler->real_field, strrpos($handler->real_field, ':')), ':');

I've seen that before. Maybe we should do a small helper for extracting the field-name. That would also help documenting what it is doing.

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+      $load = FALSE;
+      foreach ($values as $row) {
+        if (empty($row->_entity_properties) || !array_key_exists($selector, $row->_entity_properties)) {

Checking for the first row should do it, no? All rows should look equally.

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+    elseif (method_exists($handler->query, 'get_result_entities')) {
+      list($type, $entities) = $handler->query->get_result_entities($values, NULL);
+      $wrappers = array();
+      foreach ($entities as $id => $entity) {
+        $wrappers[$id] = entity_metadata_wrapper($type, $entity);
+      }
+    }
+    else {
+      return;
+    }

Don't silently fail. Let's just use get_result_entities else.

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+    $handler->entities = array();
+    $handler->wrappers = $wrappers;
+    if (isset($entities)) {
+      $handler->entities = array_filter($entities);
+    }
+    elseif (entity_get_info($type)) {
+      // We can just load the entities from the wrappers.
+      foreach ($wrappers as $id => $wrapper) {
+        $handler->entities[$id] = $wrapper->value();
+      }
+    }

Do we need both the entities and the wrapper at the handler? I guess the wrappers should suffice.

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+   * Return a Entity API data selector for the given handler's relationship.
+   *
+   * @param $handler
+   *   The handler for which to construct the selector.
+   * @param $complete

Let's provide an example selector and explain how it handles lists.

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+    $ret = '';

$return

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+      $curr_handler = $handler;

$current...

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+   * Overridden to use a metadata wrapper (if necessary).

This function does not override anything.

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+        if ($field === 'entity object') {
+          $values->_entity_properties[$selector] = $wrapper->value();
+        }
+        else {

What's the entity object special case?

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+          $values->_entity_properties[$selector] = isset($wrapper->$field) ? $wrapper->$field->value(array('identifier' => TRUE)) : $default;

This might throw an exception.

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+    return $values->_entity_properties[$selector];

As this is usable without entities too, maybe we should call _entity_properties "_wrapper_values"? Thoughts?

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+    $entity = $handler->get_value($values, 'entity object');

I see. Does that work with values already in _entity_properties ?

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+      $ret = $handler->render_single_value($value, $values);

$return

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+      unset($handler->_cache[NULL]);

cache NULL. WTF?

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+    // Lame fallback in case the field handler doesn't provide the method.
+    return is_scalar($value) ? check_plain($value) : nl2br(check_plain(print_r($value, TRUE)));

Again, let's don't provide lame defaults. Assume the method is there and call it. If not, the developer needs to see errors.

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+   * Lists of values in a property are automatically handled. They will result in
+   * nested arrays being returned.

and are not part of the selector, right? Maybe add an example.

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,450 @@
+          $id = $property->getIdentifier();
+          $entities[$property->type()][$id] = $i;

Maybe we should add a commen that we want to use entity_load() for multiple loading.

+++ b/views/handlers/entity_views_handler_field_entity.inc
@@ -0,0 +1,171 @@
+class entity_views_handler_field_entity extends views_handler_field_entity {
+
+  /**

Oh, this one overlaps with http://drupal.org/node/1209678. Let's get the other one in first, then re-use it here.

+++ b/views/handlers/entity_views_handler_field_entity.inc
@@ -0,0 +1,171 @@
+   * Render a value as a link to the entity, if applicable.

No comma here.

+++ b/views/handlers/entity_views_handler_field_entity.inc
@@ -0,0 +1,171 @@
+    if (is_object($entity) && ($url = entity_uri($type, $entity))) {
+      // We LOVE our core bugs! (http://drupal.org/node/1057242)
+      if ($type === 'file' && !is_array($url)) {
+        $url = array(
+          'path' => file_create_url($url),
+          'options' => array(),
+        );
+      }

Don't add workarounds - let's rely upon the fix to arrive soon.

+++ b/views/handlers/entity_views_handler_relationship.inc
@@ -0,0 +1,27 @@
+ * Relationship handler for pure entity tables.

pure entity tables!? I guess we should find a good terminology for that. Data selection tables? :D Basically, we are using data selectors instead of queries to get the data.

drunken monkey’s picture

Yes, this patch definitely breaks views that list messages. The Message module is based on the EntityDefaultViewsController, so maybe something has changed?

Nope, nothing. Except for adding the "entity type" to tables.
So, again, please find out what could cause this.

Just posted a patch in the issue queue of the sandbox - I hope this is the right place to do so.

Fixed it – but no, please mention problems here directly. I don't think this issue is complex enough to warrant its own issue queue.

Checking for the first row should do it, no? All rows should look equally.

Should, but needn't. And as you have maybe noticed, I'm in favor of correctness instead of brevity.

What's the entity object special case?

A special case to get the entity object.

I see. Does that work with values already in _entity_properties ?

No. If handlers need the entity object regardless of the "link_to_entity" option, they have to pass $load_always = TRUE.

cache NULL. WTF?

If the handler's normal field is retrieved, $field = NULL is passed to get_value(). Hence, we need to cache the value for that case, and that seemed to me the more intuitive variant.

Again, let's don't provide lame defaults. Assume the method is there and call it. If not, the developer needs to see errors.

It's more a sensible default than a lame one. After all, that's what some field handler might want to do, so we shouldn't need it to implement the method, I think. Would it be OK if I just changed the comment?

tnightingale’s picture

I ran into an issue with extract_property_multiple in entity_views_field_handler_helper.inc.

I have a list of entities that I want to show in a view. These entities have a relationship to a parent node. The view displays some fields from the entity and some from the parent. Some of these entities have the same parent.

When collecting the parent nodes the entity wrapper ids are mapped in an array keyed by the parent node ids:

foreach ($wrappers as $i => $wrapper) {
  try {
    $property = $wrapper->$name;
    if ($property instanceof EntityDrupalWrapper) {
      $id = $property->getIdentifier();
      $entities[$property->type()][$id] = $i;
    }
    // ...
  }
}
if ($entities) {
  // Map back the loaded entities back to the results array.
  foreach ($entities as $type => $id_map) {
    $entities[$type] = entity_load($type, array_keys($id_map));
    foreach ($entities[$type] as $id => $entity) {
      $results[$id_map[$id]] = entity_metadata_wrapper($type, $entity);
    }
  }
}
// ...

This doesn't work if more than one entity reference the same node as only the last entity's wrapper gets mapped.
The following changes are a possible solution:

foreach ($wrappers as $i => $wrapper) {
  try {
    $property = $wrapper->$name;
    if ($property instanceof EntityDrupalWrapper) {
      $id = $property->getIdentifier();
      $entities[$property->type()][$i] = $id;
    }
    // ...
  }
}
if ($entities) {
  // Map back the loaded entities back to the results array.
  foreach ($entities as $type => $id_map) {
    $entities[$type] = entity_load($type, $id_map);
    foreach ($id_map as $i => $id) {
      $results[$i] = entity_metadata_wrapper($type, $entities[$type][$id]);
    }
  }
}
// ...

Patch attached.

fago’s picture

Checking for the first row should do it, no? All rows should look equally.

Should, but needn't. And as you have maybe noticed, I'm in favor of correctness instead of brevity.

ok, let me rephrase it - all rows have to be equal. That's something we really should require, as it doesn't make much sense to support a case where a single row misses values which all others have, for which row we then load the values.

A special case to get the entity object.

What would be other values one can pass in? Different selectors? Might there be a name-conflict with them?

+  public static function get_value($handler, $values, $field = NULL, $default = NULL) {
...
+    return $values->_entity_properties[$selector];
+  }

It looks strange to me that the function accepts parameters, but in the end has only one fixed value. So calling it multiple times with different parameters leave different values in there? Do we actually need $values->_entity_properties[$selector] ?

No. If handlers need the entity object regardless of the "link_to_entity" option, they have to pass $load_always = TRUE.

I see, we should add some docblock comment to explain that parameter.

If the handler's normal field is retrieved, $field = NULL is passed to get_value(). Hence, we need to cache the value for that case, and that seemed to me the more intuitive variant.

!?

+    // There is a value cache on each handler so parent handlers rendering a
+    // single field value from a list will get the right data.
+    if (isset($handler->_cache[$field])) {
+      return $handler->_cache[$field];
+    }

I don't get what this cache thing does at all? Could you explain it for me/us? :)

It's more a sensible default than a lame one. After all, that's what some field handler might want to do, so we shouldn't need it to implement the method, I think. Would it be OK if I just changed the comment?

I see - yep, in that case the comment shouldn't say it's lame :)

drunken monkey’s picture

What would be other values one can pass in? Different selectors? Might there be a name-conflict with them?

Yes, different selectors. And since they don't contain spaces, I think we can preclude conflicts.

It looks strange to me that the function accepts parameters, but in the end has only one fixed value. So calling it multiple times with different parameters leave different values in there? Do we actually need $values->_entity_properties[$selector] ?

No, that's just caching. As long as you don't provide different defaults for each call, it should be correct, though. Should I remove it?

I don't get what this cache thing does at all? Could you explain it for me/us? :)

Example: integer field handler, for a list of integers.
-> render_list() is called
-> calls render_single_value() for each contained number
-> calls the parent's (Views' own views_handler_field_number) render() method
-> that method doesn't take a value, but uses get_value() to get it
-> without this cache, the parent handler would get an array and fail
-> we store the single value in this cache before calling parent::render() so get_value() will return that

drunken monkey’s picture

Let's document what the views integration is about and how it works (using the objects...).

Let's comment why we statically cache it.

Let's provide an example selector and explain how it handles lists.

If you see such documentation shortcomings, could you please just fix them yourself? You know better what you expect for your module, you have commit access to the sandbox (although a note would of course be good) and I think writing back and forth about costs both of us more time in the end.

+++ b/views/entity.views.inc
@@ -35,10 +36,171 @@ function entity_views_data() {
+  // If we re-use aliases (tables might be different), we have to make sure we
+  // never return the same alias for two different fields (even this is highly
+  // unlikely).

How would that be possible? Do we really need that?

E.g., foo_bar:baz and foo:bar_baz would otherwise get the same alias. So yes, if we want to avoid bugs, we do need that.

Label and machine-name should basically use the same name.

I don't really understand what you mean, and also think the quote you selected for that wasn't the right one?

As this is usable without entities too, maybe we should call _entity_properties "_wrapper_values"? Thoughts?

_wrapper_values wouldn't be namespaced. I'd stick with _entity_properties, but in the end it's of course up to you.

Oh, this one overlaps with http://drupal.org/node/1209678. Let's get the other one in first, then re-use it here.

And how exactly should I do this? Also, the other one has only limited functionality in comparison, as far as I can see?

@ #53: Thanks for spotting and patching this. Committed your fix.
The patch didn't apply correctly, though. To avoid this (and also to simplify seeing what was changed), it would be best to create patches based on the latest sandbox version.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
59.35 KB

Here is the updated patch, hope I didn't forget anything.

By the way, maybe a new „Views integration“ component for issues would be appropriate?

mh86’s picture

Status: Needs review » Needs work

tested patch from #57 and two things I have noticed so far:
- the existing relations (like message -> user -> profile ->field_collection) are broken (that's the reason why my message views didn't work any more), but re-adding them works
- 'retrieve result data from index' (Search API Solr setting) still ignored

drunken monkey’s picture

- the existing relations (like message -> user -> profile ->field_collection) are broken (that's the reason why my message views didn't work any more), but re-adding them works

fago, do you have any idea what part of this patch could influence the Messages module?

- 'retrieve result data from index' (Search API Solr setting) still ignored

It does work for me, sorry. You'll have debug what's going wrong yourself – try manually adding field values in SearchApiSolrService::extractResults() (set $result['fields'][$field] somewhere in the foreach loop).
If that doesn't work, see what happens in get_value() – are the field values correctly stored in $entity->_entity_properties, and why aren't they returned? Checking the former additionally in pre_render() could also help.

fago’s picture

> I don't get what this cache thing does at all? Could you explain it for me/us? :)

Example: integer field handler, for a list of integers.
-> render_list() is called
-> calls render_single_value() for each contained number
-> calls the parent's (Views' own views_handler_field_number) render() method
-> that method doesn't take a value, but uses get_value() to get it
-> without this cache, the parent handler would get an array and fail
-> we store the single value in this cache before calling parent::render() so get_value() will return that

Thanks. ok, I was not able to get that from previous comments / looking through the code, so we should make that clearer. I think "cache" is not an appropriate term, because a cache needs to be able to handle cache misses. For us this is not the case. Also, I've seen no other use of the ->_cache beside with NULL as key. So what about just using $handler->value and just return it if its there and no $field is specified?
Then maybe improve the comment to not talk about the "right value", but the "single value".

If you see such documentation shortcomings, could you please just fix them yourself? You know better what you expect for your module, you have commit access to the sandbox (although a note would of course be good) and I think writing back and forth about costs both of us more time in the end.

I've not applied the patch yet, but just visually reviewed the code - so I doubt I'd be faster. Also, I think it makes much sense that the code writer comments its own code. So the comments can help me to understand the patch while reviewing it, what in turn should help proving that the comments work. Thus, let's stay like that please.

E.g., foo_bar:baz and foo:bar_baz would otherwise get the same alias. So yes, if we want to avoid bugs, we do need that.

I see. Still, this is problematic as it might result in different views fields depending on some other configuration. What would make configurations exports unreliable and unstable. Maybe we can come up with another method that works around that?
What about replacing underscores with double-underscores and colons with a single underscore. Yes, ugly - but that should be reliable. Or maybe dashes are allowed in views-field names?
Also, I don't think having the same field names in two different tables should be a problem.

Label and machine-name should basically use the same name.

I don't really understand what you mean, and also think the quote you selected for that wasn't the right one?

I'm referring to the glue - vs. separator terminology what isn't consistent across machine-names and labels.

> As this is usable without entities too, maybe we should call _entity_properties "_wrapper_values"? Thoughts?

_wrapper_values wouldn't be namespaced. I'd stick with _entity_properties, but in the end it's of course up to you.

Indeed, ok so let's stay with that.

Oh, this one overlaps with http://drupal.org/node/1209678. Let's get the other one in first, then re-use it here.

And how exactly should I do this? Also, the other one has only limited functionality in comparison, as far as I can see?

This and an issue spotted by mh spots a conceptional problem of the current approach: We are not solely relying on get_result_entities() any more, what makes this incompatible with handles created based upon views_handler_field_entity.

The problem is with the handler on the on side which is generally working with entities - fine. But it relies on the contract that the query handler gives the entity to it.
Then, we have our new wrapper handlers that entity-load when necessary. There the contract is different and both worlds don't match.

So, here is a proposal to fix it:
* Query handlers have to provide the data, so we should move our entity-loading in there (->get_result_entities() or optionally wrappers).
* In a way the "data-selection" part is our query (or part of it) and needs to be applied by the query handler.
* So any table/query backend that links to one of those entity-tables using the wrapper-handler need to be able to entity-load on demand and to apply the data selectors. Consequently it would be probably a good idea to provide some static query helper functions which implement the data-selection logic.
* I guess this also means we have to split up "data-selectors" in the part that gets the entity and the part that remains afterwards. That makes sense to me though.

Sry for coming up with that this late, looks like we completely missed that during our initial planning. :(
Please ping me if you want to discuss that more.

@issue-overlap:
Nevermind. We can easily postpone the other issue for that one too.

fago’s picture

fago, do you have any idea what part of this patch could influence the Messages module?

ouch yes. I think it's caused by https://drupal.org/node/1287240 - thus unrelated.

drunken monkey’s picture

I see. Still, this is problematic as it might result in different views fields depending on some other configuration. What would make configurations exports unreliable and unstable. Maybe we can come up with another method that works around that?
What about replacing underscores with double-underscores and colons with a single underscore. Yes, ugly - but that should be reliable. Or maybe dashes are allowed in views-field names?
Also, I don't think having the same field names in two different tables should be a problem.

I think, just replacing colons with double underscores makes more sense – and in any case, this still isn't completely reliable of course. (And no, Views field names cannot contain dashes. They have to be valid PHP function name suffixes, although nowhere documented.) It might still be the preferable option, though, you're right.
Having the same alias for different fields in two different tables is a problem because of the static caching we currently do. Thinking it through, it might result in identical aliases being returned for different fields for a third table. But I guess the solution there is really just to get rid of that part and do some simple str_replace()ing.

ouch yes. I think it's caused by https://drupal.org/node/1287240 - thus unrelated.

What are change records, how can one create them and why can't I find any documentation on them? (The last question is rethorical.)
Anyways, good to know this isn't the fault of this patch, would have been rather mysterious to me.

mh86’s picture

Thanks for clarifying the changes in the relationship naming, sometimes it's hard to identify where the problems come from.

Concerning the 'retrieve result data from index', I performed some debugging and it turned out that it works for some properties (e.g. user:uid), but not for more advanced fields like field_peronal_info:field_firstname (field_personal_info is a field_collection).

All values are correctly available in SearchApiSolrService::extractResults(), but changing the field_peronal_info:field_firstname e.g. had no impact on the rendered result. entity_views_handler_field_field::pre_render calls EntityFieldHandlerHelper::pre_render($this, $values, TRUE), where $load_always is set to TRUE.
Is there a way to avoid $load_always for fields?

drunken monkey’s picture

Is there a way to avoid $load_always for fields?

Hm, no, I don't think so … Since we now use the Field API field formatters, which need the loaded entity, there is hardly anything we can do in this respect. Your only choice is to override the Views field handler for that field, or for Field API fields in general.

drunken monkey’s picture

Ah, missed one paragraph in my reply:

Thanks. ok, I was not able to get that from previous comments / looking through the code, so we should make that clearer. I think "cache" is not an appropriate term, because a cache needs to be able to handle cache misses. For us this is not the case. Also, I've seen no other use of the ->_cache beside with NULL as key. So what about just using $handler->value and just return it if its there and no $field is specified?
Then maybe improve the comment to not talk about the "right value", but the "single value".

This was meant for handlers that need some additional fields, not just the one they represent. However, as we don't have such a case in our code, and handlers can also easily handle this themselves if the need arises, your proposal makes sense. Maybe $handler->current_value would be even clearer for the name.

Updated patch probably coming tomorrow, had a busy week.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
61.46 KB

Patch attached, I hope I didn't forget anything.
Regarding the mentioned „conceptual problem“, I still don't have a clear picture, so no solution for that is included yet.

The last submitted patch, 1266036--views-field-handlers-66.patch, failed testing.

drunken monkey’s picture

Wow, nice! :D

fago’s picture

Hm, no, I don't think so … Since we now use the Field API field formatters, which need the loaded entity, there is hardly anything we can do in this respect. Your only choice is to override the Views field handler for that field, or for Field API fields in general.

At least we should ensure it entity-loads-multiple in that case, i.e. pre-load it with multiple so the single operations can benefit from static cache. Maybe we should check though whether static caching is active since comments don't have it?

drunken monkey’s picture

At least we should ensure it entity-loads-multiple in that case, i.e. pre-load it with multiple so the single operations can benefit from static cache.

Ah, you're right, it doesn't do that at the moment as the wrapper-extraction stops at that point, before loading this level of entities, too.

I also discovered that since the Search API doesn't load the base entities, they are loaded individually at the moment. Will have to fix that (probably better in the Search API).

Maybe we should check though whether static caching is active since comments don't have it?

Yes, might be a good idea, didn't think of that.

drunken monkey’s picture

While testing, I discovered that Field API fields on listed entities (e.g., fields on tags) result in a fatal error. Therefore re-vamped Field API field handling a bit, should work now. Other kinds of nested fields might also be affected (more so as I also spotted a very nasty typo (by fago, not me :P) completely invalidating list handling in some cases) and now fixed.

I also now clarified the requirements for query plugins.

Regarding pre-loading the entities I discovered that this is already done (except the base entities, as explained above – that one is fixed in #1231512-30: Use real Relationships instead of level magic in Views integration). However, it doesn't seem to work for entity lists, like multi-valued taxonomy term references. See the new @todo in entity_views_field_handler_helper.inc:461 – the list iteration seems to return wrappers with the complete objects, not just the IDs, therefore loading all contained items individually. I think this has to be fixed separately – as far as I can see, it will never make sense to create a new wrapper with the loaded entity, if you have just the ID.
Can't see where to fix this, though, the wrapper code is just too complex. However, it might be both sufficient, and also generally sensible to always use 'identifier' => TRUE for internal use of value().

fago’s picture

I think this has to be fixed separately – as far as I can see, it will never make sense to create a new wrapper with the loaded entity, if you have just the ID.

What's wrong with having a wrapper only containing an id? It won't load the entity if not necessary.

However, it might be both sufficient, and also generally sensible to always use 'identifier' => TRUE for internal use of value().

Sounds like a good way to go.

das-peter’s picture

Looks like I've found a possible issue with entity_view().
It seems like the structure of the returned array isn't consistent anymore.
Before the patch the result was keyed by the entity type e.g. "node". But now node_view_multiple() is called - which returns the result keyed by "nodes".
I guess this could break quite a lot of stuff :|

Related issue #1305950: Undefined index: node in entityreference_view_widget_field_widget_form

drunken monkey’s picture

What's wrong with having a wrapper only containing an id? It won't load the entity if not necessary.

Yes, that's exactly what I'm saying. Currently, when iterating over a list wrapper containing entities, the returned wrappers for each element seem to contain the loaded entity, not just the ID. Hence missing usage of the multiple-load functionality in this case.

Sounds like a good way to go.

Could you then please fix this in the wrapper, probably in a separate issue? As said, I don't really get the internals of the wrapper code, with the parent references, etc.
It's not that urgent either, the code basically works after all. It will probably just be a bit faster with this fixed.

@ das-peter: I don't see how this would affect this issue here? The code does use entity_view(), but I don't think we should fix that here if it's broken.

das-peter’s picture

Tomas: I'm slapping myself right now - for being a douche not searching in the patch to look if it changed something there.

mh86’s picture

There is one use case I still couldn't get working:
I have a Search API index with field collections. Additionally I have my own entity property from a field collection to a profile2. Adding this relation in Views basically works, but when using a field that is based on the views_handler_field_entity handler (e.g. Rules Link in combination with the patch in #1264258: Update to latest views changes), errors are thrown and no values are displayed. Following code in pre_render
list($this->entity_type, $this->entities) = $this->query->get_result_entities($values, !empty($this->relationship) ? $this->relationship : NULL, $this->field_alias);
seems to ignore the relationship (although it is correctly set in $this->relationship) and returns the field collections instead of the profiles.

fago’s picture

Yes, that's exactly what I'm saying. Currently, when iterating over a list wrapper containing entities, the returned wrappers for each element seem to contain the loaded entity, not just the ID. Hence missing usage of the multiple-load functionality in this case.

It only loads the entity if you call value() but didn't tell it to get the id only.

Could you then please fix this in the wrapper, probably in a separate issue? As said, I don't really get the internals of the wrapper code, with the parent references, etc.

There is nothing to fix. E.g.

$wrapper = entity_metadata_wrapper('list<node>', array(100, 2));
echo $wrapper[0]->getIdentifier();

does not cause an entity-load.

drunken monkey’s picture

There is nothing to fix. E.g.
$wrapper = entity_metadata_wrapper('list<node>', array(100, 2));echo $wrapper[0]->getIdentifier();
does not cause an entity-load.

Look in the patch (search for "@todo") for my example and debug it yourself to see that the foreach loop does cause separate entity loads.

@ Matthias: That's a Search API problem, not a problem of this patch. Please report it in #1231512: Use real Relationships instead of level magic in Views integration.

Other than that, is the patch now good to go? Can we finally get it committed?

fago’s picture

Status: Needs review » Needs work
+++ b/entity.api.php
@@ -383,5 +383,27 @@ function entity_hook_field_info() {
+ * Alter the handlers used by the entity tables provided by this module.

I don't think this comment is clear. *This module* also provides regular view-tables for entity types using the Views intetgration controller. We'll need to find a name for this tables + document what those tables are for and how they work at a single place (-> entity_views_table_definition() ?)

Naming idea: "Entity data selection tables"

+++ b/entity.api.php
@@ -383,5 +383,27 @@ function entity_hook_field_info() {
+ *   Values for all specific entity types can additionally be added.

can be additionally added.

+++ b/entity.api.php
@@ -383,5 +383,27 @@ function entity_hook_field_info() {
+function hook_entity_views_get_field_handlers_alter(array &$field_handlers) {

Let's remove the "get" from the name - we usually don't have that in drupal hooks.

+++ b/includes/entity.property.inc
@@ -311,6 +326,20 @@ function entity_property_list_extract_type($type) {
+ *   The innermost type in the nested list type, or the type itself if it is no
+ *   list type.

Hm, just returning the type if it's not a list is a bit weird and not consistent with the other extract list function. So let's follow its function signature and return FALSE if there is no list.

Also, let's name the function the same way as the other function, i.e. "entity_property_list_extract_inner_type".

+++ b/views/entity.views.inc
@@ -35,10 +37,208 @@ function entity_views_data() {
+  // Add generic (non-base) tables for all entity types.

Let's insert our name for the tables here.

+++ b/views/entity.views.inc
@@ -35,10 +37,208 @@ function entity_views_data() {
+function entity_views_table_definition($type) {

Great work on the docs here! Maybe, we should point to the search-api as example though too.

+++ b/views/entity.views.inc
@@ -35,10 +37,208 @@ function entity_views_data() {
+function _entity_views_field_identifier($field, array $table) {

We still need to solve this. I fear suddenly breaking Views configurations as soon as some properties/fields are added else.

+++ b/views/entity_views_example_query.php
@@ -0,0 +1,69 @@
+ * Contains an example for a Views query plugin that could use the Entity API Views tables.

Let's insert our name and the pointer to the docs here.

+++ b/views/entity_views_example_query.php
@@ -0,0 +1,69 @@
+   *   An array with two entries, keyed by 0 and 1: the entity type of entities

I'd call that:
A numerically indexed array containing two items: foo, bar.

+++ b/views/entity_views_example_query.php
@@ -0,0 +1,69 @@
+   *   An array with two entries, keyed by 0 and 1: the entity type of entities

see above

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,511 @@
+   * Automatically takes care of relationships, either defined via Views or via
+   * Entity API data selectors.

hm, both are defined via Views, not?
So maybe just say:
"including entity data selection relationships."

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,511 @@
+          $values->_entity_properties[$selector] = $wrapper->value();

This might throw an exception too.

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,511 @@
+   * to arrive at a desired property, that may be nested in related entities or

There is no comma before a that.

+++ b/views/handlers/entity_views_handler_field_boolean.inc
@@ -0,0 +1,98 @@
+class entity_views_handler_field_boolean extends views_handler_field_boolean {

So as we require only get_result_entities() now our handlers are finally compatible with every entity table, right? That's great stuff!

+++ b/views/handlers/entity_views_handler_field_entity.inc
@@ -0,0 +1,165 @@
+   * Render a value as a link to the entity, if applicable.

no comma here.

+++ b/views/handlers/entity_views_handler_relationship.inc
@@ -0,0 +1,27 @@
+ * Relationship handler for pure entity tables.

insert our name.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
63.6 KB

Should contain all requested improvements.

das-peter’s picture

Strange, I see the commit of #80 in the sandbox commit log and the commit shows up in the repo but I can't get it using git. Even if I make a new clone the last commit in the log is the one from October 10, 2011 20:06.
Any ideas?

drunken monkey’s picture

Sorry, should work now.

(Confusingly, Git allows pushing a tag without pushing the commit it is tagging.)

das-peter’s picture

Thanks Tomas - it's working :) I'll test our current stuff with the latest patch asap.

fago’s picture

Status: Needs review » Needs work

Patch does not apply any more. I fixed merge conflicts and committed them to the repo. Also I've done through and added documentation improvements - see Git for details. Updated patch attached.

   * Load the entities for all rows that are about to be displayed.
   *
   * Automatically takes care of relationships, including data selection
   * relationships.
   */
  public static function pre_render($handler, &$values, $load_always = FALSE) {

This function code need some more docs though - how are which relationships resolved?

Then I started testing the patch in conjunction with the search api patch....

a) Require this relationship does not work. I guess we should just remove this option, or better set it to #disabled and explain why it's not supported.

b) Multiple values handling of relationships is different, we don't have multiple rows but just end up with multiple values. What's weird in case we'd have again multiple values in a field because they are just combined... We should explain that there to the user.

c) The "view mode" option of the entity field only makes sense in case of the complete entity display. Thus we should use such a #dependency key there too.

d)When listing tags referenced of tags referenced by a node (my index) I get:

    Warning: Missing argument 2 for EntityFieldHandlerHelper::extract_list_wrapper_values(), called in ....drupal-7/sites/all/modules/entity/views/handlers/entity_views_field_handler_helper.inc on line 266 and defined in EntityFieldHandlerHelper::extract_list_wrapper_values() (line 262 of ....drupal-7/sites/all/modules/entity/views/handlers/entity_views_field_handler_helper.inc).
    Notice: Undefined variable: field in EntityFieldHandlerHelper::extract_list_wrapper_values() (line 273 of ....drupal-7/sites/all/modules/entity/views/handlers/entity_views_field_handler_helper.inc).
    Notice: Undefined variable: field in EntityFieldHandlerHelper::extract_list_wrapper_values() (line 276 of ....drupal-7/sites/all/modules/entity/views/handlers/entity_views_field_handler_helper.inc).

Not sure if its related, but it seems that this only appears if I filter for "article" nodes in my exposed content type filter.

e) I got a fatal error when I used a node multiple node reference field and added it as relation:

Fatal error: Cannot use object of type stdClass as array in ....drupal-7/sites/all/modules/views/handlers/views_handler_field_entity.inc on line 80 

Steps to produce it:
• Add a node-reference field (in my case to pages) to a node type
• add the dataselection relationship to those fields
• add the "node link" field to display this field from the page-nodes
• bang

Then I tried listing the main body via field formatters for those referenced page nodes. Got sql errors in preview mode. After saving I got an exception:

EntityMalformedException: Missing bundle property on entity of type node. in entity_extract_ids() (line 7381 of ....drupal-7/includes/common.inc).

I guess there is again an array (multiple values) passed to something that does not support it. That finally crashed my test view :(( Do fields correctly recognize they are "multiple" if the parent relationship is already multiple? Next, this seems very problematic to me as fields based upon the views_handler_field_entity (e.g. the node edit link in views) don't have support for dealing with multiple field values.

f)Fields are listed twice in the views UI, e.g.
search for "title" -> "node: Title" appears twice (I guess due to my node base table and the node relationship). I tried it with the regular views "node" base table, in which case the field keeps appearing only once although of the relationship.

g)Linking to entities does not seem to work in case of my referenced pages (multiple). If that does not work with multiple entity references, the option should at least not be available.

f)Minor: Views description don't look nice, e.g.: ":Error: missing help appears for the index main body text"
Else "(No information available)" appears quite often. What's not so nice but should be ok if there is nothing.

Then multiple entity loading only seems to work for the base-table's entities. I'll look into that now whether it's related to the wrappers or not.

fago’s picture

        elseif ($property instanceof EntityListWrapper) {
          // @todo This way of accessing a list seems to trigger entity loads.
          foreach ($property as $item) {

ok, I had a look at that:

The wrappers behave correct in that they do single loads, e.g. if you access $wrapped_node->field_tags[3]->value() it issue a single entity load. But that's the way values are retrieved from lists in our current code, as entities are unwrapped on a singular basis.

So a simple fix would be just doing $wrapped_list->value() before looping over it, what would cause the multiple load before and entities would benefit from the static cache (but it would suck in case there is no static cache as for comments). Still, it would only cause a multiple load *per row*, but not for the whole result set.
In order to support a multiple load for the whole result set the function needs to be improved to collect ids, multiple load them and map back the right entities afterwards - as we do it for single-entity references already. Given this is applicable to lots of use cases (e.g. list tags of "field_tags") I think that's the direction we should pursue.

drunken monkey’s picture

add the "node link" field to display this field from the page-nodes

I couldn't find such a field. However, I've made some changes/improvements/fixes now in that area, so perhaps it works now?

f)Fields are listed twice in the views UI, e.g.

Of course, since they are from different base tables. Nothing we can do about it, I'm pretty sure – Views just isn't really suited for our use cases (although I consider this behaviour rather confusing in normal Views, too – in my opinion, the group shouldn't list the base tables, but also directly the relationships, as I did it in my previous Views integration).

f)Minor: Views description don't look nice, e.g.: ":Error: missing help appears for the index main body text"
Else "(No information available)" appears quite often. What's not so nice but should be ok if there is nothing.

"G" is followed by "H", not "F" again. ;P
And I guess the error only appears for the indexed value, and thus is a Search API problem, not one of this patch? „(No information available)“ appears wherever the property information doesn't provide any description, so there is really nothing we can do.

@ #85: But that's still another issue and doesn't need to happen here, right?

A patch that fixes everything and is absolutely perfect should be attached.

drunken monkey’s picture

Status: Needs work » Needs review

Settings status to „needs review“, even though „is beyond all doubt and should be committed immediately“ is more like it.

fago’s picture

Status: Needs review » Needs work

Of course, since they are from different base tables. Nothing we can do about it, I'm pretty sure – Views just isn't really suited for our use cases (although I consider this behaviour rather confusing in normal Views, too – in my opinion, the group shouldn't list the base tables, but also directly the relationships, as I did it in my previous Views integration).

Ouch. I had another look and indeed each field only works with one of the base tables. That's a real UX nightmare though, so we need to find a solution here somehow. Changing how Views does it is probably out of scope and I think it basically makes sense as it keeps the amounts of fields to select lower.

So maybe we should just fix the names of the search api index fields to be named differently? E.g. "Indexed node: Title"?
Related but a minor thing is, that the base-table name is just named like the index. If you choose "node" as index name choosing base tables is going to be confusing. I'd suggest using "Search API: index name" there. So maybe "Search API Index name: Field name" would be a good naming for fields too then? Well, that would be belong into the search api issue then.

b) Multiple values handling of relationships is different, we don't have multiple rows but just end up with multiple values. What's weird in case we'd have again multiple values in a field because they are just combined... We should explain that there to the user.

I've not seen any explanation? It would help reviewing if you could comment how you fixed or not fixed individual issues.

I now get this notice on my test-view:

    Notice: Undefined index: base in views_handler_field_entity->query() (line 55 of ....drupal-7/sites/all/modules/views/handlers/views_handler_field_entity.inc).
    Notice: Undefined index: base in views_handler_field_entity->query() (line 55 of ....drupal-7/sites/all/modules/views/handlers/views_handler_field_entity.inc).

Sounds like a view problem though?

@ #85: But that's still another issue and doesn't need to happen here, right?
Agreed. Let's handle that in a follow-up.

@node:link

I couldn't find such a field. However, I've made some changes/improvements/fixes now in that area, so perhaps it works now?

oh, sry you'll need the patch I'll linked above for it to appear: #1270890: provide a way to provide entity-related views fields
Then, list nodes and search for "link".

g)Linking to entities does not seem to work in case of my referenced pages (multiple). If that does not work with multiple entity references, the option should at least not be available.

That works now, however now we've a bit strange behavior:
* When I use the node:title field with my referenced multiple pages I'll get all three titles.
* When I use the node link field mentioned from above, I'll get only one link (the first).

That's inconsistent and unpredictable for users. Thus, I don't think we can support the first way but should always list only one item.

@pre_render (see above):

This function code need some more docs though - how are which relationships resolved?

That's still unfixed. Also, why doesn't it pass the $relationship to get_result_entities()? I guess because of multiple value handling, but that's bad. It creates a second code path we have to support and maintain instead of re-using the existing one. -> Another reason to do away with the multiple-referenced-entities complexity.

fago’s picture

ok, I've fixed that notice. The views entity handler didn't correctly rely on the 'base field' property of the relationship definition, as it is elsewhere. I've updated the patch over at #1270890: provide a way to provide entity-related views fields to fix that. Next, we've not set this property: I've updated this patch to do so and pushed it into the Git repo. Interdiff attached.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
63.39 KB

That works now, however now we've a bit strange behavior:
* When I use the node:title field with my referenced multiple pages I'll get all three titles.
* When I use the node link field mentioned from above, I'll get only one link (the first).

That's inconsistent and unpredictable for users. Thus, I don't think we can support the first way but should always list only one item.

I don't think it's worth it to recount our long discussion here. Suffice it to say that I still disagree and think that we should support this – but that we eventually requested an impartial ruling which decided in your favor. So the attached patch removes this functionality and adds a note to the config form for multi-valued relationship stating that only the first entity is inspected.
I think this also resolves all other issues with the previous patches?

The only unusual thing in these latest changes is the fact that we now pass the field name as the third parameter to get_result_entities() (and get_result_wrappers()). This is necessary (and, I believe, by far the best option) to support fields which already contain a data selector, which the Search API uses to provide indexed related fields directly.
However, as Views itself (completely undocumentedly, of course) passes the field name as the third parameter, this is actually a 100% compatible, so no worries there. (I previously had the complete handler as the third parameter, which would have allowed greater flexibility, but explicitly changed it when I discovered this little fact.)

fago’s picture

Status: Needs review » Needs work

Unfortunately, I was not able to test this with latest the search-api + its views patch. I kept getting this error:

An error occurred while trying to index items on index node_search: No field settings for index with id node_search.

I got it with the default index as well as with newly created index.

+++ b/views/entity_views_example_query.php
@@ -0,0 +1,78 @@
+  /**
+   * Returns the according entity objects for the given query results.
+   *
+   * This is compatible to the get_result_entities() method used by Views.
+   *
+   * @param $results
+   *   The results of the query, as returned by this query plugin.
+   * @param $relationship
+   *   Optionally, a relationship for which the entities should be returned.
+   * @param $field
+   *   Optionally, the field for which the entity should be returned.
+   *
+   * @return
+   *   A numerically indexed array containing two items: the entity type of
+   *   entities returned by this method; and the array of entities, keyed by the
+   *   same indexes as the results.
+   */
+  public abstract function get_result_entities($results, $relationship = NULL, $field = NULL);

This does not tell me that my function has to implement the logic to apply data-selection relationships. Also, you've dropped the helper for doing so from the current patch? But that's needed by everyone who wants to make use of this.
I think we should provide a helper function resolving the relationships, which one can easily make use of there. Then in the helper, we can call get_result_entities() without the relationship again. So in get_result_entities() you really only have to care about loading your own entities // invoking the helper.

From search-api-patch:

+        elseif ($property instanceof EntityListWrapper) {
+          foreach ($property as $item) {
+            $results[$i] = $item;
+            $type = $item->type();
+            break;
+          }
+        }

That way we cannot change the item being used afterwards anymore - so what about adding a :0 to the selector of lists? That way, we should be able to remove that special case of the selector application code but just rely upon the recursion for applying this and gain the flexibility of possibly making that an option later on. Also, it would make data selectors to look equally to what rules uses (consistency++).

However, as Views itself (completely undocumentedly, of course) passes the field name as the third parameter, this is actually a 100% compatible, so no worries there. (I previously had the complete handler as the third parameter, which would have allowed greater flexibility, but explicitly changed it when I discovered this little fact.)

Oh, I think that's unintentional. Let's fix it in the Views issue.

+++ b/views/handlers/entity_views_field_handler_helper.inc
@@ -0,0 +1,389 @@
+      list($handler->entity_type, $entities) = $handler->query->get_result_entities($values, $handler->relationship, $handler->real_field);

Still, shouldn't be the entity always the same regardless of the field passed? If so, that parameter doesn't make sense in case of get_result_entities().

drunken monkey’s picture

Unfortunately, I was not able to test this with latest the search-api + its views patch. I kept getting this error:

An error occurred while trying to index items on index node_search: No field settings for index with id node_search.

I got it with the default index as well as with newly created index.

Can't reproduce that and is also completely unrelated. Might have to do with #1308638: Reduce size of stored index settings.

This does not tell me that my function has to implement the logic to apply data-selection relationships.

Neither does the documentation of the original method in the Views module – which you yourself wrote, if I might remind you:

  /**
   * Returns the according entity objects for the given query results.
   */

I do think my variant is a large improvement already. But yeah, added that.

That way we cannot change the item being used afterwards anymore - so what about adding a :0 to the selector of lists? That way, we should be able to remove that special case of the selector application code but just rely upon the recursion for applying this and gain the flexibility of possibly making that an option later on. Also, it would make data selectors to look equally to what rules uses (consistency++).

Come on now, if having this work correctly is a useless feature (or not worth the 20 additional lines of code), then the same has to go for being able to poorly emulate the correct behaviour. Not to mention your „Views doesn't intend such use“ argument. Or the „Is there a use case?“ question.
Appending :0 for lists would maybe mean a consistency++, but also a compatibility-- as Search API doesn't use this, meaning that we'd break all related fields in views.

Still, shouldn't be the entity always the same regardless of the field passed? If so, that parameter doesn't make sense in case of get_result_entities().

Not if the field is, e.g., author:roles. Then we have to return the user object, while for, e.g., type we return the node (if there is no relationship, or it leads to a node).
Changing this would further complicate the code, so I think this is the better option.

drunken monkey’s picture

Status: Needs work » Needs review
fago’s picture

Neither does the documentation of the original method in the Views module – which you yourself wrote, if I might remind you:

and which you've reviewed..?

Appending :0 for lists would maybe mean a consistency++, but also a compatibility-- as Search API doesn't use this, meaning that we'd break all related fields in views.

I see.

Not if the field is, e.g., author:roles. Then we have to return the user object, while for, e.g., type we return the node (if there is no relationship, or it leads to a node).
Changing this would further complicate the code, so I think this is the better option.

I still don't get that, sry. Which entity should it return else? Doesn't it always return the entity *in* which the field is contained?

fago’s picture

Can't reproduce that and is also completely unrelated. Might have to do with #1308638: Reduce size of stored index settings.

Tried it again, using latest dev versions of search_api and search_api_db, including views patch and the follow-up fix of that issue (minus the conflict in the views integration). Still no luck - looks like a db specific thing.
So I gave it a test using solr and did not experience any troubles. :-)

So let's clarify the remaining open question about the get_result_entities() parameter. In my test removing it from the only single call did not create any problems. Please enlighten me.

drunken monkey’s picture

I still don't get that, sry. Which entity should it return else? Doesn't it always return the entity *in* which the field is contained?

Example: a view on a node index.
Two fields: Node: Title (title) and Node: Author » Name (indexed) (author:name).
No relationships needed.

Now, in the pre_render() for the field handlers, the following are called:
For Node: Title: get_result_entities($values, $relationship = NULL, $field = 'title')
-> nodes are returned (as title has no colons -> is a direct property)
For Node: Author » Name (indexed): get_result_entities($values, $relationship = NULL, $field = 'author:name')
-> users are returned (corresponding to the author property of the nodes)

The same would go for a view on some other index, on entities with a node reference node. When you add the relationship node and again the above two fields, the calls would now be get_result_entities($values, $relationship = 'node', $field) -> with a „direct property“ (no colons), nodes would be returned; otherwise, the users would be returned (the prefix of the property, up to the last colon, is suffixed to the relationship selector).

fago’s picture

Thanks, finally d.o. is working again so I can reply.. ;)

I've added this to the docs, and improved docs little bit. Interdiff attach - does that work / is it correct? If so patch (Git repo) is RTBC from my side.

drunken monkey’s picture

Still works perfectly for me.
Maybe let Matthias give it another whirl, too, but other than that I'd also call it RTBC.

mh86’s picture

Status: Needs review » Needs work

I can't see multi loads for my base entities, instead each one is loaded separately. Does that work for you?
Everything else seems to work for me.

fago’s picture

Status: Needs work » Fixed

We had a look at that and figured out the problem lies in the search api implementation of get_result_wrappers(). Discovered a bug in get_render() though, which used a not initialized variable - fixed that.

-> Committed, thanks!

mh86’s picture

:-)

das-peter’s picture

Absolutely awesome! Thank you all for this work!

amitaibu’s picture

Maybe anyone write a short issue summary? If I understand correctly this allows modules such as Search API to not write their own field handlers, but to re-use existing ones?

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

khan2ims’s picture

Status: Closed (fixed) » Needs review

Status: Needs review » Needs work

The last submitted patch, 41: 1266036--views-field-handlers-41.patch, failed testing.

drunken monkey’s picture

Issue summary: View changes
Status: Needs work » Closed (fixed)

What in the name of German Alternative Rock group Falco …