See #2648260: Add a "display" plugin type to list all known search pages

Once the patch in issue above is in, search api page should support this plugin to allow modules like search_api_sorts and facets to find out which display is triggering the query. I'll keep this open in a tab and let you know when that happens :)

CommentFileSizeAuthor
#43 2709953-43--support_display_plugin.patch33.75 KBdrunken monkey
#43 2709953-43--support_display_plugin--interdiff.txt6.05 KBdrunken monkey
#39 support_the_search_api-2709953-33.patch26.86 KBdakku
#32 support_the_search_api-2709953-32.patch26.82 KBjosephdpurcell
#32 interdiff-28-32.patch645 bytesjosephdpurcell
#28 support_the_search_api-2709953-28.patch26.56 KBedurenye
#26 Screen Shot 2017-02-14 at 21.28.21.png42.86 KBborisson_
#25 support_the_search_api-2709953-25.patch27.02 KBborisson_
#25 interdiff.txt4.03 KBborisson_
#23 support_the_search_api-2709953-23.patch25.81 KBborisson_
#20 2709953-20.patch4.09 KBrasikap
#6 support_the_search_api-2709953-6.patch4.08 KBborisson_
#6 interdiff.txt1.8 KBborisson_
#3 support_the_search_api-2709953-3.patch4.05 KBStryKaizer
#2 support_the_search_api-2709953-2.patch4.23 KBStryKaizer
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

StryKaizer created an issue. See original summary.

StryKaizer’s picture

StryKaizer’s picture

Issue summary: View changes
borisson_’s picture

Status: Needs review » Needs work

Looks good, I did notice some nitpicks but overall this looks great.

  1. +++ b/src/Plugin/search_api/display/SearchApiPageDisplayDeriver.php
    @@ -0,0 +1,41 @@
    +      foreach ($this->entityTypeManager->getStorage('search_api_page')
    +                 ->loadMultiple() as $search_api_page) {
    

    Not sure about the newline in here, I don't think we do this.

  2. +++ b/src/Plugin/search_api/display/SearchApiPageDisplayDeriver.php
    @@ -0,0 +1,41 @@
    +            'id' => $base_plugin_id . PluginBase::DERIVATIVE_SEPARATOR . $machine_name,
    +            'label' => $search_api_page->label(),
    +            'description' => $this->t('Represents the search api page %search_api_page_title.', array('%search_api_page_title' => $search_api_page->label())),
    +            'index' => $search_api_page->getIndex(),
    +            'search_api_page' => $search_api_page,
    +          ) + $base_plugin_definition;
    

    This is indented too deep, needs to be one level less indented.

The last submitted patch, 2: support_the_search_api-2709953-2.patch, failed testing.

The last submitted patch, 2: support_the_search_api-2709953-2.patch, failed testing.

The last submitted patch, 2: support_the_search_api-2709953-2.patch, failed testing.

The last submitted patch, 3: support_the_search_api-2709953-3.patch, failed testing.

The last submitted patch, 3: support_the_search_api-2709953-3.patch, failed testing.

The last submitted patch, 3: support_the_search_api-2709953-3.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: support_the_search_api-2709953-6.patch, failed testing.

The last submitted patch, 6: support_the_search_api-2709953-6.patch, failed testing.

The last submitted patch, 6: support_the_search_api-2709953-6.patch, failed testing.

swentel’s picture

Issue tags: +Needs reroll

Probably just needs a reroll

ashishdalvi’s picture

We will work on this issue in Drupal Mumbai code sprint #15

rasikap’s picture

Assigned: Unassigned » rasikap
rasikap’s picture

Adding rerolled patch

rasikap’s picture

Assigned: rasikap » Unassigned
borisson_’s picture

It'd be great if @StyKaizer can have another look at this, to see if it works with sorts.

borisson_’s picture

borisson_’s picture

Why are the tests not discovered anymore? Did I make a mistake? I guess so. Looking into that.

borisson_’s picture

To make the tests pass, I needed to integrate a couple of issues here:

- #2832565: Search ID was moved to a property instead of an option is in this patch.
- #2784297: Fatal error when using facets is no longer relevant.
- #2827869: Call to a member function getResults() on null is no longer relevant.

I also had to change the test from simple test to browser test base to make use of the traits in search api and facets.

The test is now passing locally, no idea why the testbot is not picking them up.

borisson_’s picture

Issue summary: View changes
FileSize
42.86 KB

Ah, that's the reason: PHP Fatal error: Trait 'Drupal\Tests\facets\Functional\TestHelperTrait' not found in /var/www/html/modules/contrib/search_api_page/tests/src/Functional/FacetsIntegration.php on line 17. The dependencies / test_dependencies are only rebuilt on commit. So that's to be expected.

@swentel: You'll have to run tests locally to prove that they are green.

borisson_’s picture

Issue summary: View changes

So, as discussed yesterday, this can't be committed before we tag a new version of facets. We'll do that at drupalcamp northern lights.

