Example: A facet on node->author->uid would display the user names as the facet links, even though the uid is what is used for the filter.

An open question is whether this should always be done or only be an option for facet blocks of entity IDs (maybe enabled by default). Flexibility is generally a good thing, but on the other hand we could always just add the option later, if people complain. ;)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

I'd say if someone indexes "node->author" it should just index the id, but show the label. But if one selects "node->author->uid" I think it should just leave the id as is.

drunken monkey’s picture

You can't index node->author at the moment, and changing that would be quite a lot harder than the change proposed here. That's why I suggested it (i.e., the latter) in the first place, since I think it's a good solution.
Also, the alternative would change the core API just for the sake of one extension, without any effect on all other modules. It would only make a difference to the facet module, whether node->author or node->author->uid are indexed, the exact same data would be indexed and displayed everywhere else — so why not restrict the change to the place where it actually has an effect?

fago’s picture

I see that it's much harder, still I think its the best thing to do as from a UX point it absolutely makes sense, while no one would think that adding "uid" makes that possible.

Also, it would be nice to have the search api dealing with entity-references in other cases except facets too, thus I'd expect an entity reference to behave like that:
* for fulltext use, only the entity_label() would be indexed. That way term-names, or author-names or for referenced nodes, the referenced node titles could be used in the search. Sounds useful to me.
* else I'd expect the identifier to be indexed. That way it could be properly filtered for it, maybe via a facet, while the display should make use of the entity_label. For exportable entities being referenced, I'd suggest using the machine name as that ensures exports still behave properly. E.g. if you export a view filter for a certain profile type, the export would work just fine after import.

So, maybe search api could just treat the entity reference as the entity id, i.e. converting it to string/integer before passing it further internally? Then I guess we could solve the labelling problem generically at the entity API level, so fulltext/display could just rely on that.

drunken monkey’s picture

OK, you've certainly got a point there. I'm still not lucky adding a new way to functionality that is largely already there, but I guess it does make sense from a UI perspective …
I'll see what I can do …

BenK’s picture

Subscribing

drunken monkey’s picture

Status: Active » Needs review
FileSize
20.51 KB

Managed to do this (I think) with as little disturbance as possible. See attached patch.

Entity-typed fields can now be indexed directly, "Add related entities" is renamed to "Add related fields". Internally, the fields are handled (for the most part) just as if it was the ID, and not the whole entity. Most importantly, the ID is indexed. There are only two places (currently) where there is special handling (other modules could add their own):
- Facets will show the entity labels instead of the ID. (Since this necessitates entity loads for all facets of that field, this could harm performance, though.)
- When displaying as a Views field, there is an option to use the label instead of the ID. Also, the "Link to entitiy" option will link the field to the right entity (i.e., the field value, not the "parent" result entity).

Internally, entity-typed fields can be spotted by the "entity_type" key that is present in addition to the "type" key, in the index's field options.

@ BenK: Wow, that's what I call timing!

drunken monkey’s picture

Now with additional "Current search" block support!

Shadlington’s picture

FileSize
20.95 KB

Just started testing this patch out.
My initial thought on seeing the revised fields page was 'wow, this is a lot more intuitive'.

Really didn't get very far though, I got hung up on a little issue that was bothering me and now its late and I'm going to bed.

But I thought I would post the problem anyway... The taxonomy term fields have no options on them on the field screen now. Or at least, I think that is what is intended, but it seems there's an odd little glitch causing the 'boost' select box to show up for taxonomy terms if there is a field above them that has a 'boost' select box too. I'm very tired and not explaining this very well, so there's a screenshot attached :)

I'll be trying this out properly tomorrow.

drunken monkey’s picture

Thanks for spotting this! The attached revised patch should fix this. It's pointless to define boosts for taxonomy IDs anyways …

Shadlington’s picture

That corrected patch does indeed stop the boosts showing.

Right, now I'm going through this a bit more thoroughly I'll keep track of the things I'm finding.

I should note that where I was previously adding related entities like author and taxonomies and then indexing their names, I am now indexing the fields 'author' and [insert_taxonomy_name_here], which I understanding is indexing their IDs.
My assumption is that this will allow me to do pretty much everything I could do when I was indexing the names, just more intuitively.

