Using checkboxes for exposed filters results in zero rows displaying

Ideally we could check in that function whether we are dealing with checkboxes here, but this is not really possible, sadly.

Maybe we could provide a method in the base class and call it from there? It seems that this method could be helpful for more than one place

Updating steps to reproduce

Updating steps to reproduce, again

I've manually tested #16. Can confirm the presence of the bug as reported, and that the fix makes checkboxes work as expected, without breaking the default multiselect.

Test failure appears to have been a transient test bot issue.

Adding related issue

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -1325,15 +1325,39 @@ public function storeGroupInput($input, $status) {
+  public function detectEmptyCheckboxes(array $input) {

Why public? Shouldn't this be protected?

Also this feels like something form API should provide. I've added the #options issue as related, which might make the general problem go away, but even without that feels like a FAPI issue that we could add a helper there for.

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -1325,15 +1325,39 @@ public function storeGroupInput($input, $status) {
+  protected function detectEmptyCheckboxes(array $input) {

I think what @catch means is that we could move this helper method to \Drupal\Core\Render\Element\Checkboxes as a static method.

Today chx and I talked about this problem and had a slightly nicer idea to solve the problem. The idea was to add an _= query parameter in the expose dform, and
detect by that whether the views form was actually submitted. This would have just to be added for forms with checkboxes ... so its seems to be not disruptive.

The _= query parameter will break in this scenario:

- user checks an option and submits the form
- user unchecks that option and submits again
- now the URL will contain the _ query param (because the form was submitted), but all checkboxes will be unchecked and we're back to the initial problem

I'm afraid there's also another problem with the current patch, it doesn't fix the issue completely. Specifically, if the view is paged and the user navigates to the next page when all checkboxes are unchecked, the much-dreaded An illegal choice has been detected. Please contact the site administrator. shows up and no view results are displayed.

Initially, I posted a patch in the BEF queue (#2687773: Using checkboxes with an "is all of" filter fails if any options are selected), but, on second thought, the paging problem should be fixed by this patch as well. I updated the tests to include this scenario but I'm not yet sure what's the best place to filter the user input for empty checkboxes in core.

Thanks @mikeker! I found a couple of small things that should be fixed and I'll let @dawehner comment on the views parts.

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -128,22 +128,34 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   +    // Checkboxes show up as an array in the form of option_id => option_id|0. If all
   

   This comment needs to be re-wrapped to 80 chars.

2. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -128,22 +128,34 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   +    return (bool) empty(self::getCheckedCheckboxes($input));
   

   We should use static:: here.

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -127,4 +127,35 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   +    return (bool) empty(static::getCheckedCheckboxes($input));
   

   nitpick: empty() always returns a boolean

2. +++ b/core/modules/views/src/Form/ViewsExposedForm.php
   @@ -151,26 +152,43 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +          // Handle checkboxes, we only want to include the checked options.
   +          $checked = Checkboxes::getCheckedCheckboxes($value);
   +          foreach ($checked as $option_id) {
   +            $view->exposed_raw_input[$option_id] = $value[$option_id];
   +          }
   

   It would be great to have a todo + an issue to handle that on a more fundamental level, aka. the GET form, not sure whether this is even feasible though.

3. +++ b/core/modules/views/src/Plugin/views/filter/InOperator.php
   @@ -287,15 +287,17 @@ public function reduceValueOptions($input = NULL) {
   +   * @inheritdoc
   

   Note, we use {@inheritdoc}, but sure what the exact difference is, but its just how it is.

Yeah that issue seems not really super targeted ... not sure though about a better one either. Maybe we should simply create a new one?

This patch works great, but causes a fatal error when used with taxonomy lists that have a hierarchy.

I get the following errors:

Recoverable fatal error: Object of class stdClass could not be converted to string in Drupal\Component\Utility\Xss::filter() (line 72 of /vagrant/web/core/lib/Drupal/Component/Utility/Xss.php).

Warning: strlen() expects parameter 1 to be string, object given in Drupal\Component\Utility\Unicode::validateUtf8() (line 691 of /vagrant/web/core/lib/Drupal/Component/Utility/Unicode.php).

Based on the API additions, this issue is potentially only allowed during minors; however, given its impact on BEF (an important contrib project) @Cottser, @effulgentsia, @alexpott, and I agreed to consider whether this is an RC target.

@alexpott also said that he would like subsystem maintainer review for this patch.

Meant to add the rc target triage tag.

The reset button doesn't work with this patch

#40 and #43 both sound like CNW.

I need to step through this with a debugger, but here's an initial review of the patch itself.

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -127,4 +127,35 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   +  static public function getCheckedCheckboxes(array $input) {
   ...
   +  static public function detectEmptyCheckboxes(array $input) {
   

   If this needs a reroll, please change to public static

2. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -127,4 +127,35 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   +      return $value !== 0;
   

   Does this run after \Drupal\Core\Render\Element\Checkbox::valueCallback()? I believe it does, just sanity checking

3. +++ b/core/modules/views/src/Form/ViewsExposedForm.php
   @@ -151,26 +152,45 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +          // @TODO: revisit the need for this when
   +          // https://www.drupal.org/node/342316 is resolved.
   

   Lowercase @todo, indent the second line by 2 spaces

Needs tests for #40/#43

Hi guys,

I've got the same problem with my custom exposed form module.
I want to resolve this without modify the core.

Someone have a hint ?

This seems done.

I also can't reproduce the issue with the reset button.

Tested patch and works well for me. Looking forward to this patch being committed so BEF can start working on a D8 release. Good work guys!

A few small things, otherwise looks OK to me.

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -122,4 +122,35 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   +   * Determines if the submitted checkbox values include a selected option.
   

   The comment is the opposite of the method name - should both be either positive or negative.

2. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -1332,15 +1333,20 @@ public function storeGroupInput($input, $status) {
   -   * Check to see if input from the exposed filters should change
   -   * the behavior of this filter.
   +   * Determines if the input from a filter should change the generated query.
   +   *
   +   * @param array $input
   +   *   The exposed data for this view.
   +   *
   +   * @return bool
   +   *   TRUE if the input for this filter should be included in the view query.
   +   *   FALSE otherwise.
   

   Looks out of scope.

3. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -1358,6 +1364,12 @@ public function acceptExposedInput($input) {
            }
   

   all options

4. +++ b/core/modules/views/src/Plugin/views/filter/InOperator.php
   @@ -282,15 +282,17 @@ public function reduceValueOptions($input = NULL) {
   +    // The "All" state for this type of filter could have a default value. If
   +    // this is a non-multiple and non-required option, then this filter will
   +    // participate, but using the default settings *if* 'limit' is true.
   

   but? Should it be 'by?'.

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -1358,6 +1364,12 @@ public function acceptExposedInput($input) {
+          // filter if all option are unchecked.

all options

I really like the solution. I mean its a workaround, but its a workaround for a bug (IMHO) in all major browsers.

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -122,4 +122,35 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   +  public static function getCheckedCheckboxes(array $input) {
   

   Could we link to same place which explains the weirdness of GET checkboxes? I think this would be a great resource for people.

2. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -1332,15 +1333,20 @@ public function storeGroupInput($input, $status) {
   -   * Check to see if input from the exposed filters should change
   -   * the behavior of this filter.
   +   * Determines if the input from a filter should change the generated query.
   +   *
   +   * @param array $input
   +   *   The exposed data for this view.
   +   *
   +   * @return bool
   +   *   TRUE if the input for this filter should be included in the view query.
   +   *   FALSE otherwise.
       */
      public function acceptExposedInput($input) {
   

   Nice!

We got tests, we got a fix, all feedback from #58, #60 and #62 is addressed.

Manually tested that with this applied, non-checkbox filters keep working like they should.

PHPCS isn't happy with the test .theme file.

FILE: ...es/views_test_checkboxes_theme/views_test_checkboxes_theme.theme
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 1 | ERROR | [x] Missing file doc comment
 5 | ERROR | [x] Namespaced classes/interfaces/traits should be
   |       |     referenced with use statements
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Tagging novice for those changes.

Giving it a try.

+++ b/core/modules/views/tests/themes/views_test_checkboxes_theme/views_test_checkboxes_theme.theme
@@ -0,0 +1,15 @@
+ */
+ ¶
+use Drupal\Core\Form\FormStateInterface;

Nitpick: White space error

Removed the white space, patch and interdiff attached.

Feedback from @catch has been addressed, looks good again.

For me the reset button isn't working either when used in combination with views_exposed_filters. I am running on 8.1.2 and I tried using the patches in #47 and #71. I tried to reproduce this on 8.2.x without success. Does anyone know how to solve this for 8.1.x? Otherwise leaving RTBC for 8.2.x.

Absolutely. Just thought if anyone knew a quick answer to this, it would haven been quite helpful to me :)

Committed d1ae9b7 and pushed to 8.2.x. Thanks!

I don't know if there is any intention to backport this fix to 8.1.x, but here is a re-roll of #78 for 8.1.x. I can confirm that the patch works in 8.1.3.

My reroll was missing core/modules/views/tests/themes/views_test_checkboxes_theme/. Testing again.

@tidrif Your patch works for me, except for the reset button

This patch as it currently stands contains API changes so it isn't eligible for 8.1.x, that's why it was moved to 8.2.x, see Allowed changes for what can and can't be changes in certain versions.

So unless somebody does a major rewrite for 8.1.x that doesn't contain the API changes, this will have to remain a 8.2.x only fix.

There are some issues introduced by this. Would be nice to get some feedback on #2687773: Using checkboxes with an "is all of" filter fails if any options are selected.

Submitting a patch for the issue reported in #88 for 8.2

Submitting a patch against the reroll patch for 8.1 in #83 and an interdiff!

Thanks
Sukanya

sorry, started tests against the wrong version and status changed to needs work! changing to needs review again!

Please open a new issue for new patches. Otherwise its super hard to discuss stuff.

Thanks for your time and guidance. Submitted the patch in this issue https://www.drupal.org/node/2687773#comment-11546257

Issue is that upon pager navigation, checkbox values are not retained!