Using a form_alter to change the display of an exposed form element to a checkbox results in no results until an option has been selected. (This is different behavior that the default multi-select element).

Steps to reproduce:

  1. Clean install of D8 with dummy content (requires devel) (drush si standard -y --account-pass=admin && drush dl devel && drush en devel_generate -y && drush generate-content 50)
  2. Navigate to /admin/structure/views/view/content
  3. Click the "Content: Type" filter to edit it, tick "Allow multiple selections" and press Apply and Save the view.
  4. Add the following to core/themes/seven/seven.theme
    function seven_form_views_exposed_form_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state) {
      $form['type']['#type'] = 'checkboxes';
    }
    

    (you may need to clear the cache after this)

  5. Navigate to /admin/content

No results are displayed. If you tick one of the checkboxes, the view works as expected.

Files: 
CommentFileSizeAuthor
#90 interdiff-83-90.txt799 bytessukanya.ramakrishnan
#90 2651102-90-reroll-checkboxes-in-exposed-forms-for-8-1-issues-with-pagers.patch10.13 KBsukanya.ramakrishnan
#89 2651102-89-checkboxes-forms-issue-with-pagers.patch799 bytessukanya.ramakrishnan
#83 2651102-83-reroll-checkboxes-in-exposed-forms-for-8-1.patch11.3 KBthomas.fleming
#80 2651102-80-reroll-checkboxes-in-exposed-forms-for-8-1.patch10.04 KBthomas.fleming
#71 2651102-69-71.interdiff.txt590 bytesmikeker
#71 2651102-71-checkboxes-in-exposed-forms.patch11.4 KBmikeker
#69 2651102-69-checkboxes_in_exposed_forms.patch11.08 KBSagar Ramgade
#69 interdiff-2651102-67-69.txt590 bytesSagar Ramgade
#67 interdiff.txt963 bytesgaurav.pahuja
#67 2651102-67.patch11.4 KBgaurav.pahuja
#63 2651102-61-63.interdiff.txt1.13 KBmikeker
#63 2651102-63-checkboxes-in-exposed-forms.patch11.28 KBmikeker
#61 2651102-61-checkboxes-in-exposed-forms.patch10.98 KBmikeker
#61 2651102-59-61.interdiff.txt801 bytesmikeker
#59 2651102-55-59.interdiff.txt1.6 KBmikeker
#59 2651102-59-checkboxes-in-exposed-forms.patch10.98 KBmikeker
#55 2651102-52-checkboxes-in-exposed-forms.patch10.99 KBmikeker
#52 2651102-52-checkboxes-in-exposed-forms.patch10.99 KBmikeker
#47 2651102-38-47.interdiff.txt1.99 KBmikeker
#47 2651102-47-checkboxes-in-exposed-forms.patch10.89 KBmikeker
#38 2651102-38-checkboxes-in-exposed-forms.patch10.89 KBmikeker
#38 2651102-35-38.interdiff.txt1.88 KBmikeker
#35 2651102-35-checkboxes-in-exposed-forms.patch10.78 KBmikeker
#35 2651102-33-35.interdiff.txt1.17 KBmikeker
#33 2651102-33-checkboxes-in-exposed-forms.patch10.78 KBmikeker
#33 2651102-30-33.interdiff.txt6.51 KBmikeker
#30 interdiff.txt1.53 KBamateescu
#30 2651102-30-checkboxes-in-exposed-forms.patch7.66 KBamateescu
#29 2651102-25-29.interdiff.txt3.17 KBmikeker
#29 2651102-29-checkboxes-in-exposed-forms.patch7.12 KBmikeker
#25 2651102-16-24.interdiff.txt955 bytesmikeker
#24 2651102-24-checkboxes-in-exposed-forms.patch6.49 KBmikeker
#16 2651102-11-16.interdiff.txt3.42 KBmikeker
#16 2651102-16-checkboxes-in-exposed-forms.patch6.48 KBmikeker
#11 2651102-6-11.interdiff.txt541 bytesmikeker
#11 2651102-11-checkboxes-in-exposed-forms.patch4.79 KBmikeker
#6 2651102-6-checkboxes-in-exposed-forms.patch5.32 KBmikeker
#6 2651102-6-tests-only.patch3.04 KBmikeker
#3 2651102-3-checkboxes-in-exposed-forms.patch1.76 KBmikeker

Comments