I'm just going to document how everything went as I started using the patched module:
(Only 1, 4, 5, 7, 8, 9 and 10 are problems)

1. The views handler broke on my existing taxonomy >> name and taxonomy >> url fields... Not bothered about this one as I said above I will not be using anymore.
2. Created a new solr index - everything on the index's config pages is working fine now. Added facet for author, content type and a taxonomy term.
3. Created a search page & enabled facet blocks
4. Noticed that facet blocks created since the patch are missing the 'Display for searches' options.
5. Noticed that the facet block for the taxonomy term appends on a '-' to be beginning of child terms, despite not actually displaying according to hierarchy. Not a big issue but this doesn't happen on the taxonomy >> name facet block I already had.
6. Search page works fine. Creating a table-style view.
7. When adding the author as a field I got 'Broken/missing handler'. Filter + Sort handlers were fine for it though, and I was able to add author >> name as a field instead.
8. The 'link to entity' item is still linking to the indexed node rather than the related entity. I'd expect that from the author >> name field but it is also happening for the term field.
9. Just noticed something about the facets + current search block. I can see that the facet blocks appear to be taking their title (on the search page, not on the block admin page) from the field they represent (so I get Author instead of Index Name: Author) but this isn't reflected in the current search block, it still has the index name.
10. Tested out exposed sorts. Seem to work, mostly. Sorting on the taxonomy term (and author, so presumably all ID fields) sorts by ID rather than label still.

Soooo mostly little things. This patch really is brilliant - it makes the module a lot more simple and user-friendly.

This isn't an exhaustive test, obviously, but I've run out of time. I'll be trying this out more later.

Shadlington’s picture

The handler for image fields is also broken.

EDIT: After pondering it a bit, I don't think I really 'get' the full scope of this patch, what it is meant to achieve. Its good, but I think I was thinking too much of it...

drunken monkey’s picture

Wow! First off, thanks for the great feedback! There are really quite a number of serious flaws in the patch, it seems. I didn't manage to look into all of them, yet, but will continue tomorrow.
Here are remarks to at least some of them:

ad 1: Thanks for spotting this, that was a rather major one – should be fixed with the attached patch.
ad 4: That option is only shown if more than one option is available. The new block has probably just been displayed for only a single search, therefore the option is hidden.
ad 5: This is due to both #1107396: The "view" option list for taxonomy term fields shouldn't have dashes prefixed (Entity API issue) and a little problem in the patch, both of which could separately fix the problem. However, I think both should be fixed, and will update the patch accordingly. (The problem, as with 1, is that I didn't consider that entity-valued fields might still have option lists, as is the case for taxonomy term references.)

OK, have to call it a day for now. Coming soon: more fixes, and a more detailled explanation of what the patch does and doesn't. (Which should probably also be included into the documentation.)

Shadlington’s picture

I'm glad to be of service :)

You're right about #4 - I thought I'd setup a second search but it was set to the wrong index.
I'll try out the new patch in a little while.

Shadlington’s picture

#1 is fixed.
#5 isn't, but I wasn't clear from your response if you meant it to be yet

drunken monkey’s picture

Sorry for the ambiguity: no, #5 wasn't meant to be fixed, yet. It should be now, though, with the attached patch.

ad 7: Can't reproduce – maybe I already fixed that along with #1 or #5.
ad 8: Oh, that was generally rather messy … But should be fixed now (with the help of #1109130: Add better structure for Views field rendering). "Author » Name" (or similar) still linking to the node, not the author, is an outstanding issue I should probably fix some time.
ad 9: This is a different issue. I agree it's not optimal – it currently uses the facet name, instead of the field name. Maybe we should make this an option – instead of a checkbox for "Display field name for active filters" a select that lets you also choose what should be displayed, the field or the facet name. Or just the last part of the field name (without e.g. "Author » " prefixed). In any case: please open a new issue for this.
ad 10: This is by design. Since only the ID is indexed, there is no way to make it sort on the label.
ad "broken image field handler": I guess you mean the "Image: The image file" field? I can reproduce this (fatal error – very bad :-/) but after some analyzation it seems to be a weird Views bug. Or, after closer inspection, maybe it working for all other fields is a bug (or glitch). Can't really tell – for such purposes, Views can be considered undocumented. Additionally, #1057242: entity_uri() should not use $entity->uri (was $file->uri doesn't match the contract for uri callbacks) played into this when checking "Link this field to its entity" – really a mess, that is entirely not my fault. ;) Anyways, should be working now, due to a little work-around / addition.