edurenye’s picture

edurenye’s picture

About comment #27, a new facets version was released since then, I tested the patch manually and works fine, also fixes #2872947: Search API pages and Facets Issue, so I closed it as duplicated.
I just found an issue, and it's that in the "Page form", in the field "Searched fields" appears empty, but this seems to be a search_api issue or might be something in my config, I'm still trying to figure it out.

brathbone’s picture

Status: Needs review » Reviewed & tested by the community

Hello...new Solr+Facets user here. Here because following the instructions to set up Solr+Facets with the modules in their current state leads to a broken site and the exact error @edurenye pointed out in https://www.drupal.org/node/2872947.

Can confirm that patch #28 gets past the error and allowed me to get my Facets going. This worked both locally and when pushed to Pantheon with Solr enabled.

@edurenye, I noticed the same issue on the Page Form you mention in #29. However, at the same time I was not getting basic Solr search results. After I went back and adjusted my results to be search results (not view modes), removed some duplicate fields, and re-indexed, I got search results and saw fields in "Searched Fields."

ctrlADel’s picture

Priority: Normal » Major

Patch fixed the errors I was having with with search_api(8.x-1.3) and facets(8.x-1.0-alpha11)

Bumping this up to major since without this patch the module is incompatible with the latest releases of search_api and facets.

josephdpurcell’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
645 bytes
26.82 KB

I've identified #2897615: Facet does not display on Search API Page, but realize that issue is with this patch. I'm OK if the issue is addressed there since this is already RTBC.

In favor of addressing it here, I've updated the patch with a proposed resolution, but it still needs work. If we don't go that path, set this back to RTBC and we'll address it in the other ticket.

This patch should consider having tests added and needs reviewed. Consider it a "proof of concept" as I've not had a chance to run this live yet.

Setting to needs work.

borisson_’s picture

Status: Needs work » Needs review

Setting to needs review, looks good at first glance but haven't reviewed manually yet.

The last submitted patch, 32: interdiff-28-32.patch, failed testing. View results

josephdpurcell’s picture

Status: Needs review » Needs work

I finally had a chance to test this. The #32 patch does not work as I expected.

What I'm discovering, I think, is that the facet block doesn't even attempt to render in the scenario described: clean URLs enabled and user has submitted a search term.

Setting back to needs work as this does not fix the issue.

richgerdes’s picture

My initial investigation showed that the facet display issue from #35, is somehow linked to the "Hide facet when facet source is not rendered" since once thats toggled on, it make the block visible on the search page. When selecting a value, it looks to me like it bounces the user to the front page with a query string containing the searched term, so the facet looks to be unaware or the location of the search page.

I am going to look into this further, but am just getting into facets with search_api, so it might take sometime.

dakku’s picture

Any update on this gents?
I am currently in a position where I have this issue: https://www.drupal.org/node/2909956

borisson_’s picture

Yeah, that looks like a duplicate of this issue.

If we fix these and the test still work I think we can commit this.

  1. +++ b/src/Plugin/search_api/display/SearchApiPage.php
    @@ -0,0 +1,28 @@
    + * Represents a Views block display.
    

    Should be "A search api page entity"

  2. +++ b/src/Plugin/search_api/display/SearchApiPageDeriver.php
    @@ -0,0 +1,59 @@
    +      $plugin_derivatives[$page->id()] = [
    +        'label' => $page->label(),
    +        'description' => $page->label(),
    +        'index' => $page->getIndex(),
    +        'path' => '/' . $page->getPath(),
    +      ] + $base_plugin_definition;
    

    'display_id' => $page->id, should be added to the plugin definition created here.

dakku’s picture

borisson_’s picture

Thanks for the patch @dakku, I'll look at this (and other open issues in this queue) at drupalcon next week with @drunken_monkey. We'll figure out a solution there.

dakku’s picture

Thank you for making this happen Ser @borisson and @drunken_monkey.

drunken monkey’s picture

Status: Needs work » Needs review

'display_id' => $page->id, should be added to the plugin definition created here.

Why? I don't think that's used anywhere?
Or do you just want the page ID handy, in case we need it (and not via getDerivativeId())? In that case, I'd suggest something like 'page_id' as the key.
Also, it's $page->id(), not $page->id.

Apart from that, "No tests found" doesn't seem right? Looking at the console output, it seems that's just because of a PHP fatal error – not a very good sign. (On the other hand, the branch tests have the same problem, so it's at least not this patch's fault.)

But yeah, thanks for the patch! Reviewing during DrupalCon sounds good – especially if swentel will be there?

drunken monkey’s picture

Issue tags: -Drupalcon Vienna 2017 +Vienna2017

drunken monkey’s picture

Status: Needs review » Fixed

Joris ran the tests manually and provided a verbal RTBC, so: committed.
Thanks, everyone, for your work in here!

dakku’s picture

Thank you for making this happen Ser @borisson and @drunken_monkey.
Great work everyone :D

Status: Fixed » Closed (fixed)

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