mikeker created an issue. See original summary.

mikeker’s picture

Assigned: Unassigned » mikeker

The underlying issue is that at core/modules/views/src/Plugin/views/filter/FilterPluginBase.php:1351, we check that the exposed input is an empty array, but a bunch of unchecked checkboxes results in an array of zeros. Thus the filter is added to the query when it shouldn't be.

Patch coming shortly...

mikeker’s picture

Assigned: mikeker » Unassigned
Status: Active » Needs review
Issue tags: +Contributed project blocker
FileSize
1.76 KB

This is blocking the D8 port of Better Exposed Filters.

dawehner’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
mikeker’s picture

Assigned: Unassigned » mikeker

@dawehner, I was afraid you would say that... :)

mikeker’s picture

Assigned: mikeker » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.04 KB
5.32 KB

Adds rudimentary tests.

The last submitted patch, 6: 2651102-6-tests-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: 2651102-6-checkboxes-in-exposed-forms.patch, failed testing.

The last submitted patch, 6: 2651102-6-checkboxes-in-exposed-forms.patch, failed testing.

mikeker’s picture

Assigned: Unassigned » mikeker

Ack... I included my debug code by accident.

mikeker’s picture

Assigned: mikeker » Unassigned
Status: Needs work » Needs review
FileSize
4.79 KB
541 bytes
dawehner’s picture

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

mikeker’s picture

@dawehner: yeah, I couldn't find a good way to do that.

We could move this to \Drupal\views\Plugin\views\filter\InOperator::acceptExposedInput(). It would be better scoped there, I suppose.

dawehner’s picture

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

mikeker’s picture

Assigned: Unassigned » mikeker

After talking the @dawehner in IRC, we decided to provide a detectEmptyCheckboxes() function in FilterPluginBase and move the validation of that input into InOperator.

Working on an updated patch.

mikeker’s picture

This change has the odd side-effect of moving the docblock/code style updates to FilterPluginBase::acceptExposedInput out of scope (as there are no longer any code changes to that function). I would like to suggest that we accept those doc changes anyhow, but can remove them if the powers-that-be object.

mikeker’s picture

akalata’s picture

Issue summary: View changes

Updating steps to reproduce

akalata’s picture

Issue summary: View changes

Updating steps to reproduce, again

akalata’s picture

Status: Needs review » Reviewed & tested by the community

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 2651102-16-checkboxes-in-exposed-forms.patch, failed testing.

Torenware’s picture

Status: Needs work » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Needs work
Related issues: +#342316: Introduce proper Form API #types for 'option' and 'optgroup', and make #options consistent.

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.

mikeker’s picture

Status: Needs work » Needs review
FileSize
6.49 KB

@catch, thank you for the review.

Why public? Shouldn't this be protected?

Yes it should. Fixed. I also updated the comment to more accurately state what is returned when checkboxes are checked.

Also this feels like something form API should provide.

Agreed. But #342316: Introduce proper Form API #types for 'option' and 'optgroup', and make #options consistent. is using a Twig template for each select #option (which has serious performance issues), adds two new FAPI elements, and has generally lost momentum any number of times since it was opened in 2008. You're right, this approach is working around an inconsistency in FAPI's handling of #options, but I'm concerned that the other issue will never be finished because of all it's extra baggage. (Edit: fixed bad grammar.)

This issue blocks the most common use-case for the Better Exposed Filters module: using checkboxes instead of multi-selects. I can't really release a beta of BEF on D8 until this is fixed. Also, shouldn't #342316: Introduce proper Form API #types for 'option' and 'optgroup', and make #options consistent. be a 8.1.x issue since it's an API change?

@catch, if you feel strongly against this approach, I'll jump in the other issue and see if I can restart that discussion.

mikeker’s picture

FileSize
955 bytes

Sorry, forgot the interdiff.

amateescu’s picture

+++ 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.

dawehner’s picture

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.

mikeker’s picture

Assigned: Unassigned » mikeker

@dawehner, I considered that workaround but I really don't like it. We spend so much effort getting pretty URLs (eg: we allow users to specify the filter ID) that it would be a shame to make them ugly again. It feels like a kludge.

@amateescu: Ahhh... That makes sense. I'll post an updated patch. Thanks.

mikeker’s picture

amateescu’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 30: 2651102-30-checkboxes-in-exposed-forms.patch, failed testing.