As for the scope of the patch: This patch merely provides an additional indexable property for each entity-valued field (node author, taxonomy term references, profile2 of users, etc.). Indexing such a field indexes only the entity ID (meaning, as you said, that that will also be used for sorting) but gives a hint to other modules that they should use the entity label for displaying the field in any context.
Since most of what the patch achieves was already possible previously with some workarounds, the patch is meant primarily to solve the problem of renaming: Previously, when you wanted taxonomy term or author facets, you would have to either index and use the name, or hack the theming, to get a sensible display. This would result in problems when a taxonomy term or user was renamed, as the old name would still be indexed – you'd essentially have to re-index the whole node index whenever that happened. Sadly, this is still true for all other properties of related entities, but this patch should at least fix the most common use case. For other use cases, I think that fago is working on a Rules solution to this (allowing you to set up a rule like "when term name changes, re-index all nodes containing that term").

I've also now added a little clarification to the "Fields" page. Does that help, in your opinion? Any way to phrase or present this better?

Shadlington’s picture

#5 Is indeed fixed now

#7 You're correct - It didn't occur to me to retest this after your previous patch revision, but it was fixed then.

#8 Another fixed one!

#9 Ah! I apologise for throwing this in here - for some reason I thought this was behaving differently before I applied the patch and so had also been altered. I'll file a new issue for this.

#10 I had guessed as much after thinking about it again the next day. Its a shame but makes sense (and the clarification you added resolves any confusion).

You're correct about the image field I meant. And you have fixed the error. Even the 'Display label instead of ID' and 'Link this field to its entity' options are working. I wonder then, if the next step with this would be to display the image rather than the ID/Label. If you've got the url already then presumably its doable (and a much more likely use case than the file name by itself). Though this is probably new-issue material, right?

The clarification you added is great, definitely helps. I'm not sure if the asterisk (*) is necessarily required - particularly as I (and I think most people, though I'm not 100% on this) typically look for asterisked notes at the bottom of the page. A simple note at the top seems fine by itself. Maybe with a short list of examples of entity-valued field types (like gave in #15) for those who aren't familiar with what this means.

All in all, this patch is looking very healthy now :)

I'll do a full retest today if I get the time (I have only targeted these specific points for now).

drunken monkey’s picture

I wonder then, if the next step with this would be to display the image rather than the ID/Label. If you've got the url already then presumably its doable (and a much more likely use case than the file name by itself). Though this is probably new-issue material, right?

Yes, this is definitely material for a new issue (and could have been done even without this patch).

