Closed (fixed)
Project:
Select2 Boxes
Version:
8.x-1.x-dev
Component:
Code
Priority:
Major
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
21 Jan 2020 at 08:54 UTC
Updated:
14 Dec 2020 at 19:49 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
sergei_semipiadniy commentedComment #3
sergei_semipiadniy commentedComment #4
jacobbell84 commentedThanks for kicking this off. The patch probably does work for some situations, but it's still using the old BEF 3 style form alters and overrides to accomplish it's task. That means it's not working in some situations (secondary filters is at least one) and isn't supporting some of the nice enhancements that BEF 4 has to offer, like being able to rewrite field values of custom widgets, like this one. I did a more extensive refactoring of the select2bef module to move everything into BetterExposedFiltersFilterWidget plugins which fixes the above issues.
I tried to preserve the existing functionality as much as possible, but there is one important feature my patch doesn't currently have. The original select2bef had a bundle/vocabulary selector on any entity reference field. My patch doesn't have that selector, but does still work for Taxonomy filters (by reading the actual filter values). This is a bit of a larger discussion, but I was hoping we could take this opportunity to explore other ways to recreate that existing functionality. It had a lot of issues because it was just looking for 'target_id' in the filter key. There's lot of situations where that key doesn't match the field and it causes BEF to break entirely. In the case of taxonomy, it would override default filter functionality (such as limiting the list to only certain items in a given vocabulary). Lastly, it also felt a little outside the scope of this module. Being able to render any entity reference field as a dropdown (select2 or otherwise) could be useful for lots of situations. Maybe it makes sense to split that part off into a separate project, assuming we could come up with a more stable way to handle it?
As with Sergei's patch, I haven't gotten tests working. Looking at the queue though, it appears they were broken already.
Comment #5
jacobbell84 commentedSupporting a few more situations and some minor bug fixes
Comment #6
jacobbell84 commentedAdding in some missing config entries for the list widget
Comment #7
jacobbell84 commentedHopefully fixing the issue with the patch not applying
Comment #8
jacobbell84 commentedFixing a few PHP warnings and issues with flag module integration
Comment #9
matsbla commentedComment #10
jacobbell84 commentedHi matsbla, can you give a little bit of detail on what your patch is adding/changing?
Comment #11
matsbla commentedSorry, I just tried to fix/update the test.
I've briefly checked the patch and it looks good, but I will check and test further when I have more time.
About the bundle/vocabulary selector, I think it was added there originally as it was difficult to get the configurations from the view about which vocabulary that should be allowed, so it was a work-around. I think we can remove it, it is okay with this BC if we can get a more clean solution.
The tests were working fine for BEF 8.x-3.0-alpha6, but I think they broke when BEF 8.x-4.0-alpha1 was released.
Comment #12
jacobbell84 commentedNo worries, that makes sense! Thanks for trying to help on that, I'm fairly new with phpunit tests.
Comment #13
matsbla commentedOne issue I found, when using the multiple value widget. I go to the view and then choose a term and click "Filter". It filters, but when the page reload the filter value is empty, so the selected value is not displayed in the widget.
Comment #14
jacobbell84 commentedThis should fix the issue with the filters disappearing in the multiple value widget. This also adds support for the new entity reference filter being worked on in #2429699: Add Views EntityReference filter to be available for all entity reference fields. That patch isn't required to apply this patch, but if you are using both of them they should work together and fill the gap left by removing the custom bundle selection code of the original select2_bef module.
Comment #15
matsbla commentedThanks!
I've tested now, but I still get the same problem, the filter values disappears on page reload.
I fixed some few coding standards.
I wonder if Select2BoxesAutocompleteMulti should be available even when multiple values are not allowed, and then limit allowed selected values like this:
https://select2.org/selections#limiting-the-number-of-selections
Comment #16
jacobbell84 commentedHi matsbla, can you give me some details on your view? I'm definitely not able to reproduce the disappearing filter issue anymore, so I'm wondering if there's some configuration I'm not taking into account.
Couldn't hurt, and it would maintain backward compatibility. I'll look into that for the next patch.
Comment #17
matsbla commentedI use last dev version of selectboxes with your last patch + last BEF 8.x-4.0-beta1.
In the view I have a taxonomy filter for tags with "Dropdown" in filter settings + enable "Allow multiple selections"
On Exposed form options I choose Select2 Boxes (Multiple values).
I got to the view, select 2 tags. Click "Filter". After page reload it is filtering, but it is not filtering.
I get this address:
http://localhost/admin/content?title=&type=All&status=All&langcode=All&f...
But I think it should be like this to filter:
http://localhost/admin/content?title=&type=All&status=All&langcode=All&f...
However, even when I manupulate it to correct address the filter field is still empty on page reload (even if it is now filtering), so I can't visually see the selected tags.
Comment #18
jacobbell84 commentedHi @matsbla, I think I tracked down the issue. Not sure why it was working differently between our environments, but it looks like BEF 4 has a work around in place for select boxes to account for a core issue, so I had to add the same work around to these filters. I also switched single/multi back to being available to all drop downs like it was before.
Comment #19
jacobbell84 commentedTrying to get the tests working again.
Comment #20
jacobbell84 commentedFixing field name in test target.
Comment #21
jacobbell84 commentedOne more try with tests.
Comment #22
jacobbell84 commentedNot sure why the tests still aren't working. Problem for another day :)
Comment #23
matsbla commentedI tried the patch in #21.
I added an exposed filter with widget "Multiple values".
I go visit the page and I get an error:
TypeError: Argument 1 passed to Drupal\Core\Entity\EntityStorageBase::loadMultiple() must be of the type array or null, string given, called in /var/www/html.original/modules/contrib/select2boxes/modules/select2_bef/src/Plugin/better_exposed_filters/filter/Select2BoxesAutocompleteMulti.php on line 162 in Drupal\Core\Entity\EntityStorageBase->loadMultiple() (line 257 of /var/www/html.original/core/lib/Drupal/Core/Entity/EntityStorageBase.php)
#0 /var/www/html.original/modules/contrib/select2boxes/modules/select2_bef/src/Plugin/better_exposed_filters/filter/Select2BoxesAutocompleteMulti.php(162): Drupal\Core\Entity\EntityStorageBase->loadMultiple('All')
Comment #24
jacobbell84 commentedHi matsbla, was this on a single or multiple filter? I was able to reproduce that by applying the multi select widget on a single item filter. I added some additional error checking, but the more I was testing the multiple widget on a single filter I decided to revert to the original way I had it before where it prevents adding the multiple widget on a single filter and vice versa. Even with setting the 'limit allowed selected values' on the select2 instance there was a lot of usability issues. I don't think this was really a valid configuration on the existing implementation.
Comment #25
matsbla commentedThanks again for working on this!
Alright, it makes sense to prevents adding the multiple widget on a single filter, at least for simplicity.
However I tried now with multiple widget (on multiple filter).
I make one node with 2 tags. Then I go to filter on those two tags.
Once I click to filter I can't any longer see the selected tags in the exposed filter (even though I can see them in the address).
In addition, I get 2 rows of the same node.
Comment #26
jacobbell84 commentedNo problem! Thanks for helping me work through this. Just wondering, what version of Drupal core are you on? I'm on D8.8. It looks like the URL is being constructed correctly for you now at least. I've always tested on the front-end of my site, so I can give the admin a try and see if there's a different result.The initial values are handled by javascript as well, so can you let me know if there's any console errors happening?
Concerning the duplicate results, if you switch back to the default filter view does that go away? This module doesn't really alter the query itself at all, so I'm wondering if there's a different issue there.
Comment #27
matsbla commentedYou are right, I had not enabled the option "Reduce duplicates"
8.9.0-beta3
Comment #28
matsbla commentedHere is link you can check on simplytest.me (not sure how long it take before it expire)
username / passowrd: admin / admin
https://stm5ec80f9f59ccf-zha3tptwzellvo6cd5g6j1gye8vazojk.tugboat.qa/admin/content?title=&type=All&status=All&langcode=All&field_tags_target_id%5B%5D=1&field_tags_target_id%5B%5D=2
Comment #29
jacobbell84 commentedThank you for setting that up! I was able to identify the issue, it occurs when you aren't using preloaded entries (I always do, so I never noticed it). I took a quick look and I think it's an easy fix so I'll look into getting another patch posted for testing.
Comment #30
jacobbell84 commentedHopefully fixing the missing multi-value item issue.
Comment #31
matsbla commentedI tested now, when I tried to confiugre the view I get these errors in the log:
TypeError: Argument 2 passed to select2_bef_form_views_exposed_form_alter() must be an instance of FormStateInterface, instance of Drupal\Core\Form\FormState given, called in /var/www/html.original/core/lib/Drupal/Core/Extension/ModuleHandler.php on line 539 in select2_bef_form_views_exposed_form_alter() (line 59 of /var/www/html.original/modules/contrib/select2boxes/modules/select2_bef/select2_bef.module)Error: Call to undefined method Drupal\select2_bef\Plugin\views\exposed_form\BetterExposedFilters::getSettings() in Drupal\select2_bef\Plugin\views\exposed_form\BetterExposedFilters->buildOptionsForm() (line 29 of /var/www/html.original/modules/contrib/select2boxes/modules/select2_bef/src/Plugin/views/exposed_form/BetterExposedFilters.php)Comment #32
jacobbell84 commentedDid you get any errors running the patch? That function doesn't exist anymore in the patched version.
Comment #33
matsbla commentedOkay, my mistake.
I think I've fixed the test.
Comment #34
jacobbell84 commentedExcellent! With working tests I think we can set this for needing review now.
Comment #35
matsbla commentedI checked over the patch again and found some few small issues.
The 'minimum_search_length' is hidden using field name, and not filter identifier, making a little confusing results for filters not using field name as filter identifier.
The country filter class name was using 'Drupal\country\Plugin\views\filter\CountryField' instead of 'Drupal\country\Plugin\views\filter\CountryItem'
Also the support for country field from address module was lacking.
I've attached diff, what do you think?
Comment #36
matsbla commentedFor list filters I'm changing back to "Select2 boxes" label, and also add back support for multiple values.
Comment #37
matsbla commentedAlso change variable names field_id to filter_id
Comment #38
matsbla commentedComment #39
matsbla commented@jacobbell84
I looked a bit more into this and I'm not sure of the preloading function actually make sense.
As the tag fields are already output as a select list all items are already loaded.
I think we can merge all those 3 filters into one, and just change the widget in background depending on if multiple values are allowed or not.
Take a loot at this patch, what do you think?
Comment #40
matsbla commentedComment #41
jacobbell84 commentedHi matsbla, I tested patch 40 a bit and it seems to work well. Only thing is I think if we're going to combine everything to one widget we should include an update hook to change the old plugin ids to the new one. When I first installed this version of the patch the BEF settings wouldn't load at all because of the mismatched plugin ids. I believe BEF takes care of moving the BEF 3.x settings into the BEF 4.x format during the update, so you could probably just target BEF 4 settings for our update hook.
Comment #42
matsbla commentedAgree, here it is!
Comment #43
matsbla commentedJust forgot to change hook name.
Can you please help test the update hook?
Comment #44
jacobbell84 commentedThanks! I made some small tweaks to the select2_bef.install file to fix some PHP warnings, but otherwise everything worked perfectly!
Comment #46
matsbla commentedGreat, thanks! Committed.
Comment #47
matsbla commentedComment #49
jacobbell84 commented