mikeker’s picture

Assigned: Unassigned » mikeker

I'll spend some time this afternoon on this.

In my initial look, I think we should move the check for empty checkboxes back into FilterPluginBase. In this case the "type" filter uses Bundle instead of InOperator. I can only imagine there will be other filter plugins that will have the same issue. Basically I'm flip-flopping what I said in #13... :)

mikeker’s picture

Assigned: mikeker » Unassigned
Status: Needs work » Needs review
FileSize
6.51 KB
10.78 KB

OK, that took a bit longer than I expected... :)

I believe I've solved the problem with the pager links not working. And it raised a few other issues in the process. There is currently a bug where clicking the pager link with an exposed form option selected will add a bogus &=Filter query string argument. (The #value of the submit button gets added to the pager links.) While it may sound like issue scope-creep, it's the same underlying problem: that checkboxes are handled so differently than multi-select elements.

And, yes, to @catch's point in #23, this should be handled by the FormAPI, but that is going to be a big lift while this needs to be fixed in order for BEF to get a release. I am reiterating my hope that we can fix this in a limited scope and remove these workarounds when #342316: Introduce proper Form API #types for 'option' and 'optgroup', and make #options consistent. is resolved.

So, this patch:

  • Moves the check for empty checkboxes back to FilterPluginBase (see #32)
  • Adds Checkboxes::getCheckedCheckboxes
  • Sets $view->exposed_raw_input to include just the checked checkboxes (which fixes the "illegal choice" error)
amateescu’s picture

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.

mikeker’s picture

@amateescu, Thanks for the review!

We should use static:: here.

Man, I can never keep those straight!

dawehner’s picture

  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.

dawehner’s picture

Status: Needs review » Needs work
mikeker’s picture

Status: Needs work » Needs review
FileSize
1.88 KB
10.89 KB

@dawehner, thanks for the review!

I've fixed #1 and #3.

I added an @TODO for #2, but I'm not sure that the best issue to link it. Thoughts?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

vivianspencer’s picture

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).

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Assigned: Unassigned » tim.plunkett
Priority: Normal » Major
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs subsystem maintainer review

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.

xjm’s picture

Issue tags: +rc target triage

Meant to add the rc target triage tag.

vivianspencer’s picture

The reset button doesn't work with this patch

catch’s picture

Status: Needs review » Needs work

#40 and #43 both sound like CNW.

tim.plunkett’s picture

Issue tags: +Needs tests

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

mikeker’s picture

Assigned: tim.plunkett » mikeker

From #40:

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

That is by design. Taxonomy filter options come as objects and need to be converted to strings. This should happen in the same place you're changing the element type.

  $form['field_tags_target_id']['#type'] = 'checkboxes';
  $clean = [];
  foreach ($form['field_tags_target_id']['#options'] as $index => $value) {
    // Some options, such as the "any" text, are really just plain
    // text so we keep those as they are. Others, like taxonomy terms
    // need to be converted to text.
    if (is_object($value) && !is_a($value, 'Drupal\Core\StringTranslation\TranslatableMarkup')) {
      reset($value->option);
      list($key, $val) = each($value->option);
      $clean[$key] = $val;
    }
    else {
      $clean[$index] = $value;
    }
  }
  $form['field_tags_target_id']['#options'] = $clean;

re: #43 (reset button doesn't work): I'll look into that.

re: #45: Thanks for the review @tim.plunkett. I'll address those items shortly.

mikeker’s picture

Assigned: mikeker » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
10.89 KB
1.99 KB

Attached patch addresses #45.1 and .3.

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

No. Checkboxes::valueCallback() runs before Checkbox::valueCallback(). Though I'm totally in the dark as to what this implies... :)

re: #43:

The reset button doesn't work with this patch

I wasn't able to reproduce this. Note that the reset button does not appear until a filter value is specified. Can you provide steps to repro?

Also, since #40 is by-design, I'm removing the Needs tests tag. Let me know if there was another use-case that needed tests that I missed.

I would reassign this to @tim.plunkett, but I don't have permissions to do that. Thank you @xjm for bumping this to Major and considering it for 8.1 RC.

Cybercraft2003’s picture

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 ?

mikeker’s picture

@Cybercraft2003, at this point, the only way for this to work correctly is to apply the latest patch to core or wait for the fix to be accepted into core. The best way to move this forward it to test the patch and report your results here.