The clarification you added is great, definitely helps. I'm not sure if the asterisk (*) is necessarily required - particularly as I (and I think most people, though I'm not 100% on this) typically look for asterisked notes at the bottom of the page. A simple note at the top seems fine by itself. Maybe with a short list of examples of entity-valued field types (like gave in #15) for those who aren't familiar with what this means.

OK, I'll change that.

I'll do a full retest today if I get the time (I have only targeted these specific points for now).

Thanks!

Shadlington’s picture

Retesting this now and I've hit a bug that's a bit weird.
I have a facet based on a taxonomy ID field. 'Show active facet links' is enabled.
The label displays correctly (as the term name) before choosing a facet.
However, after selecting a term, the 'active facet link' just shows the ID.
The label shows correctly in the current search block.
This also happens with the author ID field/facet.

Now the confusing bit is that I created this index just now to re-test the patch with.
However, I still have an index I created after the initial patch (the one in #7).
The index is pretty much identical as far as I can tell.
However, the term/author ID facets work fine for this one still.

I can't see any difference. Not sure what to make of it.

drunken monkey’s picture

OK, now that's plain weird …
I can't reproduce this, not even with a new index, and the behaviour you describe is really strange. Could it be that it is related to your testing of the search_api_facets_paths module?
Or maybe try creating yet another index and see if the problem shows up there, too.
And does the "Current search" block show the labels or the ID? If so, does commenting out lines 357 and 595 in the patched search_api_facets.module file solve the problem, or at least partly? (Too lazy to make a proper patch for this … ;))

But frankly, I'm a bit clueless here. If you don't find something, please post some screenshots to really clarify the problem (although I'm pretty sure I understand it correctly) and I'll try to come up with a patch for you to debug the problem.

Shadlington’s picture

I had the search_api_facets_paths module uninstalled while I tested this.
The current search block correctly displays the label.

No change with those lines commented out.
I'll get back to you on a retest in a minute.

Shadlington’s picture

FileSize
31.53 KB

Oh and here's a screenshot

Shadlington’s picture

I'm really not sure what's going on. I'm going to delete all my indexes and start again.

Shadlington’s picture

Now that I've rebuilt it its still happening.
Did some additional testing.
It now appears that this ONLY happens on that one taxonomy facet.
The Taxonomy >> Name equivalent of the same facet is fine.
As are the other taxonomy term fields I have tried.

Odd, huh?

Still, this has resulted in me doing quite a lot of re-testing and everything else looks fine.

Shadlington’s picture

You know that problem I've had with the pretty facets paths module? #1098380: Several warnings and notices
Seems to be related, bizarrely. Its now only this one taxonomy term facet I am having any problem with - in *both* issues.

For the record the taxonomy in question has no extra fields or anything like that. Its got a bunch of terms in in, some of them children of other terms.
The term field in question is a required, single-value field.
There's really not a lot else to say about it.

I kind of want to just delete the vocabulary completely and start afresh with it, in case this is just some weird anomaly.
However, if its not, then I'd have thrown away what little evidence we have of a potentially serious problem.

I am really quite frustrated with this. Its late though and I'm going to have to leave it for now.
If you have any ideas please let me know.

EDIT: I just noticed I said its this one facet that's the problem. It isn't, as its an ID facet in this issue and a Name facet in the other. It IS the same taxonomy term field though. I suppose it could be a coincidence though...

Shadlington’s picture

Okay the two issues are unrelated I think. Just a coincidence.
I can now reliably (I think) reproduce this by creating a very similar term field.
When the term field is single-value this problem happens.
When the term field is unlimited-value it doesn't (though that other issue still has the problem, hence why I think its a coincidence).

I'm going to see if there's anything else that applies to this.
I realise I've probably wasted a fair bit of your time with false information so I'm sorry about that - I shouldn't do this when I'm exhausted, I make waaaay too many mistakes :(

Shadlington’s picture

Buuuuuut when I change it from single to multi-value it doesn't fix it. Bah.

Though I have just now noticed that if I re-save a taxonomy field I've got a facet based on when I next go to a view page that has that facet showing (though I'm not using it anywhere in the view), I get the following notice:

"Notice: Undefined variable: entity_info in field_views_field_default_views_data() (line 167 of /var/www/sites/all/modules/views/modules/field.views.inc).
Notice: Undefined variable: entity_info in field_views_field_default_views_data() (line 168 of /var/www/sites/all/modules/views/modules/field.views.inc)."

Just to clarify, this is only how I caused it. There is probably a more generic cause.

EDIT: Also, changing a multi-value taxonomy term field back down to a single-value field caused the following warnings when I went to re-index:
"Warning: Invalid argument supplied for foreach() in SearchApiSolrService->addIndexField() (line 290 of /var/www/sites/all/modules/search_api_solr/service.inc).
Warning: Invalid argument supplied for foreach() in SearchApiSolrService->addIndexField() (line 290 of /var/www/sites/all/modules/search_api_solr/service.inc).
Warning: Invalid argument supplied for foreach() in SearchApiSolrService->addIndexField() (line 290 of /var/www/sites/all/modules/search_api_solr/service.inc).
Warning: Invalid argument supplied for foreach() in SearchApiSolrService->addIndexField() (line 290 of /var/www/sites/all/modules/search_api_solr/service.inc).
Warning: Invalid argument supplied for foreach() in SearchApiSolrService->addIndexField() (line 290 of /var/www/sites/all/modules/search_api_solr/service.inc).
Warning: Invalid argument supplied for foreach() in SearchApiSolrService->addIndexField() (line 290 of /var/www/sites/all/modules/search_api_solr/service.inc).
Warning: Invalid argument supplied for foreach() in SearchApiSolrService->addIndexField() (line 290 of /var/www/sites/all/modules/search_api_solr/service.inc).
Warning: Invalid argument supplied for foreach() in SearchApiSolrService->addIndexField() (line 290 of /var/www/sites/all/modules/search_api_solr/service.inc).
Warning: Invalid argument supplied for foreach() in SearchApiSolrService->addIndexField() (line 290 of /var/www/sites/all/modules/search_api_solr/service.inc).
Warning: Invalid argument supplied for foreach() in SearchApiSolrService->addIndexField() (line 290 of /var/www/sites/all/modules/search_api_solr/service.inc)."

Shadlington’s picture

Guh.
Its happening with the author facet too now.
I have no idea how to find out what's going on here.

I give up.

EDIT: I just gave miiimooo access to my test site so he can have a look at that other issue. Would that be helpful to you at all?

miiimooo’s picture

I think you need to Re-index.

Shadlington’s picture

No its not that, I re-index every time I make any kind of change.

Shadlington’s picture

The pretty facets rewrite issue got resolved so I have renewed hope :)

I standardised my testing a bit.
Author ID facet is doing this.
Taxonomy term ID facets are doing this is they are single-value.
If they are multi-value they work correctly.
Whether the term field is required or the vocab is hierarchical makes no difference.

Shadlington’s picture

Ah! I think I might have had a break-through.

It isn't a matter of single vs. multi value at all!
Its a matter of 'are there still facets left to choose from in this block after selecting on facet within it'

With my multi value term field, if I select a term for which every node that is associated with that term is not associated with any other terms in that field, then it changes to show the ID as the active facet link and has no other facets to choose from.

But if I choose a term that appears on nodes with other terms selected too, then it appears as the label in the active facet link and shows other terms as possible facets still.

Author is always single-value so it might be the same thing with that.

Shadlington’s picture

FileSize
10.66 KB

Screenshot attached showing a facet block doing this.
Top left: No term selected
Bottom: Term 'Three' selected - No other terms on this field on any of the node in the results set
Right: Term 'Two' selected - Other terms on the field within the results set.

drunken monkey’s picture

Title: Display entity labels instead of IDs » Add a way to index an entity directly
Status: Needs review » Needs work

Thanks for your thorough investigation! This should be enough to find the error quickly …

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
25.08 KB

Could now reproduce it, and should be fixed in the attached patch. Thanks again!

Shadlington’s picture

Oh man, I am so relieved you could reproduce it. This one was driving me nuts.
I was so close to just scrapping everything and starting afresh because I was starting to think I had some random anomaly happening in my site alone.

patching file contrib/search_api_facets/search_api_facets.module
Hunk #1 FAILED at 345.
"1 out of 1 hunk FAILED -- saving rejects to file contrib/search_api_facets/search_api_facets.module.rej
patching file README.txt
patching file contrib/search_api_facets/search_api_facets.admin.inc
patching file contrib/search_api_facets/search_api_facets.module
patching file contrib/search_api_views/includes/handler_field_entity.inc
patching file contrib/search_api_views/search_api_views.info
patching file contrib/search_api_views/search_api_views.views.inc
patching file includes/index_entity.inc
patching file search_api.admin.inc
patching file search_api.module"

The patch didn't apply cleanly.
Not sure why but this one hunk failed... But its just a comment so I carried on without it.

However, it doesn't seem to have fixed it :(
Do you think I'd have to recreate the index again or should it 'just work'?

drunken monkey’s picture

WTF? Why are there two chunks for search_api_facets.module?!?
Git is great and all, but sometimes it does REALLY strange things … Anyways, working patch should be attached. Please see if that one fixes your issue.
And it should just work (it did for me), no need to recreate the index or do anything else.

Shadlington’s picture

Bah. Patch applied this time but still no change.
Cleared caches, reindexed. No joy.
I've got something I need to do now but I'll try recreating it from scratch again in an hour or so.

Shadlington’s picture

Okay, not sure why this wasn't happening before, but I'm getting quite a bad reaction from this now!
When I try to index a taxonomy term field, I get a Solr exception when I reindex!!
If I untick all the term fields, it reindexes again.

This *wasn't* happening before. It was definitely able to reindex immediately after I patched it. I don't know what's changed in the time that's past since then.

Anyway, this is the message I get in my log:
The server encountered an internal error (For input string: "" java.lang.NumberFormatException: For input string: "" at java.lang.NumberFormatException.forInputString(NumberFormatException.java:65) at java.lang.Long.parseLong(Long.java:450) at java.lang.Long.parseLong(Long.java:478) at org.apache.solr.schema.TrieField.createField(TrieField.java:426) at org.apache.solr.schema.SchemaField.createField(SchemaField.java:94) at org.apache.solr.update.DocumentBuilder.toDocument(DocumentBuilder.java:246) at org.apache.solr.update.processor.RunUpdateProcessor.processAdd(RunUpdateProcessorFactory.java:60) at org.apache.solr.handler.XMLLoader.processUpdate(XMLLoader.java:139) at org.apache.solr.handler.XMLLoader.load(XMLLoader.java:69) at org.apache.solr.handler.ContentStreamHandlerBase.handleRequestBody(ContentStreamHandlerBase.java:54) at org.apache.solr.handler.RequestHandlerBase.handleRequest(RequestHandlerBase.java:131) at org.apache.solr.core.SolrCore.execute(SolrCore.java:1316) at org.apache.solr.servlet.SolrDispatchFilter.execute(SolrDispatchFilter.java:338) at org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:241) at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:235) at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:206) at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:233) at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:191) at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:558) at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:127) at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:102) at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:109) at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:298) at org.apache.coyote.http11.Http11Processor.process(Http11Processor.java:859) at org.apache.coyote.http11.Http11Protocol$Http11ConnectionHandler.process(Http11Protocol.java:588) at org.apache.tomcat.util.net.JIoEndpoint$Worker.run(JIoEndpoint.java:489) at java.lang.Thread.run(Thread.java:636) ) that prevented it from fulfilling this request.

