Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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 :)
Comment | File | Size | Author |
---|---|---|---|
#43 | 2709953-43--support_display_plugin.patch | 33.75 KB | drunken monkey |
Comments
Comment #2
StryKaizerComment #3
StryKaizerI need more sleep
Comment #4
StryKaizerComment #5
borisson_Looks good, I did notice some nitpicks but overall this looks great.
Not sure about the newline in here, I don't think we do this.
This is indented too deep, needs to be one level less indented.
Comment #6
borisson_Functionally this looks great, fixed my own nits from #5.
Comment #13
swentel CreditAttribution: swentel as a volunteer commentedComment #17
swentel CreditAttribution: swentel as a volunteer commentedProbably just needs a reroll
Comment #18
ashishdalviWe will work on this issue in Drupal Mumbai code sprint #15
Comment #19
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedComment #20
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedAdding rerolled patch
Comment #21
rasikap CreditAttribution: rasikap at Blisstering Solutions commentedComment #22
borisson_It'd be great if @StyKaizer can have another look at this, to see if it works with sorts.
Comment #23
borisson_Fixes the plugin,, start of adding a test. Looks like adding the field isn't working. No idea.
Comment #24
borisson_Why are the tests not discovered anymore? Did I make a mistake? I guess so. Looking into that.
Comment #25
borisson_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.
Comment #26
borisson_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.
Comment #27
borisson_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.
Comment #28
edurenye CreditAttribution: edurenye as a volunteer commentedPatch in #25 was not applying so I just rerolled it.
Comment #29
edurenye CreditAttribution: edurenye as a volunteer commentedAbout 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.
Comment #30
brathbone CreditAttribution: brathbone as a volunteer and at Caxy Consulting commentedHello...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."
Comment #31
ctrlADelPatch 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.
Comment #32
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedI'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.
Comment #33
borisson_Setting to needs review, looks good at first glance but haven't reviewed manually yet.
Comment #35
josephdpurcell CreditAttribution: josephdpurcell at Digital Bridge Solutions commentedI 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.
Comment #36
richgerdesMy 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.
Comment #37
dakku CreditAttribution: dakku commentedAny update on this gents?
I am currently in a position where I have this issue: https://www.drupal.org/node/2909956
Comment #38
borisson_Yeah, that looks like a duplicate of this issue.
If we fix these and the test still work I think we can commit this.
Should be "A search api page entity"
'display_id' => $page->id,
should be added to the plugin definition created here.Comment #39
dakku CreditAttribution: dakku commentedThanks Borisson.. Lets try this patch then :)
Comment #40
borisson_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.
Comment #41
dakku CreditAttribution: dakku commentedThank you for making this happen Ser @borisson and @drunken_monkey.
Comment #42
drunken monkeyWhy? 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?
Comment #43
drunken monkeyLooked through it, just a couple of minor code style issues and one or two tiny actual changes.
Comment #44
drunken monkeyComment #46
drunken monkeyJoris ran the tests manually and provided a verbal RTBC, so: committed.
Thanks, everyone, for your work in here!
Comment #48
dakku CreditAttribution: dakku commentedThank you for making this happen Ser @borisson and @drunken_monkey.
Great work everyone :D