What this patch does:
- Add a views filter which can render facets for that view (1)
- Add a views area which can render facets
- Support AJAX, also in combination with views ajax get module
- Add a summary area
- Add a summary filter (1)
- Remove the current AJAX support on blocks
- Provide an upgrade path for this
(1) Exposed filter block then renders the facets or summary and knows about the context.
------ original summary ----
This is a wild idea, and I'm not 100% sure if it's possible at all, but here's the idea.
Create a views filter field so that facets can be rendered in the exposed form:
- You don't need to expose the facet blocks in a specific region on the page
- Theming is consistent and bit easier since it all lives inside the view's exposed form block
- When using views AJAX, there's only one request to update the view and facets instead of the current facets AJAX implementation which means there's two calls when nothing is cached. We also use views_ajax_get which is far more performant as caching can actually work nicely here
- This probably fixes a couple issues with the current implementation (although I can't tell which exactly). Basically, any problem that this one gives with ajax, is probably a views issue ;)
Would this be something you'd be willing to commit, if so, I will start working on it. Won't be a funny one I'm afraid, but I'm willing to dive deep here as we're experiencing some minor performance issues right now on a project because of the two requests.
Dont' forget to credit the people in #2957950: Add views area plugin - most of the area code comes from there.
Issue fork facets-3073444
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedComment #3
geek-merlin+1000 for this. See related issue.
Comment #4
geek-merlinComment #5
geek-merlin@swentel: I'd say pursuing this makes sense anyway, if this views plugin will be committed to facets, or if the maintaimer feels like this should live in a separate module. I have no doubt that @borisson_ will cooperate on any APIs needed for this...
Comment #6
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedupdated title and summary a bit - this also works nicely when not using ajax. I've got a filter working, hopefully I can post a patch tomorrow.
Comment #7
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedComment #8
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedFirst patch which adds the filter. Exposed form now renders facets and works fine - even when the exposed form is rendered without the view. Theming a facet mimics block at this point - this made sure we didn't have to change any css at all since we used blocks before, but maybe this isn't needed at all.
There are two things to finish:
1. facets and AJAX.
This is not in the patch yet. I have it working partially via ajax_render_alter() (and a current hack in views but that can be solved by overriding behaviors) but I'm not sure yet if this is the right approach - need to think about it a little more, especially since we use views_ajax_get and views_ajax_history. I'm thinking to actually alter the controller (like ajax_views_get does) which makes it a bit easier to fix the ajax.
2. Summary (AJAX)
I don't handle the summary yet either. When I realised this while working on the AJAX part, I momentarily thought that the area thing was probably a better idea, but then you can't put the facets in the exposed form, so you'd have to expose blocks again too. It's crazy in which kind of brainstorm loops you get while working on this :)
Summary should probably become an area plugin so you get almost free AJAX, but still brainstorming on that.
So this becomes complexer than I thought. I think, IMO, it might be better to move this to a separate module, not even within this module. I'm totally fine with maintaining it. It will cover 4 things:
Not sure about the namespace: facets_views_plugins, facets_views_extra (currently my personal preference as it might contain much more in the future), anything other?
So, everyone fine this becomes a separate module? I can move much faster then without bothering maintainers, I know how annoying that sometimes can be :)
Comment #9
geek-merlin> Exposed form now renders facets and works fine - even when the exposed form is rendered without the view.
Wow, concerning the source, this is in fact impressingly simple. Not played with it though.
> [develop as facets_views_extra]
+1. (Maybe merged later.)
Comment #10
DamienMcKennaWhy not just "facets_views"? :)
This would be an amazing feature when it's finished, and IMHO it'd be worth including in the main Facets module.
I've tested it out and it works with AJAX, though I am using Better Exposed Filters.
One suggestion - the filter really should either hardcode the "Expose this filter" option, or include a message in the dialog that indicates that the "Expose" option must be enabled, otherwise the facets aren't displayed.
Comment #11
DamienMcKennaRelated issue: #3087911: Combine facet with active arguments to a view
Comment #12
DamienMcKennaSorry, that one was a duplicate of #2986981: Ajax facet block seems to lose Views context (filters, etc).
Comment #13
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedRight! Naming things can really be easy :)
Indeed, I think I'm going to hide the checkbox anyway (and submit it as exposed of course)
I'm fine with either road ahead, I'm basically waiting for an answer from Borisson :)
Comment #14
borisson_I don't think I'll get to this (or any other issue for that matter) before drupalcon.
Comment #15
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedOk, I might be there as well, still not 100% sure, we can talk to see what the best plan is then if I'm there :)
Comment #16
kgeeraerts CreditAttribution: kgeeraerts commentedExactly what I need, except I need to disable the source check as noted in the comment ( @todo Find better way of filtering for facet source.)
I use facets from a different view for a custom search index so no facets show up with the check in place.
Except for that, great work!
Comment #17
DamienMcKennaThe facet works as intended, but it doesn't work via AJAX.
Comment #18
borisson_After reading this issue - I agree this would be a good addition to the main facets module, I personally don't think this should be an additional module either (as in, it doesn't have to be a submodule).
I see that you've directly loaded search api's index in
facets_views_data_alter
. That won't work if someone is using core search as a source.The class docblock needs to be updated.
I don't know of an easier way to do this either.
Otherwise I'm super happy with how minimal this patch is. It doesn't seem to introduce any additional complexity. @swentel++
Comment #19
sahaj CreditAttribution: sahaj commentedJust noticed that the patch does not apply anymore via composer with the last dev version of facets.
Comment #20
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedWorking on some fixes during drupalcon, a new patch is due for this week.
Comment #21
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedNew patch. It's now a dedicated submodule. AJAX works fine, replacement of the response needs a double check and I also want to check when views_ajax_get is enabled.
Comment #22
borisson_We just discussed this at drupalcon AMS. As I said there - I'm sorry but I think we have to make this harder.
I would very much love to remove all the old ajax code, because with this implementation we can be sure that the only bugs that this produces are views bugs, and luckily we don't have to solve those ourselves but that burden goes to others.
So I suggested that we could write an upgrade path to enable the facets that are defined on a view to the filters, so that all of that "magically" keeps working for our users. That is a bug ask, I realize that, but I think that doing it like that will reduce the surface for possible bugs and it will be better for us in the long run.
Thank you for all the help on this already @swentel
Comment #23
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedNew version moving everything back to the main module. Views Ajax Get now works as well. I've marked the TODO's which do not block the actual functionality, everything works fine. The javascript to register the facet links could use a review from someone who's better in Drupal JS than me.
I've closed #2957950: Add views area plugin - in case we still want an area plugin, that can still happen, but seems a bit redundant imo.
Next up: summary plugin and ajax detection of those. After that, ripping out all the old code :)
edit - Just noticed: the replacement of the block with ajax needs an update, it's not entirely right when there are multiple facets, fixing now :)
Comment #24
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedBetter selector. Found a problem with ajax and multiple facets, checking that now (it works perfect with one)
Comment #25
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedActually multiple facets work fine - it was the 'Hide facet when facet source is not rendered' setting which got in the way for AJAX. I might add a check for that in the manager because this one can be very confusing imo.
Comment #26
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedFirst batch of removing code, that was fun :)
Comment #27
borisson_Oh, that looks great! You've been able to remove more code than is added with the new (better) approach, this is the best possible situation.
I asked @StryKaizer to to a look at this patch to see if it already works, he has a real-world project where he can test this.
Comment #28
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commented@borisson_ I'm surprised no tests are failing, there are no tests for the current ajax code? What exactly does ajaxbehaviortest do then? (unless I missed code of course)
Comment #29
borisson_Huh, it might not check that is actually an ajax request but it could possibly be that it still works because it now still does page requests. Good to know that our tests are insufficient.
Comment #30
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedRight, makes sense I guess, even though I see assertWaitOnAjaxRequest calls, but I don't know the exact internals there.
Comment #31
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commented@borisson: does facets ignore the 'page' parameter, or am I missing something in my setup. Even with a standard facets block and no ajax, if you go to page 2, and inspect the facet link, you do not see page=1 for instance - I probably never really looked at that, so if it's by design, that's fine :)
There is one outstanding bug with ajax: if you would go to a url with a facet in it, you can not disable the selected facet again, it looks like internally in the rendering somewhere it still listens to f[0] in the $request. Checking whether that that's an JS thing or something in the code (pager ajax works fine in views - so will have to compare the js/php in views for this one)
Comment #32
StryKaizerNot sure why, but it does not seem to work here.
Some remarks:
- facets rendered as checkbox are still links
- ajax does not seem to work overhere, the links are actually
What I did:
enable ajax in search api view, and add a filter of type "facet"
Do I need some extra steps to use this?
Comment #33
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedThat's the problem, I can reproduce, it only works on links now. Checking.
The dropdown probably won't work yet either.
Comment #34
StryKaizerokay, links do trigger ajax indeed.
However, they disappear once clicked, see video
One extra thing we probably need to check is how this patch behaves on existing sites which use the "better exposed filters" module.
Comment #35
StryKaizerComment #36
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commented@StryKaizer - that's probably the 'Hide facet when facet source is not rendered' setting. I'm going to add something in the FacetManager for that because that's going to be something that will be forgotten a lot.
Comment #37
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentednew patch. Exposes an area plugin for the facets too. Added that check in the FacetManager to ignore the views.ajax route (still need to inject though, but it works fine now)
Comment #38
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedComment #39
borisson_I checked with @lendude, this
assertWaitOnAjaxRequest
thing actually doesn't check if anything happened with ajax and just checks if there are any current ajax requests.This code is in the query string url processor, so yeah - this is by design.
I have no idea how to resolve without also debugging, no idea what the source of the problem is here.
Comment #43
borisson_Adding credit to people from the other issue - thanks for reminding me @swentel
Comment #44
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedLast one for today (I think). Fixes the bug I mentioned where there was already a 'f' or 'page' param in the url, done server side.
Comment #45
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedupdated title and summary
Comment #46
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedComment #47
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedComment #48
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedComment #49
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedComment #50
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedComment #51
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedOh well, while we're busy .. :) This adds the summary filter and area plugins. Works great with ajax too.
Comment #52
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedMore cleanups. I finally understand the trigger system after reading #3031581: Provide JavaScript API for facet widgets, so this will probably very easy to fix it for all widgets.
Comment #53
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedComment #54
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedLast patch for real now: all widgets now work with ajax. This is getting very close.
Comment #55
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedReally really last one for the day. This fixes the check to include the js library if the ajax is enabled or not.
Comment #56
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedOk, this is ready for another review and heavy testing. More cleanups in the code (injection, docs etc).
Outstanding besides tests and upgrade path:
1. checkboxes are duplicated with views ajax get (the behavior runs twice, which is weird)
2. retain other exposed fields (e.g. a search text field)
Comment #57
borisson_I discussed the upgrade path with strykaizer again yesterday and he figured that might not be needed. But I can't remember his reasons for that.
This change won't work on 8.6 I think - but I don't think this will get in before we drop support for 8.6 (when 8.8 is released).
I will look/test in more detail after work or on sunday afternoon. If @StryKaizer has more time - it would be good to try this already.
Comment #58
andypost+1 to support both area and field plugins
Comment #59
Murz@borisson_, many thanks for the patch, it works well! Can you please describe, how hard will be alter facet widgets for apply his changes via one "Apply" button from views exposed filters? At now, if I use Facet with checkboxes, they applies instantly, not like other Views filters, that wait when button is pressed.
Comment #60
DamienMcKennaSome fixes to the coding standards.
Comment #61
DamienMcKennaA few more small coding standards improvements.
There are a few method arguments that I'm not sure about how to document, and it might be worthwhile adding the docblock comment for skipping the coding standards tests on some lines e.g. the Views variables and methods that don't match camel case properly.
Comment #62
StryKaizerThe reason why we cant do an upgrade path is, because facets are seperate blocks, which can be placed in any region.
If we automaticly upgrade ajax enabled facets to live in the "contextual filters" block, we can not ensure their position is still in the same place.
So I guess we either support both ajax approaches for a while, or we just switch, and write a change record stating how users using ajax facets should manually upgrade their configuration to keep ajax working.
Comment #63
borisson_This is a good reason of not doing the upgrade path.
I would very much prefer to remove to old implementation because that is the source for a bunch of errors that could just be forgotten about :)
So the change record should be enough.
Comment #64
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedFair point. CSS selectors might probably be messing up things as well (e.g. because they wrapped things in a sidebar for instance), so there would be probably more disruption then really needed.
Supporting both seems reasonable indeed, but it's tricky stuff, I'd rather just remove it too and add that to the change record. Nothing will break, the facets just won't react with AJAX, which is ok to me.
That said, still need to look at the JS anyway for views ajax get. The JS API patch which was added in #3031581: Provide JavaScript API for facet widgets triggers attaching of behaviors again, which means (in case you would for instance add 2 facets), ALL Drupal behaviors are called 3 times. The new JS needs to account for that (but I'd like to figure out if we can't change that - might be separate patch of course, because the more facets, the more calls).
Comment #65
mirom CreditAttribution: mirom at MADE it digital s.r.o., Slovak Drupal Association commentedThis patch should fix problem with facets on views with contextual filters.
During testing I found out problem with fulltext exposed filter, now I'm trying to figure out if it's generic issue or project specific.
Comment #66
mirom CreditAttribution: mirom at MADE it digital s.r.o., Slovak Drupal Association commentedSome minor changes to make it work for AJAX requests as well.
Comment #67
DamienMcKennascrap that, I had to change composer to use the dev version, then this applied successfully
Comment #68
DamienMcKennaThis doesn't seem to work with views that are output in blocks, it hits facet_source\SearchApiDisplay->extractArgumentsForViewDisplay() and then it hits this:
A block view doesn't have a URL.
Comment #69
DamienMcKennaI don't know the APIs yet to be able to make a recommendation, but this line is wrong:
It makes the assumption that the current display is a page display and parses the arguments from it. Because of this assumption it doesn't handle other display types that can accept arguments / conditional filters, like blocks.
Comment #70
DamienMcKennaHere's the error you get when you load a facet that comes from a block:
Comment #71
DamienMcKennaThis verifies that the display plugin has a path before calling getUrl(). There might be a better way of doing this? It also adds a @todo item that other plugin types need to be handled too.
Comment #72
DamienMcKennaI just realized that it also doesn't work in situations where the display uses default arguments, e.g. "Content ID from URL".
Comment #73
mirom CreditAttribution: mirom at MADE it digital s.r.o., Slovak Drupal Association commentedDo you mean it's not working for blocks with "Content ID from URL"? I'm using it on page with "Content ID from URL" and it's working just fine. Can we always use $_REQUEST? It's filled in some ViewController, so maybe it's there for all displays?
Comment #74
mirom CreditAttribution: mirom at MADE it digital s.r.o., Slovak Drupal Association commentedI was thinking about problem with contextual filters and it seems to me that we must be doing something wrong if we need to do exact same things as views are already doing.
Comment #75
DamienMcKennaI don't think we should remove "use strict" from the JS. Also, it's spelled "AJAX".
Comment #76
DamienMcKennaApologies for the small change, but this should fix two more coding standards violations.
Comment #77
DamienMcKenna@mirom: On your view which has arguments, are there any arguments which do *not* use a default? Have you confirmed that it gives you the expected results?
My view has:
The view works, it just runs into the problem described above where block displays do not have a URL.
Comment #78
DamienMcKennaI should also mention that while AJAX is enabled on the view, it's failing on the initial request.
Comment #79
DamienMcKennaMight this be related? #2703771: Views argument set incorrectly when using AJAX pagination and a path alias
Comment #80
DamienMcKennaI'm looking at the arguments in the request in extractArgumentsForViewDisplay():
This results in the following error:
Will dig at it some more tomorrow.
Comment #81
DamienMcKennaThis doesn't work either:
Clarification: it doesn't result in any errors, it results in $argumentValues being an array containing the ID of the node passed as an argument on the page, but the argument still isn't passed in correctly, so the facet shows all items rather than those only relevant for the current selection.
Comment #82
DamienMcKennaInterdiff for those who'd like to play along at home.
Comment #83
mirom CreditAttribution: mirom at MADE it digital s.r.o., Slovak Drupal Association commentedI think you need to keep node as a key of that new array. Currently under pressure on project, I'll try to come back and help more after that.
Comment #84
DamienMcKennaThe array gets passed to this function in Drupal\views\ViewsExecutable:
I changed the code to this, but it didn't make any difference:
Comment #85
DamienMcKennaMight anyone be able to provide some insights on where I should look next? I have some time available to work on it, but I'm a little lost. Thanks.
Comment #86
MurzWith applied patch (from #76 and earlier), facet widgets successfully handle other Views exposed filter values (adds them to url), but when I submit Views exposed form - it clears out all Facet's selected items (AJAX is disabled). Is this known issue for this patch, or something is broken?
I fix this problem by quick, but not so good workaround in
hook_form_views_exposed_form_alter()
:Comment #87
mirom CreditAttribution: mirom at MADE it digital s.r.o., Slovak Drupal Association commented@Murz yeah, it's known issue, but it's afaik not related to this patch, it's not working for me with HEAD of facets and search api.
Comment #88
ayalon CreditAttribution: ayalon commentedHi!
I tested the latest patch and found several JavaScript syntax error that do not allow me to use the AJAX functionality.
Here is a fixed version.
Comment #89
vasi1186 CreditAttribution: vasi1186 at Amazee Labs commentedI had an issue on pages which have multiple ajax views, some of them are search api views, some not. Attached a very small change in the patch to fix that.
Comment #90
seanBJust gave this patch a try. We currently have a site where the exposed filters are in the content region, and all facets are in the first sidebar. It seems I can no longer have AJAX facets in the sidebar with this patch applied. Is that correct?
Comment #91
StryKaizer@seanB yes, that is correct. Ajax facets are moved to the exposed filters area, which does not allow individual placement...
Not sure how to proceed on this, we have no usecase where exposed filters are in other places than facets, but it looks like you do... Damn :)
Comment #92
seanBYeah sorry! As an example, check here: https://www.eur.nl/en/esl/search?s=drupal
The search bar is above the results, and the facets are in the sidebar. It doesn't really feel like an edge case to me, so I'm kind of hoping there is a way to not break our existing functionality.
I'm kind of thinking it would be cool to have a views area exposed as a block (just like the exposed filters). That would at least provide a lot of flexibility. Not sure how hard that would be though.
Comment #93
StryKaizerThat does look like a valid case indeed.
You could fix this by making the search and "include general" a block, since both are not ajaxified, but I do agree that this issue introduces some issues, while trying to solve others :)
If I'm not mistaken, @swentel started working on this issue because it allows doing 1 request (instead of multiple requests now).
We should discuss asap how to move forward with this issue, as this will break a lot of sites anyway, if this gets committed (Exposed filters are most of the time not in the same place as facets, by default).
I *think* that offering blocks per exposed filter beats the purpose of this patch, and re-introduces the multiple requests, but I might be wrong
Comment #94
DamienMcKennaIMHO this would deserve a new branch as the change is rather significant.
Comment #95
geek-merlin#93: See #22 for why this is important to make this module maintainable. Short: The only sane way to support ajax is to let views do it and do not interfere. And i guess this outweights by far the cost of blockifying areas or exposed-filters (probably in a separate module)(I won't push the ball rolling, but if someone does, i will chime in and help).
Comment #96
danharper CreditAttribution: danharper as a volunteer and at hrpr commentedI've just been testing this patch and it only seems to work on from what I can see is where machine name is page_1.
First I tried to use it with an entity browser display type then I tried to add a second page type but neither worked. The facets display but they don't filter the results.
Cheers Dan
Comment #97
DamienMcKennaMaybe because of this?
That's existing code in the module.
Comment #98
anruetherSome quick comments on the patch from testing it on a quite complex site
Comment #99
danharper CreditAttribution: danharper as a volunteer and at hrpr commented@DamienMcKenna I did see that but thought that it wouldn't impact as it's just a test isn't it?
Comment #100
DamienMcKenna@danharper: ugh, yes, you are correct. Maybe it has to do more with the type of display plugin? e.g. it has been noted above that it doesn't (didn't? I haven't tested recent patches) work with block displays.
Comment #101
danharper CreditAttribution: danharper as a volunteer and at hrpr commented@DamienMcKenna I did think that too, I tried creating a new page display so basically it's view id page_2 bit when I look at the field for facets it doesn't show any facets in the list for me to turn on or off.
Changing the machine name of the first page from page_1 to page_3 stopped it working.
Comment #102
geek-merlin@danharper #96:
> I've just been testing this patch and it only seems to work on from what I can see is where machine name is page_1. ... The facets display but they don't filter the results.
Valuable that you battle test this! What a strange result. SOme thoughts which may or may not help...:
One bet might be a missing cache context. Does clear cache change something? Does adding the filter first on page_3 change something?
And: What are the filter parameters, do they change?
Maybe it's an issue with that random value (which looks wierd anyway and might benefit from a code comment).
Comment #103
oxyc CreditAttribution: oxyc as a volunteer commentedWork in progress patch for BEF support. In it's current state it resets facets when a BEF filter changes, unless it's a sort field, in which case it keeps them.
Comment #105
oxyc CreditAttribution: oxyc as a volunteer commentedFound another issue when using this together with TVI (and their https://www.drupal.org/project/tvi/issues/2993234 patch to actually fix their side).
If a parameter wasn't available it was resolved as NULL and caused various errors down the flow. I made a change that if none of the view parameters were resolved, dont pass any arguments.
I'm not sure if this is actually the correct logic. If someone is in doubt, maybe it's better to just tackle this as a separate issue once this feature has landed. So I'm mostly posting this patch to highlight a possible issue but it needs to be considered by someone more knowledgable.
Comment #106
vuilComment #108
liquidcms CreditAttribution: liquidcms commentedMostly i am trying to do a faceted filter apply with a submit button rather than through ajax. Haven't had any luck but seemed like if this was tying into exposed filters; then it might do the trick.
But, my use case has 3 view as blocks added as blocktabs (like quick tabs). Adding a facet in the view with this and i get fatal error: Error: Call to a member function getRegion() on null in theme suggestion alter. I suspect what was reported above that this doesn't work inside a block.
It does work inside the views preview.
Comment #109
gsquirrelI have found this patch very useful and is working pretty well. I think a recent change in facets dev version is causing issues though because my checkbox facets were being disabled. There are many other issues and patches that deal with similar ajax issues but I think they are working with the same code and files as this patch so don't work well together, at least the ones I tried.
Am still experimenting with combinations of the various patches to see if there is one that will get it working again.
Does anyone else using this patch have checkbox facets, are they working still? I don't think it is actually to do with this patch but I would like to be able to find a fix where i can still use this patch.
Comment #110
ransomweaver CreditAttribution: ransomweaver commentedI agree that this has problems with DEV. With the view set to ajax it doesn't do anything (for checkbox facets). There's no ajax load after checking a box.
Checkbox facets are working well for me in Facets 1.5 and patch 89.
Comment #111
ransomweaver CreditAttribution: ransomweaver commentedCan anyone tell how to get the facets query parameters rewritten into the url? I have a previous site that does this and I'm pretty sure that the url rewrite wasn't something I impemented, so maybe the JS reworking of this patch has removed it?
ETA: to answer my own question, its having views_infinite_scroll that causes the ajax query params to be rewritten to the url during ajax load.
Comment #113
jmee CreditAttribution: jmee commentedThanks to everyone working on this!
I'm using the most recent patch on a site and the only problem I've run into is integration with Better Exposed Filters and the 'Secondary options' section. I've seen the same behaviour whether or not the facets block is the only secondary option. The site is using the Olivero theme.
The
<div class="facets-views-plugin">
element isn't nested within the secondary options element, instead it is placed just above the<details class="bef--secondary [...]">
element (it should be nested inside the<div class="details-wrapper">
element that follows)Not missions critical but 'nice to have', and I hadn't seen this mentioned anywhere in this thread yet.
Comment #114
bgilhome CreditAttribution: bgilhome commentedThe latest patches didn't apply to the latest dev - I've rerolled #104 against the latest dev commit (5ef2c7c). Some of the hunks from the views ajax js seem to already have been committed so this file might need to be checked over.
Comment #115
vaccinemedia CreditAttribution: vaccinemedia commented@jmee I'm in the process of building a site which has to mix and match facets and exposed filters and display the results on a map and in a table underneath. There's a full text search and distance exposed filters and I'll be adding in some facets too - probably 4 so it would be cool to be able to pop some of them into the "secondary options" to make the initial form smaller. Did you find a work around for this?
Comment #116
jmee CreditAttribution: jmee commented@vaccinemedia I just used some jquery to fix the markup, not the best solution but it's quick
just make sure that jquery is specified as a dependency!
Comment #117
vaccinemedia CreditAttribution: vaccinemedia commentedI've applied the latest patch against the latest stable facets module and getting a WSOD when viewing my page view. It has 2 displays - the main page display as a table and an attachment as a map. When looking at the logs I'm seeing:
TypeError: Argument 1 passed to Drupal\Core\Render\Element::children() must be of the type array, null given, called in /var/www/drupalvm/sites/flashy.local/httpdocs/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php on line 949 in Drupal\Core\Render\Element::children() (line 71 of /var/www/drupalvm/sites/flashy.local/httpdocs/core/lib/Drupal/Core/Render/Element.php)
I'm on Drupal 9 and if I remove the facets from the view and display them as blocks on the page instead the page displays without any errors.
Comment #118
mhavelant CreditAttribution: mhavelant at Brainsum for Tieto commentedI have the same issue as #117.
Drupal\facets\Plugin\facets\facet_source\SearchApiDisplay->fillFacetsWithResults
has$view = Views::getView($display_definition['view_id'])
and$view->execute()
and that execute() is the one that fails.FilterPluginBase::buildExposedForm()
usescount(Element::children($form[$value]))
, but at this point the facets exposed filter has NULL as the value, and it fails.As a temporary fix, I added
$form[$value] = $form[$value] ?? [];
there, which solves the error. Sadly, I couldn't find a place in facets where I could add something to achieve a similar result.Comment #119
geek-merlin#118: Great debug work!
Try "$form['value'] = [];" here.
Comment #120
uniquename CreditAttribution: uniquename commented#119 seems to fix that error, but I had to edit
facets/src/Plugin/views/filter/FacetsFilter.php line 110
not facets/modules/facets_summary/src/Plugin/views/filter/FacetsSummaryFilter.php
Comment #121
SiegristI created a patch with #119 s feedback in it.
Comment #122
SiegristActually also including #120 as well. Seems to work generally. I notice the block and the filter have titles? I guess I can just remove the one on the facet... Seems weird tho.
Comment #123
SiegristCan we also get the reset button harmonize with the facets? The button only appears when views filters are applied and not when a facet is filtering.
Also similarly it would be nice if the facet did not reset when a views filter was applied.
Comment #124
SiegristI added this hook. But its very unstable. I get 414 URI too large errors. Any ideas on how to solve it?
Comment #125
SiegristIt seems like facets somewhere pass the current url as a get param for future requests along. This compounds the size of the request with each apply of exposed filters. In my case after 5 times re applying the filters I get a 414 request too large. Does anyone know what code is responsible for this?
Comment #126
SiegristThis JS seems to omit the unnecessary clutter quite well. Can someone review? I will soon provide a patch file as well.
Comment #128
SiegristI added the changes from the files and my suggestions into the MR. I think its easier to proceed there. :)
Comment #129
mkalkbrennerComment #130
Leksat CreditAttribution: Leksat at Amazee Labs commentedWhat's the reason for
facets_views_ajax
settings being removed in the last patch?Comment #133
hswong3i CreditAttribution: hswong3i at PantaRei Design Limited (Hong Kong) commentedRecreate https://git.drupalcode.org/project/facets/-/merge_requests/43 as https://git.drupalcode.org/project/facets/-/merge_requests/42 for 2.0.x-devComment #134
oheller CreditAttribution: oheller commented@hswong3i I don't understand what you're doing with the above changes. https://git.drupalcode.org/project/facets/-/merge_requests/43 doesn't look like it belongs in this issue.
Comment #135
hswong3i CreditAttribution: hswong3i at PantaRei Design Limited (Hong Kong) commented> @hswong3i I don't understand what you're doing with the above changes. https://git.drupalcode.org/project/facets/-/merge_requests/43 doesn't look like it belongs in this issue.
Sorry I means recreate https://git.drupalcode.org/project/facets/-/merge_requests/34 (for 8.x-1.x-dev which now not able to apply to 2.0.x-dev) as https://git.drupalcode.org/project/facets/-/merge_requests/42 for 2.0.x-dev.
Comment #136
joshmillerFirst off, positive feedback: THIS WORKS. OMG, this is what facets should always do.
Slightly less good news: For some reason, I've narrowed down out of memory errors that we are getting to the facets that I have enabled in a solr-based view. Without the facets, renders, and caches, the same view is just fine. With the facets, I get strange, and often inconsistent, 500 errors, like the following:
Allowed memory size of 536870912 bytes exhausted (tried to allocate 20480 bytes) web/core/lib/Drupal/Core/Cache/DatabaseBackend.php:167
Allowed memory size of 536870912 bytes exhausted (tried to allocate 32768 bytes) web/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php:190
Etc.
Since these errors are 500, none of the logs can give me a backtrace, and my local doesn't experience the same instability. So I can't be sure it's related to this, but I have successfully mitigated the error by removing facets from a view that appears on most pages.
Comment #137
joshmillerUpdate: I've been reading through some of the issues in the queue for facets and found #2939710: Add support for "Search API (tags based)" caching in Views which states:
After changing the views caching that has facets added to its filters from `none` to `Search API: Tags` I am no longer seeing 500 errors. Might be worth documenting that, at least in my case, having no caching *might* have led to weird 500 errors. Anecdotally, as I don't have any backtraces to prove exactly what went wrong.
Comment #138
GrimreaperI have done some review in the MR.
Comment #139
attisanwhat I've found so far (merge request 42):
* renaming the views page machine name breaks the view (preview)
* facets are not (have been with MR 34) shown on the views preview - works on the actual views page though
Comment #140
sonfdThe latest patches seem to remove AJAX functionalityWhen using the latest patches and facets as a filter - `facets_views_ajax` is not set in drupalSettings so the facets-views-ajax.js file can't operate.Comment #141
gsquirrelI am trying to update a site using this patch to drupal 9 and finding that facets 2.0.x-dev with the latest patch from here sort of works but ajax is broken.
The patch didn't cleanly apply - these chunks were problematic:
patching file src/FacetManager/DefaultFacetManager.php
Hunk #5 FAILED at 339.
Hunk #6 FAILED at 354.
patching file src/Plugin/Block/FacetBlock.php
Hunk #4 FAILED at 147.
Comment in #140 suggests maybe something important to do with ajax got lost along the way.
Does anyone have the patch working properly in drupal 9 / facets v2 ?
Comment #142
gsquirrelLooking at the failed hunks I don't think they are that problematic (anything important got changed properly i think, in some cases changes were already made in latest facets release).
So that was a red herring i guess.
Comment #143
3liWas using patch in #122, but after updating to 2.x it seems no patches apply smoothly.
Comment #144
geek-merlinDaring to raise prio, cause this is a key step in "fixing" ajax facets according to #3075378-8: [META] Overview of Facets + Ajax issues
Comment #145
3liRe-roll of #122 that should now apply to version 2.0.x
However it should be noted that this patch along with #122 does not allow ajax to be used, I tried to resolve this issue but did not have that much luck.
Comment #146
hswong3i CreditAttribution: hswong3i at PantaRei Design Limited (Hong Kong) commentedhttps://www.drupal.org/project/facets/issues/3073444#comment-14505630 and https://www.drupal.org/project/facets/issues/2939710#comment-14480153 is now in conflict for one single line so I merge them locally in here for composer patcher...
Comment #147
Farnoosh CreditAttribution: Farnoosh commentedReceiving Undefined Index. Add Index class to the facets_summary.module
Comment #148
hswong3i CreditAttribution: hswong3i at PantaRei Design Limited (Hong Kong) commentedComment #149
hswong3i CreditAttribution: hswong3i at PantaRei Design Limited (Hong Kong) commentedComment #150
hswong3i CreditAttribution: hswong3i at PantaRei Design Limited (Hong Kong) commentedComment #151
hswong3i CreditAttribution: hswong3i at PantaRei Design Limited (Hong Kong) commentedComment #152
Farnoosh CreditAttribution: Farnoosh commentedUpdate the patch #151 against 2.0.x with the following:
1- Imported ContainerFactoryPluginInterface and RouteMatchInterface and defined \Symfony\Component\DependencyInjection\ContainerInterface in create() function `HideWhenNotRenderedProcessor.php`
2- Import Drupal\search_api\Entity\Index in `facets.module`
Note: This patch works against 2.0.x
Comment #153
Farnoosh CreditAttribution: Farnoosh commentedRe-roll my patch against 2.0.x
Comment #154
attisanwhich patch should we use / test / review? I would tend towards the MR42 but don't know what (if any) the differences are to #153.
Comment #155
mkalkbrennerI think that most of the users who provided feedback for facets 3.0 agreed that this is the right way to go. So I want to get this patch into 3.0.x-dev quickly.
But I wonder if we should remove the support for custom Search API displays and search results that are not based on Views first.
What Do you think?
Comment #157
HitchShockI need to say, that the patch works great for me. I also added an option to allow/deny to show the block title because it is not always needed.
The first +RTBC from me. But it should be reviewed again. Probably we missed something.
Comment #158
mkalkbrennerCan anyone else involved here test the latest version of the MR?
If there's a second one who sets RTBC I'll merge it into 3.0.x.
Comment #159
attisanAs views can be called programmatically it should be possible to do so for facets (I would expect that to work when there is an option to add facet-filters directly to views (see #3301075: Provide a method in QueryString for programmatically called facets for detailed explanation).
Comment #160
attisan+1 RTBC from me.
Comment #161
mkalkbrenner@attisan I think I understand your motivation and I agree with your issue. But it should not be mixed in here.
I won't commit the new feature into 2.0.x nut into 3.0.x. But your bug fix is suitable for both branches and not essential for the feature here.
Please revert your commits to the MR. They currently block this issue.
Comment #162
attisan@mkalkbrenner done
Comment #164
mkalkbrennerThanks to everyone involved here. I committed the feature to the 3.0.x branch.
Any further help on 3.0.x is welcome. And it would be great if someone could write a change record for this issue.
Comment #166
hswong3i CreditAttribution: hswong3i at PantaRei Design Limited (Hong Kong) commentedQuick re-roll for 2.0.x-dev
Comment #168
hswong3i CreditAttribution: hswong3i at PantaRei Design Limited (Hong Kong) commentedComment #169
roman.haluska CreditAttribution: roman.haluska commentedsmall correction for 2.0.x. After filtering via ajax facets were removed
Comment #170
roman.haluska CreditAttribution: roman.haluska commentedfixed previous version
Comment #171
tgoeg CreditAttribution: tgoeg commentedJust re-read the whole issue.
Came here for some of the (missing?) functionality also mentioned in the comments here. It seems they have not been addressed in the course of development (but I might be wrong).
Precisely,
#59 (https://www.drupal.org/project/facets/issues/3073444#comment-13337346) and
#108 (https://www.drupal.org/project/facets/issues/3073444#comment-13875604)
- Is it possible to apply Facet filters and search box terms in one go, without having the request sent off when clicking a facet checkbox? Can @murz and/or @liquidcms probably give feedback on this?
#92 (https://www.drupal.org/project/facets/issues/3073444#comment-13486224):
- Can facets still live in separate blocks after the changes performed here? Can @seanB probably give feedback on this?
If all (or some) of the above is not fixed yet, I think we should open new issues for the specific problems.
Thanks for your hard work!