EDIT: Actually, I updated my version of Entity API. That changed. Could that have something to do with it?

drunken monkey’s picture

No, the Entity API shouldn't have anything to do with it. But it's good that you're using the latest version.

The difference apparently was that you used optional single-valued term fields in your second try – at least that's what triggered the error for me. So you were in luck, I could instantly reproduce and fix this. Thanks again for spotting all those bugs! ;) Amazing, how many errors the original patch still had …

Is the one with the disappearing labels for active facets now fixed for you, by the way?

Shadlington’s picture

Ah, that's wonderful, you've fixed the indexing problem.

The "disappearing labels for active facets" problem remains, however.

drunken monkey’s picture

Seems like your patch tool had a reason not to apply that one comment before: apparently it was wrong. Don't know why this worked for me before …

Please see if the attached patch fixes this.

Shadlington’s picture

That's it! Its fixed! :D
Fantastic work on this.
*high five*

How much further testing (if any) do you want before you feel its ready for committing?

drunken monkey’s picture

I think, if nothing more shows up I'll just commit it this evening or tomorrow.
Your testing up to now was very thorough anyways, all of those issues were fixed and even if there were still some bugs in the patch, we could always come and fix them later.

Shadlington’s picture

Just letting you know that I've done a fair bit of further testing without finding any problems.

drunken monkey’s picture

Status: Needs review » Fixed

OK, great to hear. Just committed this now, thanks again for your big help!

Status: Fixed » Closed (fixed)

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