CommentFileSizeAuthor
#60 1567234-60--live_results.patch5.74 KBdrunken monkey
#60 1567234-60--live_results--interdiff.txt10.2 KBdrunken monkey
#59 1567234-59--display_live_results.patch10.78 KBpiggito
#58 search_api_autocomplete_live_results.patch12.01 KBmlamothe
#46 1567234-46--display_live_results.patch28.08 KBdrunken monkey
#46 1567234-46--display_live_results--interdiff.txt14.67 KBdrunken monkey
#37 interdiff-1567234-29-37.txt645 bytesheshanlk
#37 1567234-37--display_live_results.patch15.89 KBheshanlk
#35 1567234-35--display_live_results.patch15.89 KBheshanlk
#29 1567234-29--display_live_results.patch15.83 KBdrunken monkey
#29 1567234-29--display_live_results--interdiff.txt10.49 KBdrunken monkey
#27 1567234-27--display_live_results.patch6.59 KBdrunken monkey
#27 1567234-27--display_live_results--interdiff.txt8.74 KBdrunken monkey
#26 merge_with_search_api-1567234-26.patch3.68 KBedurenye
#23 merge_with_search_api-1567234-23.patch7 KBedurenye
#19 live-results-d8-1567234-19.patch10.23 KBdermario
#19 live-results-d8-2.png96.89 KBdermario
#19 live-results-d8-1.png41.36 KBdermario
#8 1567234-8-merge-search-api-live-results.diff10.15 KBdobe
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

I think they could be separate modules, but this autocomplete module could use 'fuzzy' search as well, so you can search for 'shirt' and it would show 'sweatshirt'.

alexander.sibert’s picture

Issue summary: View changes

Such function would be awesome because i work at the moment on a Apache Solr Search with Search API Module where i want to show direct as Auto Suggest the cities, states and country like www.wetter.de (e. g. if you type "Berlin") for example.

dakku’s picture

Hi,
I am a co-maintainer for Search API Live results module. It would be interesting to merge the two modules and have one consistent solution. Can someone chime in with their thoughts?

drunken monkey’s picture

Yes, that definitely sounds interesting! It seems the Live Results module is largely a clone of this one anyways, so I don't think it would be that hard to merge the two – though probably still quite some work.
But then you could just switch, for each search you are configuring, whether you want only suggestions or complete results. Or maybe we could display the live results based on the suggestions?
I also have been planning a major feature upgrade for this module for a few months now, but couldn't find enough sponsors yet. With the additional feature of live results, this would make the module even more powerful.

So, in general, I'm all for this and would definitely help review/complete a patch for this. If there are too large changes involved, we might have to make a new 7.x-2.x branch for this new version – but probably not even that will be necessary.
However, as hinted, I myself don't have any time to work on this at the moment or in the foreseeable future, so I'd depend on someone else doing the major portion of the actual developing.

drunken monkey’s picture

See #2502937: Make returned suggestions more flexible, which would help a lot with this. We'd then just need to implement a suggester plugin (cf. #2475435: Add support for switching the autocomplete implementation) that returns the live results instead of suggestions.
Would this be in line/compatible with the current Live Results code?

dobe’s picture

Search API Live Results creates its own Entity. Which modifies the form differently than this module. While I noticed the plugin from #5 allows you to extend the AutoComplete plugin it however doesn't let you mess with the Entity.

So I am trying to figure out the best way to approach this. Any help is much appreciated.

I am guessing we will likely need to pull over most of search api live results to get this all to work. Not sure where the plugin improvements, improve the ability to integrate this module yet.

drunken monkey’s picture

Does the different form altering actually provide different functionality, or is it just different?
I've already implemented live search results for two sites with the new changes, and didn't run into any problems – that's more or less what prompted (and funded) the changes in the first place.
So, I think for "merging" the live results module's basic functionality, just creating a new suggester plugin that gets results instead of suggestions, themes them approriately and returns them with their URLs should be all that's needed.
However, I haven't actually ever used the Live Results module, so there might be a lot of additional functionality there which I'm not aware of. If you're running into any restrictions or can't tell how to port a specific feature, please just explain this here more deeply and I'll try to help you along. In any case, I wouldn't be opposed to adding any necessary (backwards-compatible) changes or additional features to this module to make this merge possible.

