Since now #975400: Refactor field render() functions to accept $value, allowing reuse finally got into Views 7.x-3.x, we can now adapt the Search API field handlers accordingly. (Or wait whether Entity API will implement field handlers as well …)
Not only can we save much code in the render() methods, we could now also rather easily inherit with each handler individually from the respective Views field handler, instead of inheriting from the SearchApiViewsHandlerField class, which will only receive the most basic options from the Views base field handler.

Comments

Shadlington’s picture

This is awesome :D

Subbing

Shadlington’s picture

Hey, quick question: If a separate module implements a views handler (e.g. Date, Location, etc.) then would the relevant code for Search API views handling be best living in Search API or the relevant module?

drunken monkey’s picture

There is already a handler for date fields – if someone wanted to improve it, and the improvements are not for their specific use case, it can of course go into the Search API project. The same goes for other general improvements for the standard types.
Things like location handling, on the other hand, which aren't part of the standard types, should go into the other module.

Shadlington’s picture

I meant the handler as it occurs in the Date module.
With all the fancy date-handling that happens in that module.
If a Search API version were to be implemented it'd almost certainly be dependent on Date.
So that's what I was referring to - if someone implemented a handler that was dependent on Date, where would it live?

drunken monkey’s picture

That's a good question (although unrelated to this issue). As the Date module probably wouldn't want it, either: probably in a third module, that could maybe be kind of a collection of such extensions.

Shadlington’s picture

Ah, sorry. I'm not really sure how all this views integration works (or maybe I'm not explaining what I mean very well) and I just thought that this issue might also apply to views handlers implemented by other modules.

becw’s picture

StatusFileSize
new5.19 KB

I'm interested to see where you take this. I did some work on a patch that updates Search API Views' results array to contain objects (as Views expects) instead of arrays, and to populate the 'field_alias' property on handlers.

It may make sense to try and build off of views_handler_field_prerender_list, which implements list handling and allows you to run each list item through views_handler_field::render_text(), which is handy. However, the "advanced render" workflow on Views handlers makes me crazy, so that is not addressed in this patch at all.

drunken monkey’s picture

Thanks, that comes in handy! Seems to be fine, too, I'll use it in my eventual patch.
And I can't blame you for not diving into the advanced rendering stuff – it's really a mess, and I fear having to deal with it, myself.

becw’s picture

StatusFileSize
new5.75 KB

One thing that would be extremely useful for me would be if Search API Views didn't effectively strip out all of the properties returned in the search results in favor of properties loaded via SearchApiViewsQuery::extractFields(). I would favor an approach where the search result data was directly present in each views result, entities were loaded (as they are now) into the 'entity' key, and extracted field values were loaded into a key like '_fields'. It is straightforward to accommodate this change in SearchApiViewsHandlerField.

Here's a modification of the patch from #7 with this additional change.

davidseth’s picture

@becw #9 - I fully agree with this. It would be incredibly useful to include this information. I have been working on allowing for the normal Search API Views activity to pull in just solr results and not hit the database at all (to do this I changed the schema to store all fields).

I am still working on this, but it would be great if

Search API Views didn't effectively strip out all of the properties returned in the search results in favor of properties loaded via SearchApiViewsQuery::extractFields()

drunken monkey’s picture

First off: The patch doesn't correctly extract the score.
Then, I don't really see the benefit of this approach versus storing the field information in the "entity" key. That's the way "returning properties in the search result" is currently supported across the Search API, and this would already be possible without the need for another patch. And it doesn't touch the database either. So, please an explanation why this would be "incredibly useful" and can't be already done.
Also, even if we should allow that, I don't think the patch does this the right way. As I see it, the fields are still all extracted and take precedence over the data returned in the search results? Shouldn't only the fields not already set be extracted, and then only be used if the property wasn't set directly? Otherwise, this would require dedicated handlers that check themselves whether the data is available directly on the results instead of using get_value(). Or am I misreading the code?

becw’s picture