Thanks.

lokapujya’s picture

Status: Needs review » Reviewed & tested by the community

This seems done.

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 2651102-47-checkboxes-in-exposed-forms.patch, failed testing.

mikeker’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
10.99 KB

Rerolled #47.

Status: Needs review » Needs work

The last submitted patch, 52: 2651102-52-checkboxes-in-exposed-forms.patch, failed testing.

The last submitted patch, 52: 2651102-52-checkboxes-in-exposed-forms.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
10.99 KB

Not sure why the testbot is running the patch against 8.1, re-uploading in hopes that it'll run it against 8.2...

akalata’s picture

Status: Needs review » Reviewed & tested by the community
ipwa’s picture

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!

catch’s picture

Status: Reviewed & tested by the community » Needs work

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?'.

mikeker’s picture

Status: Needs work » Needs review
FileSize
10.98 KB
1.6 KB

@catch, Thank you for the review!

#1: Fixed. Also updated the @param comment to match that in getCheckedCheckboxes()
#2: Kinda... I figured I was changing the code in that function so I should update the docblock to meet coding standards. I can remove it -- let me know.
#3: Sorry, I don't understand...
#4: Fixed.

lokapujya’s picture

+++ 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

mikeker’s picture

@lokapujya, thanks! Didn't see that.

dawehner’s picture

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!

mikeker’s picture

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

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.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice

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.

gaurav.pahuja’s picture

Assigned: Unassigned » gaurav.pahuja
gaurav.pahuja’s picture

Assigned: gaurav.pahuja » Unassigned
Status: Needs work » Needs review
FileSize
11.4 KB
963 bytes

Giving it a try.

Lendude’s picture

Status: Needs review » Needs work
+++ 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

Sagar Ramgade’s picture

Removed the white space, patch and interdiff attached.

Status: Needs review » Needs work

The last submitted patch, 69: 2651102-69-checkboxes_in_exposed_forms.patch, failed testing.

mikeker’s picture

Status: Needs work » Needs review
FileSize
11.4 KB
590 bytes

I think the previous patch was diff'ed against a branch that already had an earlier patch applied, thus the newly added files were not marked as such. This is #67, minus the whitespace, diff'ed against 8.2.x.

mikeker’s picture

Sorry, misnamed the interdiff. It is against #67, not #69.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

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

Lukas von Blarer’s picture

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.

mikeker’s picture

@Lukas von Blarer, if the reset button is working correctly with the latest version of core, then the issue has been fixed between 8.1.2 and 8.2.x. It may be possible to backport that fix to 8.1.x, but that is not part of this issue and should be dealt with separately.

Thanks.

Lukas von Blarer’s picture

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed d1ae9b7 and pushed to 8.2.x. Thanks!

  • catch committed d1ae9b7 on 8.2.x
    Issue #2651102 by mikeker, amateescu, gaurav.pahuja, Sagar Ramgade,...

Status: Fixed » Closed (fixed)

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

thomas.fleming’s picture

Status: Closed (fixed) » Needs review
FileSize
10.04 KB

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.

thomas.fleming’s picture

Version: 8.2.x-dev » 8.1.x-dev

Status: Needs review » Needs work

The last submitted patch, 80: 2651102-80-reroll-checkboxes-in-exposed-forms-for-8-1.patch, failed testing.

thomas.fleming’s picture

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

thomas.fleming’s picture

Status: Needs work » Needs review
vivianspencer’s picture

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

Lendude’s picture

Status: Needs review » Needs work

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.

mikeker’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Closed (fixed)

Returning to what this issue was as of #79. If someone does the rewrite needed to get this into 8.1.x, please feel free to reopen.

Lukas von Blarer’s picture

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.

sukanya.ramakrishnan’s picture

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

sukanya.ramakrishnan’s picture

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

Thanks
Sukanya

sukanya.ramakrishnan’s picture

Status: Closed (fixed) » Needs review

Status: Needs review » Needs work
sukanya.ramakrishnan’s picture

Status: Needs work » Needs review

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

mikeker’s picture

@sukanya.ramakrishnan, this is a closed issue that has already been committed. Can you explain what you're trying to fix? I'm guessing this would be better handled in a separate issue.

dawehner’s picture

Status: Needs review » Closed (fixed)

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

sukanya.ramakrishnan’s picture

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!

xjm’s picture