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. :-/
Comment | File | Size | Author |
---|---|---|---|
#97 | entity_views_interdiff.patch | 3.29 KB | fago |
#92 | 1266036--views-field-handlers-92.patch | 66.41 KB | drunken monkey |
#90 | 1266036--views-field-handlers-90.patch | 63.39 KB | drunken monkey |
#89 | entity_views_interdiff.patch | 1.03 KB | fago |
#86 | 1266036--views-field-handlers-86.patch | 67.88 KB | drunken monkey |
Comments
Comment #1
fagodrunken 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)
Comment #2
drunken monkeyClarification: 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.
Comment #3
drunken monkeyAdded 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.
Comment #4
marcoka CreditAttribution: marcoka commentedsubscribing
Comment #5
drunken monkeyThe 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 circumventget_result_entities()
in the case of non-entity data (paradox use case for an Entity API, but still …).Comment #6
Pedro Lozano CreditAttribution: Pedro Lozano commentedSubscribing.
Comment #7
drunken monkey@ 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
Comment #8
fagook, I did a first short review.
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.
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.. ;)
Comment #9
drunken monkeyDone, 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.)
Comment #10
jakonore CreditAttribution: jakonore commentedsubscribe
Comment #11
tnightingale CreditAttribution: tnightingale commentedsubscribe
Comment #12
drunken monkeyAnd 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 remainsNULL
.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)?
Comment #13
drunken monkeyOK, 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.Comment #14
drunken monkeyOK, 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.).
Comment #15
marcoka CreditAttribution: marcoka commentedhm? 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.
Comment #16
drunken monkeyAh, 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).
Comment #17
marcoka CreditAttribution: marcoka commentedi 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?
Comment #18
marcoka CreditAttribution: marcoka commentedno more errors with latest search api patch. some mismatch with the entity.info but resolved that by hand (only some includes).
Comment #19
drunken monkeyRevised patch attached.
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.Comment #20
joachim CreditAttribution: joachim commentedHeh ;) 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.
Comment #21
joachim CreditAttribution: joachim commented> - 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()?
Comment #22
drunken monkeyI 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.
Comment #23
joachim CreditAttribution: joachim commentedI'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.
Comment #24
drunken monkeyPlease 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.
Comment #25
joachim CreditAttribution: joachim commentedThanks 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()?
Comment #26
das-peter CreditAttribution: das-peter commentedsubscribing
Comment #27
jsacksick CreditAttribution: jsacksick commentedsubscribing
Comment #28
drunken monkeyDiscovered 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.
Comment #29
marcoka CreditAttribution: marcoka commentedi 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.
Comment #30
fagook, I've done another review.
Hm, I don't see why we need that function. Using entity_metadata_wrapper() with a list data type would achieve the same.
Let's make use of existing terminology and call this "data selector", thus it should take an $selector argument.
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?
Variable names have to make sense *always*. We should never use abbreviations like that.
@see should be below @return.
Do we really have to support mutiple nested lists here? Usually, we don't really care about that.
Let's roll a separate patch for that, as we can commit that asap.
Please always use $property_info if it's actually info. Else one could be easily confused.
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.
Wow, is that working? Sorting without any database?
Wrong comment style.
Why isn't it possible to implement the regular orderby() method?
Ouch.
That's fixed.
Comment #31
joachim CreditAttribution: joachim commentedSince 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 :)
Comment #32
drunken monkey@ joachim: Done.
@ Wolfgang:
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.
Do you mean I should replace „property selector“ with „data selector“ everywhere, or only the two parameters you quote?
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).
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.
I don't really understand what you mean there. Did you mean to quote the same part as joachim in #31 ?
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.)
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.
Why? This is just an implementation note, not a doc comment. Or do you mean something else?
Because the regular
orderby()
method receives completely different (and misleading) parameters.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.)
Comment #33
drunken monkeyHm, 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.Comment #34
drunken monkeyUpdated patch.
Comment #35
ZuluWarrior CreditAttribution: ZuluWarrior commentedHi 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.
Comment #36
fagoEverywhere - the terms we use should be consistent.
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.
ok.
Let's roll a separate patch for the entity-type-key-location fix.
Oh, of course. (In which case solr is our "database")
Then it should be inside the function. Anyway, I think it's fine to have in the doc comment.
Still, the function does not make much sense to me. NULL should be perfectly fine in list-wrappers.
Comment #37
joachim CreditAttribution: joachim commentedCould 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.
Comment #38
drunken monkey@ ZuluWarrior:
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.
Comment #39
mh86 CreditAttribution: mh86 commentedTested 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.
Comment #40
ZuluWarrior CreditAttribution: ZuluWarrior commentedThanks 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....
Comment #41
drunken monkey@ 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?
Comment #42
dawehnerJust from convention it should be entity_handler_field_boolean.
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.
This would be indeed nice.
Alternative you could define a options callback and get the options there. See views_handler_filter_in_operator
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.
As above using a short name doesn't really help to make it more readable.
This is just a review of the first part.
Comment #43
drunken monkeyAh, finally some Views expert in this issue! ;)
Thanks for reviewing, I'll post a revised patch tomorrow!
(Further comments of course still welcome.)
Comment #44
tnightingale CreditAttribution: tnightingale commentedI get the following warning:
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:
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.
Comment #45
mh86 CreditAttribution: mh86 commentedpatches from #41 or #44 broke many of my existing views with errors like following:
Comment #46
fagoHm, I'd prefer having views integration under the entity_views namespace. handler is not a reserved keyword for views ;)
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.
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.
Comment #47
Pedro Lozano CreditAttribution: Pedro Lozano commentedDoes 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.
Comment #48
drunken monkeyIt takes care of
entity_views_data()
, but not the nestedentity_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.
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?
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.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?
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.
Comment #49
mh86 CreditAttribution: mh86 commentedYes, this patch definitely breaks views that list messages. The Message module is based on the EntityDefaultViewsController, so maybe something has changed?
Comment #50
das-peter CreditAttribution: das-peter commentedJust 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
Comment #51
fagoMaybe we should give an example with multiple nested lists here, as this is when you need the function.
Let's point to entity_views_table_definition() for more info about that using @see.
Let's document what the views integration is about and how it works (using the objects...).
Let's comment why we statically cache it.
Ok, why not.
If they do, it's there fault. Let's don't check for silly stuff like that.
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.
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.
data selectors?
How would that be possible? Do we really need that?
Label and machine-name should basically use the same name.
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.
Checking for the first row should do it, no? All rows should look equally.
Don't silently fail. Let's just use get_result_entities else.
Do we need both the entities and the wrapper at the handler? I guess the wrappers should suffice.
Let's provide an example selector and explain how it handles lists.
$return
$current...
This function does not override anything.
What's the entity object special case?
This might throw an exception.
As this is usable without entities too, maybe we should call _entity_properties "_wrapper_values"? Thoughts?
I see. Does that work with values already in _entity_properties ?
$return
cache NULL. WTF?
Again, let's don't provide lame defaults. Assume the method is there and call it. If not, the developer needs to see errors.
and are not part of the selector, right? Maybe add an example.
Maybe we should add a commen that we want to use entity_load() for multiple loading.
Oh, this one overlaps with http://drupal.org/node/1209678. Let's get the other one in first, then re-use it here.
No comma here.
Don't add workarounds - let's rely upon the fix to arrive soon.
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.
Comment #52
drunken monkeyNope, nothing. Except for adding the "entity type" to tables.
So, again, please find out what could cause this.
Fixed it – but no, please mention problems here directly. I don't think this issue is complex enough to warrant its own issue queue.
Should, but needn't. And as you have maybe noticed, I'm in favor of correctness instead of brevity.
A special case to get the entity object.
No. If handlers need the entity object regardless of the "link_to_entity" option, they have to pass
$load_always = TRUE
.If the handler's normal field is retrieved,
$field = NULL
is passed toget_value()
. Hence, we need to cache the value for that case, and that seemed to me the more intuitive variant.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?
Comment #53
tnightingale CreditAttribution: tnightingale commentedI 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:
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:
Patch attached.
Comment #54
fagook, 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.
What would be other values one can pass in? Different selectors? Might there be a name-conflict with them?
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] ?
I see, we should add some docblock comment to explain that parameter.
!?
I don't get what this cache thing does at all? Could you explain it for me/us? :)
I see - yep, in that case the comment shouldn't say it's lame :)
Comment #55
drunken monkeyYes, different selectors. And since they don't contain spaces, I think we can preclude conflicts.
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?
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
Comment #56
drunken monkeyIf 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.
E.g.,
foo_bar:baz
andfoo:bar_baz
would otherwise get the same alias. So yes, if we want to avoid bugs, we do need that.I don't really understand what you mean, and also think the quote you selected for that wasn't the right one?
_wrapper_values
wouldn't be namespaced. I'd stick with_entity_properties
, but in the end it's of course up to you.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.
Comment #57
drunken monkeyHere is the updated patch, hope I didn't forget anything.
By the way, maybe a new „Views integration“ component for issues would be appropriate?
Comment #58
mh86 CreditAttribution: mh86 commentedtested 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
Comment #59
drunken monkeyfago, do you have any idea what part of this patch could influence the Messages module?
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 theforeach
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 inpre_render()
could also help.Comment #60
fagoThanks. 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".
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.
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'm referring to the glue - vs. separator terminology what isn't consistent across machine-names and labels.
Indeed, ok so let's stay with that.
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.
Comment #61
fagoouch yes. I think it's caused by https://drupal.org/node/1287240 - thus unrelated.
Comment #62
drunken monkeyI 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.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.
Comment #63
mh86 CreditAttribution: mh86 commentedThanks 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?
Comment #64
drunken monkeyHm, 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.
Comment #65
drunken monkeyAh, missed one paragraph in my reply:
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.
Comment #66
drunken monkeyPatch 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.
Comment #68
drunken monkeyWow, nice! :D
Comment #69
fagoAt 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?
Comment #70
drunken monkeyAh, 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).
Yes, might be a good idea, didn't think of that.
Comment #71
drunken monkeyWhile 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 ofvalue()
.Comment #72
fagoWhat's wrong with having a wrapper only containing an id? It won't load the entity if not necessary.
Sounds like a good way to go.
Comment #73
das-peter CreditAttribution: das-peter commentedLooks 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
Comment #74
drunken monkeyYes, 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.
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.Comment #75
das-peter CreditAttribution: das-peter commentedTomas: I'm slapping myself right now - for being a douche not searching in the patch to look if it changed something there.
Comment #76
mh86 CreditAttribution: mh86 commentedThere 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.
Comment #77
fagoIt only loads the entity if you call value() but didn't tell it to get the id only.
There is nothing to fix. E.g.
does not cause an entity-load.
Comment #78
drunken monkeyLook 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?
Comment #79
fagoI 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"
can be additionally added.
Let's remove the "get" from the name - we usually don't have that in drupal hooks.
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".
Let's insert our name for the tables here.
Great work on the docs here! Maybe, we should point to the search-api as example though too.
We still need to solve this. I fear suddenly breaking Views configurations as soon as some properties/fields are added else.
Let's insert our name and the pointer to the docs here.
I'd call that:
A numerically indexed array containing two items: foo, bar.
see above
hm, both are defined via Views, not?
So maybe just say:
"including entity data selection relationships."
This might throw an exception too.
There is no comma before a that.
So as we require only get_result_entities() now our handlers are finally compatible with every entity table, right? That's great stuff!
no comma here.
insert our name.
Comment #80
drunken monkeyShould contain all requested improvements.
Comment #81
das-peter CreditAttribution: das-peter commentedStrange, 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?
Comment #82
drunken monkeySorry, should work now.
(Confusingly, Git allows pushing a tag without pushing the commit it is tagging.)
Comment #83
das-peter CreditAttribution: das-peter commentedThanks Tomas - it's working :) I'll test our current stuff with the latest patch asap.
Comment #84
fagoPatch 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.
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:
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:
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:
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.
Comment #85
fagook, 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.
Comment #86
drunken monkeyI couldn't find such a field. However, I've made some changes/improvements/fixes now in that area, so perhaps it works now?
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).
"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.
Comment #87
drunken monkeySettings status to „needs review“, even though „is beyond all doubt and should be committed immediately“ is more like it.
Comment #88
fagoOuch. 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.
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:
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
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".
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):
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.
Comment #89
fagook, 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.
Comment #90
drunken monkeyI 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()
(andget_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.)
Comment #91
fagoUnfortunately, I was not able to test this with latest the search-api + its views patch. I kept getting this error:
I got it with the default index as well as with newly created index.
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:
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++).
Oh, I think that's unintentional. Let's fix it in the Views issue.
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().
Comment #92
drunken monkeyCan't reproduce that and is also completely unrelated. Might have to do with #1308638: Reduce size of stored index settings.
Neither does the documentation of the original method in the Views module – which you yourself wrote, if I might remind you:
I do think my variant is a large improvement already. But yeah, added that.
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.
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.
Comment #93
drunken monkeyComment #94
fagoand which you've reviewed..?
I see.
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?
Comment #95
fagoTried 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.
Comment #96
drunken monkeyExample: 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 relationshipnode
and again the above two fields, the calls would now beget_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).Comment #97
fagoThanks, 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.
Comment #98
drunken monkeyStill works perfectly for me.
Maybe let Matthias give it another whirl, too, but other than that I'd also call it RTBC.
Comment #99
mh86 CreditAttribution: mh86 commentedI 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.
Comment #100
fagoWe 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!
Comment #101
mh86 CreditAttribution: mh86 commented:-)
Comment #102
das-peter CreditAttribution: das-peter commentedAbsolutely awesome! Thank you all for this work!
Comment #103
amitaibuMaybe 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?
Comment #105
khan2ims CreditAttribution: khan2ims commented41: 1266036--views-field-handlers-41.patch queued for re-testing.
Comment #107
drunken monkeyWhat in the name of German Alternative Rock group Falco …