With the "getfieldnames" patch (#1138230: Add increased flexibility to the service class), any solr result property (say, "score") can be mapped back to a different property name in the result set (perhaps, "search_api_relevance"), or can be passed through with the same name. I've seen uses of both property names through the Search API code. This way, the search results from the service can be passed through to Views without reprocessing every value. Entity API can be used to gather values that are not present in the service search results.

The problem with returning values in the "entity" key is that they have to be passed through an Entity wrapper--each one has to be fully described to the Entity API in order to get it in the search results. For a value that is already present and requires no further processing, this is arduous. Perhaps I am misunderstanding the behavior of the 'entity' property that is already present.

I'm not sure that this patch does this correctly. It is an only an idea--one that works for me, but that is also incomplete. Depending on where you look in Views, this kind of thing is done in several different ways. In views_handler_field_field, the default Views field handler for Field API fields, entities are loaded in the ::pre_render() method. The point of the ::get_value() method is to accommodate this.

drunken monkey’s picture

Ah, yes, that does make sense.
I'll keep it in mind when doing this.

drunken monkey’s picture

becw’s picture

drunken monkey’s picture

Oh, nice! Thanks for spotting this!

becw’s picture

Quoted from #1138230-14, Add increased flexibility to the service class:

And yes, I agree I should then document the optional "fields" key for search results. And keep this in mind for the Views handlers, first looking into "fields", then "entity" and finally doing the default.

It might be worth considering putting all field values Views results objects directly and using Views field handlers' "field_alias" property to deal with conflicting field/solr result/Search API result property names, rather than building out the Views result objects as a custom data structure. This would let Search API Views query handlers avoid implementing even ::get_value().

drunken monkey’s picture

In my opinion, we should just use all field values that are provided in $result['fields] and only use the entity and wrapper to extract those values that aren't present there. If this isn't, what you meant (not sure). Not having to implement get_value() is especially important since after #1077148: add an entity-view row plugin lands I might even remove my own field handlers in favor of those, so there wouldn't be the possibility to implement special logic there.

But thanks for reminding me, have to document this …

becw’s picture

StatusFileSize
new6.69 KB

Yes, that's what I meant. Perhaps something like the attached patch would work? However, it doesn't deal with conflicting property names--an entity property called 'score' would be overridden by the search result 'score'.

drunken monkey’s picture

As far as I can see, this would still load all entities and extract all their fields, even if all necessary fields are set in the search results.

becw’s picture

StatusFileSize
new6.79 KB

Good point. Here's an updated patch with a check for whether all the requested fields have been returned in the search result; it will only attempt to extract missing fields.

Also: this could handle the special case added in #1147466: Provide excerpt in Search pages and Views, at least for solr, if 'search_api_excerpt' were mapped to 'excerpt' in the service's result handling. I noticed that the 'excerpt' key isn't documented as part of the search result array; perhaps it should be?

drunken monkey’s picture

Good point. Here's an updated patch with a check for whether all the requested fields have been returned in the search result; it will only attempt to extract missing fields.

Thanks, but this still loads all entities. You should first check which fields are provided for which result, and then only load those entities that don't have all fields set.

Also: this could handle the special case added in #1147466: Provide excerpt in Search pages and Views, at least for solr, if 'search_api_excerpt' were mapped to 'excerpt' in the service's result handling.

That doesn't really help, as we would still need to handle it for all other backends and exceprt sources. Apart from Solr not returning an "excerpt" field.

I noticed that the 'excerpt' key isn't documented as part of the search result array; perhaps it should be?

It is, you must have overlooked it.

becw’s picture

Currently, each view result contains a fully loaded entity. Do you want to change that?

drunken monkey’s picture

Ah, you're right of course. That would mess up the "Entity view" row style, and maybe other things too.
Sorry, my mistake. We should of course load the entities in any case. The gain from not extracting all the properties should probably still be noticable, though.

becw’s picture

What else is needed need to fully address this issue? Do the field handlers need to be reviewed and revised? I'm relying on parts of this patch, so I'm willing to help with the parts that I'm not relying on.

drunken monkey’s picture

My plan was to wait for #1077148: add an entity-view row plugin to land and then see how we could use those field handlers.
However, maybe we can commit the existing patch beforehand and then retouch it after the other patch has landed. I just have especially little time this week, so didn't get round to reviewing the patch again to see if it could already be commited.
Otherwise, I'd suggest your help would probably be most appreciated in the other issue, which seems a bit stuck. How urgent is this issue for you?

becw’s picture

I've added my two cents to #1077148: add an entity-view row plugin and will continue to keep an eye on it.

This is not urgent (clearly, based on my response time :) but if you get a chance to review this patch, I'll appreciate it.

drunken monkey’s picture

I've talked to fago, and he thinks about not requiring query backends to put the entity in a certain place, but just introducing a method on the query class to return the entity. This would allow us to skip loading the entities when otherwise unnecessary and just store their IDs.

So, if this isn't urgent for you either, let's just wait for this to clear up and then do the whole revision at once.
But apart from this, your patch would be good to go, as far as I can see.

drunken monkey’s picture

Status:Active» Needs review
StatusFileSize
new7.59 KB

OK, latest development: This has now become urgent for me, so gonna commit this soon.

However, as I see it, now that we'll probably get an extra method for retrieving the entities, we can avoid loading them, unless necessary. The attached patch should do this. (Already tested with various combinations of "fields" and "entity" values.) It only breaks compatibility with something that wasn't committed yet – and probably won't be, either.
Also, it re-adds some fields that I just realized were missing in your patch (excerpt and score).

becw’s picture

Status:Needs review» Reviewed & tested by the community

I don't like that the value of the 'entity' property on Views results is variable, but it seems like the plan is to address loading entities consistently in #1172970: provide a unified way to retrieve result entities. Otherwise, this looks good, and works for my uses as long as I tweak my field handler to load entities.

drunken monkey’s picture

Status:Reviewed & tested by the community» Active

Yes, that's not optimal, but as you say, it's only provisional. Hopefully, we can soon change that to a unified system across all backends.

Committed the patch, but leaving active as there is of course still some work to be done.

drunken monkey’s picture

Moving discussion over from #1154116-5: Search API Solr retrieving search results data directly from SOLR, avoid going through MySQL:

@drunken monkey: perhaps the 'retrieve_data' flag from your patch in #4 should be used to trigger the new Search API Views behavior?

No – we can hardly make the Views integration depend on such a backend-specific setting.

The rest is a valid point, though, we should really think about fixing this.
On the other hand, extracting the data out of the entities still takes some time and we have to assume that people who want to load the data directly from the server know what they are doing, and what they want.
Additionally, the "fields" key is specified as containing extracted field data, not necessarily from the search server. So we can't really assume it in the Views integration, even if this is at the moment the case for the only (known) usage of this key.
In the end, I think we have to assume that if there are fields returned along with the results, this is what the user (i.e., site admin) wants to be used as the field data.
It might even be that the field data returned is already formatted in some way the user wishes, so replacing it with fresh entity data is really undesirable. It could also impede integration of non-entity searches later, which the "fields" key seems to support quite well.

Shadlington’s picture

I am not 100% but it sounds like you were waiting for #1172970: provide a unified way to retrieve result entities to be committed before carrying on.
If so, that has happened so I guess I'm bumping this as a reminder :)

drunken monkey’s picture

Yes, thanks. Now I just have to find the time, in September. The plan has actually expanded quite a bit, now I'm looking at implementing entity base tables in the Entity API, along with the field handlers, and then use that to implement field handlers and relationships (#1231512: Use real Relationships instead of level magic in Views integration) in the Search API.
So, quite some work still to do—and before the 1.0 stable release, too. :-/

drunken monkey’s picture

I think, the new field handlers would be generic enough to live in the Entity API. I created an issue there with my (half-baked) plans: #1266036: Add generic Views entity tables with fields and relationships.

After that is done, it's just a matter of using this with the Search API, maybe extending field handlers where necessary, and even maybe-er writing an update function for this stuff.

jakonore’s picture

subscribe

drunken monkey’s picture

Status:Active» Closed (duplicate)

Even though this issue was here first, all work is now happening over in #1231512: Use real Relationships instead of level magic in Views integration, so finally marking this one as a duplicate to avoid confusion.