dobe’s picture

Ok here is my first go at it. Everything seems to work and I pulled most of the features except for the experimental caching mechanism that Live Search Results had. I left the entity view name the same as Live Search Results as well so whoever changes over to this should have no issues. In addition I applied the view mode to all entities that have view modes so it should work with most entities unlike the current state of the live search results.

I was completely unable to get live search results original module to work in my site so this is definitely a step forward.

Overall I feel there is little impact on this module so. I don't think we need a 2.x release. But then again that is not up to me.

Another note the function diff for: search_api_autocomplete_autocomplete. Looks really ugly in this diff. All that I did was wrap it in an if statement.

Looking for feedback.

dobe’s picture

Status: Active » Needs review
drunken monkey’s picture

Status: Needs review » Needs work

Great job, thanks! At a quick test, it seemed to work fine, and it also looks pretty good already.
However, there are still a number of smaller issues with this patch:

  1. +++ b/search_api_autocomplete.module
    @@ -140,6 +146,20 @@ function search_api_autocomplete_entity_info() {
    +function search_api_autocomplete_entity_info_alter(&$entity_info) {
    +  foreach($entity_info as $entity_type => $info) {
    +    if(!empty($info['view modes'])){
    +      $entity_info[$entity_type]['view modes']['live_results_search'] = array(
    +        'label' => t('Live results search'),
    +        'custom settings' => TRUE,
    

    That seems a bit excessive. Maybe only do that for entity types with a corresponding search index?

  2. +++ b/search_api_autocomplete.pages.inc
    @@ -36,54 +36,59 @@ function search_api_autocomplete_autocomplete(SearchApiAutocompleteSearch $searc
    +          foreach ($suggestions as $suggestion) {            ¶
    

    Here and elsewhere: trailing whitespace.

  3. +++ b/src/SearchApiAutocompleteLiveResultsSuggester.php
    @@ -0,0 +1,129 @@
    +  public static function supportsIndex(SearchApiIndex $index) {
    +    try {
    +      return $index->server() && $index->server()->supportsFeature('search_api_autocomplete');
    

    It doesn't seem like you actually need that feature?
    On the other hand, though, you seem to rely on the results being entities, so probably you should check for that.

  4. +++ b/src/SearchApiAutocompleteLiveResultsSuggester.php
    @@ -0,0 +1,129 @@
    +        else {
    +          $url = entity_uri($entity_type, $entity);
    +          $render = l($entity->title, $url['path']);
    +        }
    +        $ret[] = array(
    +          'entity' => $entity,
    +          'render' => $render,
    +        );
    

    Instead of creating a link, you should set $ret[]['url'] (no matter what display method). That way, the changes to search_api_autocomplete_autocomplete() might be unnecessary, too.

  5. +++ b/src/SearchApiAutocompleteLiveResultsSuggester.php
    @@ -0,0 +1,129 @@
    +    return array(
    +      'fields' => array(),
    +    );
    

    This should have a default for 'display', too. Then you don't need the !empty() below.

  6. +++ b/src/SearchApiAutocompleteLiveResultsSuggester.php
    @@ -0,0 +1,129 @@
    +        'title' => t("Only show title (linked to node)"),
    

    "linked to node" is confusing/misleading for indexes containing other kinds of entities.

  7. +++ b/src/SearchApiAutocompleteLiveResultsSuggester.php
    @@ -0,0 +1,129 @@
    +        if ($entity_view) {
    

    This needs an !empty() to avoid notices.

drunken monkey’s picture

dobe’s picture

Thank you sir! I will address the feedback shortly. Appreciate your help and time.

roxflame’s picture

Would it be possible to have both autocomplete terms AND live results returned in the same query? They could be separated in the dropdown perhaps.

drunken monkey’s picture

Would it be possible to have both autocomplete terms AND live results returned in the same query? They could be separated in the dropdown perhaps.

Combining the suggestions from several different suggesters would be a separate feature request – but might be possible.
Otherwise, you're free to write your own suggester returning a mix of terms and results as desired.

Andy_D’s picture

There is very little activity in the Live Results D8 port thread https://www.drupal.org/node/2842912 so I would like to propose that the module is deprecated in favour of a D8 suggester plugin.

dermario’s picture

Can we use that issue for the D8 port as well? I accidentally already created #2854528: Port live results to D8 which might be a duplicate of this one.

BarisW’s picture

Please have a look at the Fast Autocomplete module, which integrates with Search API as well. For an example, check https://www.abab.nl/

drunken monkey’s picture

Can we use that issue for the D8 port as well?

Sure, why not? Of course, quite a bit of the rest of the module isn't yet ported, either, but once we finish this for D7 we can also port it to D8.

dermario’s picture

Status: Needs work » Needs review
FileSize
41.36 KB
96.89 KB
10.23 KB

I worked on the live results port for D8 a bit, as i need that soon. I got it basically working. We can select the live results plugin and configure what view mode to use for the bundles in the index.

Here are some screens from my local machine:

I know this is not perfect now but i wanted to ask for feedback on that patch to improve things.

dermario’s picture

msti’s picture

Thanks, pathc #19 works for me.
Is it possible to highlight the search terms ?

drunken monkey’s picture

Status: Needs review » Needs work

Looks pretty good, thanks!

However, instead of the "follow_link" stuff, this should use the same mechanism already supported (mostly) by the module: see \Drupal\search_api_autocomplete\Controller\AutocompleteController::autocomplete() (around line 91, currently) – if a URL is given instead of suggested keys, the autocomplete value will be set with a leading space, telling the Javascript that it's a link, not a suggestion.
However, since the Javascript isn't properly ported yet, this won't work on that side at the moment, I imagine – which is why it's a bad idea to add features before the original module has been ported. Anyways, if you need this now, please port at least that part of the Javascript and use this mechanism for making links.

Otherwise, as said, looks pretty good. Several smaller issues, of course, but basically fine already.

edurenye’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Component: Code » General code
Status: Needs work » Needs review
FileSize
7 KB

Rerolled.

I tried what you said about the leading space ($keys = ' ' . $url->toString();) but looks like the clean up with the JS cleaned that as well.
Anyway, by now I left it as did @dermario, IMHO I don't think that JS thing is the best solution, as I proposed here #2903767: Return a diferent field than the searched, we could return the ID of the rendered entity to the search button selector and then in the submit method we can redirect to the entity or do whatever we want.

What do you think?

Status: Needs review » Needs work

The last submitted patch, 23: merge_with_search_api-1567234-23.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

drunken monkey’s picture

Apparently, URL-based suggestions were just broken: #2906141: Fix support for URL-based suggestions.
Using the patch from there, please try again, with just adding a suggester and not changing any of the framework code.

edurenye’s picture

Status: Needs work » Needs review
FileSize
3.68 KB

Patch with just the suggester.

drunken monkey’s picture

Thanks, that already looks much better!
You still focused too much on entities (all of this is available for all Search API datasources, completely independent of the Entity API), were missing the config schema and I don't see how inheriting from Server would make any sense, but all in all, this looked pretty good.
I've tried to bring this to a commitable version with the attached patch. Please test/review and see if this still works for you.

If it does, then all this issue needs is some test coverage (integration, kernel or unit, could all make sense – but I guess a kernel test would be the best middle ground) and we can commit it.

In any case, thanks again for your work on this!

Status: Needs review » Needs work

The last submitted patch, 27: 1567234-27--display_live_results.patch, failed testing. View results

drunken monkey’s picture

Right, forgot all the other stuff we need to change now. Mostly, because we'll now have a suggester available in all cases (unless someone wilfully disables it, we can rid of all the special "no suggesters available" logic, I'd say.
Also, access checks and error handling.

Status: Needs review » Needs work

The last submitted patch, 29: 1567234-29--display_live_results.patch, failed testing. View results

drunken monkey’s picture

Status: Needs work » Needs review

Should apply now, sorry.

The last submitted patch, 8: 1567234-8-merge-search-api-live-results.diff, failed testing. View results

drunken monkey’s picture

Status: Needs review » Needs work

Excellent.
As said, tests are still needed, so back to "Needs work".
Also, it would of course be great if someone else could give this a try, or review the code.

heshanlk’s picture

This patch doesn't appeared to be working, are there any special configs that we need to do for this?. The $query->execute(); appeared to return null no matter what I used as terms.

heshanlk’s picture

Status: Needs work » Needs review
FileSize
15.89 KB

Changed a little bit to make it works with results showing up. Stil need the tests added.

edurenye’s picture

Provide interdiff please.

heshanlk’s picture

edurenye’s picture

Status: Needs review » Needs work

It does not work for me in d8.4, #29 was working and at some point it got broken, #37 does not work for me either.

heshanlk’s picture

@edurenye any error message or anything?

edurenye’s picture

No, just that the suggestions where empty while with the 'Retrieve from server' it's still working fine.

heshanlk’s picture

I think this need more work, for example when you add this in to a View the results should respect the views filters and arguments.

drunken monkey’s picture

I think this need more work, for example when you add this in to a View the results should respect the views filters and arguments.

That's exactly the thing you broke with your change. The $query passed to the method already contains all of that. Creating a new, blank $query will of course ignore it.
I don't know what problems you ran into with the passed $query, but this is most definitely not the solution.

heshanlk’s picture

@drunken monkey I couldn't get results and that appeared to be since the query was wrong, I'll do little bit more testing and see if I can fix that so reverting my comment and un-publishing form the issue.

mkalkbrenner’s picture

drunken monkey’s picture

Issue tags: +release target

Would be great if we could get this in for the 1.0 stable release.

drunken monkey’s picture

Here it is.
One bug that I found is that the fulltext fields checked for the suggester weren't stored properly. Maybe that was why the search query then didn't produce any results for some of you?
Anyways, this now also comes with proper dependency handling and tests (though not for said dependency handling, unfortunately, since that requires #2903889: Properly react when a search's dependencies are removed – bit of a race condition there, we might need a follow-up for that).

Anyways, please test carefully and tell me whether it works for you or not! (It's working fine for me.)

henk’s picture

Patch doesn't apply to 8.x-1.0.0-beta2, I will try to re-roll that.

drunken monkey’s picture

Patch doesn't apply to 8.x-1.0.0-beta2, I will try to re-roll that.

It applies to latest dev, so no, please don't do that. Please test with the latest dev version.

henk’s picture

Ok, on dev it works fine, already test it. There is plan to release it?

henk’s picture

  • drunken monkey committed 2e0c111 on 8.x-1.x authored by dobe
    Issue #1567234 by dobe, edurenye, drunken monkey, heshanlk, dermario:...
drunken monkey’s picture

Version: 8.x-1.x-dev » 7.x-1.x-dev
Status: Needs review » Patch (to be ported)
Issue tags: -release target

I was just waiting for someone to test/review it and confirm it works. So, yes, it will now be part of the soon-to-come stable release. (Gonna release an RC tomorrow.)
Committed.
Thanks a lot again to everyone involved here!

Now, it would just be nice to also get this into the D7 version.

henk’s picture

Thanks! +1 from me. Tested with Search API and Elasticsearch Connector.

ratvas’s picture

I have an issue on 8.x-1.x-dev with this suggester and case insensitive search.
One of titles that I'm searching for is, in example, "Gladiator".
When I enable "Display live results" suggester and "Ignore case" processor is enabled, suggestions appear only when I type lowercase letters in search bar (in example start with first 4 letters - glad). When I try to type uppercase letters (GLAD),or combined, as original title is (Glad) there are no suggestions. Only works when I enter first letter (G), no suggestions after that.
Search works well for all cases, results are there. Only suggestions/autocomplete doesn't work.

drunken monkey’s picture

@ ratvas: This patch has now been committed for D8, so please create a new issue for your problem!

jernejZ’s picture

Is there any possibility to get this functionality in D7 version? I would very much appreciate that.

drunken monkey’s picture

Sure, we'd just need someone to port the patch. It should actually be pretty straight-forward, I think – a lot of the code has stayed basically the same.

mlamothe’s picture

Here is a small patch that adds the basic live_results functionality to the 7.x-1.x version of the search_api_autocomplete module.

piggito’s picture

Status: Patch (to be ported) » Needs review
FileSize
10.78 KB

The patch in #58 wasn't working for me so I created a new patch based on that one but applying to current dev branch

drunken monkey’s picture

Thanks a lot for posting this, nice job!
Also thanks for the re-roll, piggito – the original patch in #58 didn’t work for me, either.

However, there are quite a few problems with the patch in its current form:

  1. +++ b/search_api_autocomplete.js
    @@ -175,6 +175,14 @@ if (typeof Drupal.jsAC != 'undefined') {
    +      // Add information on the field that triggered the autocomplete search
    +      if (typeof db.owner.input.name !== 'undefined' && db.owner.input.name.length > 0) {
    +        url += (db.uri.indexOf('?') >= 0 ? '&' : '?') + 'field_name=' + encodeURIComponent(db.owner.input.name);
    +      }
    +      if (typeof db.owner.input.id !== 'undefined' && db.owner.input.id.length > 0) {
    +        url += (db.uri.indexOf('?') >= 0 ? '&' : '?') + 'field_id=' + encodeURIComponent(db.owner.input.id);
    +      }
    

    Why? Doesn’t seem related, and doesn’t seem to be used anywhere in the patch.

  2. +++ b/search_api_autocomplete.module
    @@ -139,6 +145,20 @@ function search_api_autocomplete_entity_info() {
    +/**
    +* Implements hook_entity_info_alter().
    +*/
    +function search_api_autocomplete_entity_info_alter(&$entity_info) {
    +  foreach($entity_info as $entity_type => $info) {
    +    if(!empty($info['view modes'])){
    

    Instead of adding a new view mode to every single entity type, I’d have just let the user select the view mode to use. They could then add a new one themselves, if they wish. (Although I guess that wasn’t possible through the UI in D7 Core … So, might make sense. Let’s leave it in, now that it’s here.)

  3. +++ b/search_api_autocomplete.pages.inc
    @@ -41,53 +41,59 @@ function search_api_autocomplete_autocomplete(SearchApiAutocompleteSearch $searc
    +            // If we want to render the suggestion.
    +            if(isset($suggestion['render'])) {
    +              $ret[] = $suggestion;
    +            }
    

    Same question: why? This makes the suggestion just auto-complete the search keys to their numerical array index, instead of linking to a URL.

  4. +++ b/src/SearchApiAutocompleteLiveResultsSuggester.php
    @@ -0,0 +1,129 @@
    +  public static function supportsIndex(SearchApiIndex $index) {
    +    try {
    +      return $index->server() && $index->server()->supportsFeature('search_api_autocomplete');
    +    }
    +    catch (SearchApiException $e) {
    +      return FALSE;
    +    }
    +  }
    

    Same here: The “Live results” suggester just executes a normal search query, so there is no reason to require this server feature.

  5. +++ b/src/SearchApiAutocompleteLiveResultsSuggester.php
    @@ -0,0 +1,129 @@
    +    if(!empty($ids)) {
    +      // Load all searched suggested entities.
    +      $entity_type = $search->index()->item_type;
    +      $entities = entity_load($entity_type, $ids);
    

    This whole method assumes all indexes contain a single type of entities, and that the item type equals the entity type.
    It’s easy to rewrite the code to not need that assumption (though the “view mode” option then won’t work – but we can just hide it in that case).

The attached revision should fix all these, and makes this actually work. Before, this just lead to numeric autocompletion instead of linking to the results – at least for me. Did you actually test the patch before posting?

Still, it was a great start and took care of the bulk of the work for this. So, thanks again for posting!

Please test/review my revision!

drunken monkey’s picture

Could one or two of you please test/review? Then I could commit and we’d finally get this feature in D7, too!

drunken monkey’s picture

Status: Needs review » Fixed

OK, would have been great to get at least one additional tester for the final patch, but to avoid this patch now just lying around for months, even though it’s probably working well enough, let’s just commit this.
Thanks again for the port, anyways!

Status: Fixed » Closed (fixed)

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