Fatal error when trying to save a View with grouped filters using other than string values

This seems like a duplicate of #2595027: View crash on saving having exposed filter with groupings, different filter, same error. Curious if this fixes that issue too.

Patch didn't apply anymore. Minor reroll.

Small update of the issue summary to make this a bit more general.

Manually tested this for both date and permission filter and manually works for both. The test from #2595027: View crash on saving having exposed filter with groupings still fails though. No idea why. Added the test from that issue to this patch and closed that as a duplicate.

Putting on needs review to fire up the testbot, but this will fail.

It's the same issue as #2595027: View crash on saving having exposed filter with groupings? Closing this as duplicate.

I was wrong.

The above patch is working fine! +1 to RTBC, Not understanding why the bot getting failed.

I rolled the same patch with latest pull, Let see is that making any difference.

Patch in #25 worked for me as well on 8.0.5

I don't have time to help with this issue, but I did some research and it seems the plugin_id of the testing views loaded on the tests that are failing have some differences when created by the UI.

See:

http://cgit.drupalcode.org/drupal/tree/core/modules/views/tests/modules/...

It says: 'node_type'

But when the filter is created using the UI and saved, the plugin_id for type now says: bundle

I think you should review that part of the code, maybe the views.view.test_exposed_admin_ui.yml needs to be updated. But I'm not sure if this will make the tests pass.

My two cents.

#25 fixed the issue for me as well in 8.0.5

We should not have failing tests to allow this to be set as RTBC.

It worked well with drupal core 8.0.5 but it doesn't work with 8.1.0-rc1. That's why I bumped version to 8.2.x-dev.

I disagree. This is a very ugly bug that should be fixed in a patch release of the current minor. So, 8.1.x.

#2718353: Views group filter on user created won't save set as duplicated of this one.

+1 to fix in 8.1, very annoying bug

checking file core/modules/node/config/optional/views.view.content.yml
checking file core/modules/user/config/optional/views.view.user_admin_people.yml
checking file core/modules/views/config/schema/views.data_types.schema.yml
checking file core/modules/views/config/schema/views.filter.schema.yml
checking file core/modules/views/src/Plugin/views/filter/Date.php
Hunk #1 succeeded at 131 (offset -5 lines).
checking file core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
Hunk #1 succeeded at 1000 (offset -9 lines).
Hunk #2 succeeded at 1261 (offset -10 lines).
Hunk #3 succeeded at 1358 (offset -10 lines).
checking file core/modules/views/src/Tests/Handler/FilterDateTest.php
Hunk #1 succeeded at 2 (offset -5 lines).
Hunk #2 succeeded at 11 (offset -5 lines).
Hunk #3 succeeded at 48 (offset -5 lines).
Hunk #4 succeeded at 156 (offset -5 lines).
checking file core/modules/views_ui/src/Tests/ExposedFormUITest.php
Hunk #1 FAILED at 174.
1 out of 1 hunk FAILED

rerolled

It was missing a schema for the bundle filter. Added that. This is now passing locally for me.

Take a look to the status filter for admin/content, this is the default if you install drupal with this patch and click on the exposed status filter.

Same situation applies fro admin/people/list

I cannot replicate locally the failure reported by automated tests.

I'm adding a new test to check the scenario I posted in the previous screenshot that fails locally.

I cannot replicate locally the failure reported by automated tests.

Weird, Drupal\system\Tests\Update\UpdatePostUpdateFailingTest fails for me locally too.

+++ b/core/modules/node/config/optional/views.view.content.yml
@@ -400,11 +400,11 @@ display:
           plugin_id: boolean

Can't really rhyme the plugin_id with the set values. That's where this seems to be going wrong now. But haven't found a fix yet either.

Sorry, I was testing the wrong Testcase. Anyway now we have more test coverage.

@dagmar the extra test is exactly what was needed because it tests the boolean exposed filter. Undid a bit of work that olli did in #8 because the fix there was never going to work like that. The problem with the boolean filter was that 'boolean' is fine for the non-exposed filter, you only need to have 2 settings there, but it wasn't going to work for the exposed filter (it needs yes-no-all). You just can't fit three options into a boolean.
So we need a way to make the exposed filter use a different config then the non-exposed filter. So views.filter.group_item.* is back, with some overrides for filters where the 'value' is a different format for exposed and non-exposed filters.

All previous failing test now pass locally for me, so lets see what happens here.

And for more fun with boolean filters, check out #2459289: Boolean default values are not saved

Took out a little too much.

Thanks for the feedback olli!

54.1 I agree, the override should be on the exposed filter, but as far as I know there is no way to currently override the format for the exposed filter. So adding that here feels like a bit of a scope creep. So I'd say just focus here on getting the grouped filters working with the current fix, then in #2459289: Boolean default values are not saved see what we can do about overriding the format for the exposed filter (or just convert all the boolean filters to strings since that is the only filter that I see that needs a different exposed filter).
I'll post some of our findings there too.
Unless I'm wrong and there is a way to override the exposed filter format, then yeah we should totally use that!

54.2 Absolutely right

54.3 Yeah makes more sense

1. +++ b/core/modules/views/config/schema/views.filter.schema.yml
   @@ -94,7 +94,7 @@ views.filter_value.numeric:
    views.filter_value.equality:
   -  type: views.filter_value.numeric
   +  type: views.filter_value.string
      label: 'Equality'
   

   IMHO config schema should simply mirror the class hierarchy. For that, it would be great if this type would be actually its own thing.

2. +++ b/core/modules/views/src/Tests/Handler/FilterDateTest.php
   @@ -153,4 +156,83 @@ protected function _testUiValidation() {
   +    $edit['options[group_info][group_items][1][title]'] = 'simple-offset';
   +    $edit['options[group_info][group_items][1][operator]'] = '>';
   +    $edit['options[group_info][group_items][1][value][type]'] = 'offset';
   +    $edit['options[group_info][group_items][1][value][value]'] = '+1 hour';
   +    $edit['options[group_info][group_items][2][title]'] = 'between-offset';
   +    $edit['options[group_info][group_items][2][operator]'] = 'between';
   +    $edit['options[group_info][group_items][2][value][type]'] = 'offset';
   +    $edit['options[group_info][group_items][2][value][min]'] = '+1 hour';
   +    $edit['options[group_info][group_items][2][value][max]'] = '+2 days';
   +    $edit['options[group_info][group_items][3][title]'] = 'between-date';
   +    $edit['options[group_info][group_items][3][operator]'] = 'between';
   +    $edit['options[group_info][group_items][3][value][min]'] = format_date(150000, 'custom', 'Y-m-d H:s');
   +    $edit['options[group_info][group_items][3][value][max]'] = format_date(250000, 'custom', 'Y-m-d H:s');
   

   Nice test coverage!

Unless I'm wrong and there is a way to override the exposed filter format, then yeah we should totally use that!

Yeah I'm not sure either!

In general for me its not that much of scope creep to be honest, its still kinda related to the issue, but sure, please go with what you prefer.

@olli oh yeah that looks like a nice addition.

I've rearranged the schema in views.filter.schema.yml a bit because there was a header for '# Schema for the views filter value' but they were actually scattered throughout the entire file which was driving me nuts. So all filter values now fall under that heading. Makes the patch and interdiff a little harder to read, but makes the actual file a lot easier to read, so thought it was worth it.

So the only odd duck in the file is now

views_filter_boolean_string:
  type: views_filter
  label: 'Boolean string'

That sounds like what the exposed boolean filter should be using, but then it really should be views.filter.boolean_string and it should actually do something other then just inheriting views_filter I think. Maybe something to look at in #2459289: Boolean default values are not saved

But, let's see what this does.

The latest patch in 58 failed for me when i chose a between operator for a grouped filter on a date field.

InvalidArgumentException: The configuration property display.default.display_options.filters.field_from_date_value_1.value.min doesn't exist. in Drupal\Core\Config\Schema\ArrayElement->get() (line 74 of core/lib/Drupal/Core/Config/Schema/ArrayElement.php).
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters.field_from_date_value_1.value.min', '') (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters.field_from_date_value_1.value', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters.field_from_date_value_1', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display', Array) (Line: 212)
Drupal\Core\Config\Config->save() (Line: 280)

Thanks for the feedback @sukanya.ramakrishnan. Not sure why this only revealed itself after adding the catch-all, but datetime needs its own thing.

This fixes the issue, but we need a test where we add a datetime field.

Setting to NW for the tests. Also bumping to 8.2.x as this patch adds strings.

Fixes the test that was broken in 8.2.x. Doesn't address the need for tests mentioned in #60 so this should go back to NW after the testbot runs.

Added test for datetime field, it fails without the change in #60.

Removing the 'needs tests' tag.

Can the test-only patch be uploaded to demonstrate the issue/fix?

+++ b/core/modules/views/src/Plugin/views/filter/Date.php
@@ -131,7 +132,15 @@ public function acceptExposedInput($input) {
-    $type = $this->value['type'];
+    $type = NULL;
+    if ($this->isAGroup()) {
+      if (is_array($this->group_info)) {
+        $type = $this->group_info['type'];
+      }
+    }
+    else {
+      $type = $this->value['type'];
+    }

@@ -154,8 +163,11 @@ public function acceptExposedInput($input) {
-    // restore what got overwritten by the parent.
-    $this->value['type'] = $type;
+    // Restore what got overwritten by the parent.
+    if (!is_null($type)) {
+      $this->value['type'] = $type;
+    }
+

Interesting, this same bit of code is being changed over in #2648950: [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter, although in a slightly different way. I'm going to move that issue back to NR to take a look at this approach.

With the YAML changes to the schemas, does this require an update path?

@mpdonadio, I don't think so. This patch should make existing Views config not be broken anymore without changes to the current config.

@jhedstrom absolutely, here it goes!

Back to Needs Review per the results of #66.

This needs an IS update to describe what is really going on and what this patch is fixing. What is a little odd about this situation, is the bug is most evident in the Views UI if you already have content created and then make the view. If you do it the other way around, then you get the error when the view is executed. I don't totally grok this patch to update the IS myself, though (I'm following this because I am working on the patch mentioned in #67).

Few nits with the patch.

1. +++ b/core/modules/views/src/Tests/Handler/FilterDateTest.php
   @@ -153,4 +182,115 @@ protected function _testUiValidation() {
   +    $edit['options[group_info][group_items][3][value][min]'] = format_date(150000, 'custom', 'Y-m-d H:s');
   +    $edit['options[group_info][group_items][3][value][max]'] = format_date(250000, 'custom', 'Y-m-d H:s');
   +
   

   Weird format, 'Y-m-d H:i:s' would be more normal.

2. +++ b/core/modules/views/src/Tests/Handler/FilterDateTest.php
   @@ -153,4 +182,115 @@ protected function _testUiValidation() {
   +  /**
   +   * Test datetime grouped filter.
   +   */
   +  protected function testFilterDatetimeUI() {
   +    $this->drupalLogin($this->drupalCreateUser(array('administer views')));
   

   Two things here. The docblock doesn't describe this test accurately. I think "Test datetime filter UI." would be more accurate. Also, the other tests use the weird underscore convention and get called from testDateFilter(). For consistency sake, this should probably do the same thing.

Thanks for the feedback @mpdonadio. Updated the IS to hopefully beter describe what is going on here.

72.1 yeah that format is strange, changed it.
72.2 updated to follow the _underscore format. You are right, lets stick to that (and since we don't need a fresh drupal install for this test anyway, this way saves on overhead)

Sorry, forgot the interdiff...too much fun at DevDays to keep track of things!

Took one more look at this. I think the IS is mostly OK, but could explain why there are also code changes in addition to the schema changes. Right now, it somewhat appears to have out-of-scope changes if you aren't familiar with the patch. Either way, I don't feel comfortable setting RTBC on this, as I am still scratching my head a bit on a few parts of the schema changes.

@mpdonadio I added little addition to the IS to account for the non-schema changes in the patch, do you think this covers it now?

Unfortunately got an error similar to #59 at first:

exception 'InvalidArgumentException' with message 'The configuration property display.default.display_options.filters.field_event_date_value.value.min doesn't exist.' in                                 [error]
/var/www/html/core/lib/Drupal/Core/Config/Schema/ArrayElement.php:74
Stack trace:
#0 /var/www/html/core/lib/Drupal/Core/Config/StorableConfigBase.php(179): Drupal\Core\Config\Schema\ArrayElement->get('display.default...')
#1 /var/www/html/core/lib/Drupal/Core/Config/StorableConfigBase.php(211): Drupal\Core\Config\StorableConfigBase->castValue('display.default...', '')
#2 /var/www/html/core/lib/Drupal/Core/Config/StorableConfigBase.php(211): Drupal\Core\Config\StorableConfigBase->castValue('display.default...', Array)
#3 /var/www/html/core/lib/Drupal/Core/Config/StorableConfigBase.php(211): Drupal\Core\Config\StorableConfigBase->castValue('display.default...', Array)
#4 /var/www/html/core/lib/Drupal/Core/Config/StorableConfigBase.php(211): Drupal\Core\Config\StorableConfigBase->castValue('display.default...', Array)
#5 /var/www/html/core/lib/Drupal/Core/Config/StorableConfigBase.php(211): Drupal\Core\Config\StorableConfigBase->castValue('display.default...', Array)
#6 /var/www/html/core/lib/Drupal/Core/Config/StorableConfigBase.php(211): Drupal\Core\Config\StorableConfigBase->castValue('display.default', Array)
#7 /var/www/html/core/lib/Drupal/Core/Config/Config.php(212): Drupal\Core\Config\StorableConfigBase->castValue('display', Array)
#8 /var/www/html/core/modules/locale/locale.module(384): Drupal\Core\Config\Config->save()
#9 /var/www/html/core/modules/locale/locale.module(316): locale_system_set_config_langcodes()

Steps taken:
- Applied the latest patch on 8.1.3 (worked nicely)
- Ran all the unit tests, no errors there
- Tried to do a new site install with importing the existing views through config yml files.
Exception only occurred when I did a drush si with --locale=XX.
A site install in English gave no issues with the installation, but saving the view through UI after the install gave the Exception you see above as well.
- Tried to save the same existing view with the following settings: Screenshot settings

After that I decided to remove the filter, add it again and export it through the UI to see any difference in the YML files.
Noticed that there was a diff on plugin_id: datetime added to the exported yml file. Which resulted in having no errors anymore :).
Not sure if it is because of outdated yml files or anything that the plugin_id wasn't there before.

After upgrading to Drupal 8.1.7 and applying the latest patch of this issue, I still get an error when saving one of my views, which includes a geolocation field filter:

The configuration property display.default.display_options.filters.field_geo_location_proximity.value.min doesn't exist.

I don't know if this a Drupal core issue or an issue of the contrib module geolocation.

This error also occurs on configuration import of the view. This is the filter part:

      field_geo_location_proximity:
          id: field_geo_location_proximity
          table: node__field_geo_location_proximity
          field: field_geo_location_proximity
          relationship: none
          group_type: group
          admin_label: ''
          operator: '<='
          value:
            min: ''
            max: ''
            value: '35'
            lat: '52.520015'
            lng: '13.404954'
            units: km
          group: 1
          exposed: true
          expose:
            operator_id: ''
            label: 'Distance (in kilometers)'
            description: 'Lat:52.5234146 Long:13.3702304 '
            use_operator: false
            operator: ''
            identifier: POI
            required: false
            remember: false
            multiple: false
            remember_roles:
              anonymous: anonymous
              authenticated: authenticated
              admin: '0'
          is_grouped: false
          group_info:
            label: ''
            description: ''
            identifier: ''
            optional: true
            widget: select
            multiple: false
            remember: false
            default_group: All
            default_group_multiple: {  }
            group_items: {  }
          expose_units: 0
          plugin_id: geolocation_filter_proximity

I'm only able to import the view configuration successfully, if I remove display.default.display_options.filters.field_geo_location_proximity.value from the configuration file.

If I only set display.default.display_options.filters.field_geo_location_proximity.value.max or something different I also get the appropriate error:

The configuration property display.default.display_options.filters.field_geo_location_proximity.value.max doesn't exist.

Unfortunately I cannot reproduce this error on a fresh installation. Can somebody please give me a clue, where to look for the error cause? I guess it's an configuration schema issue but I'm not familiar with this topic.

@pminf yes this looks like a configuration issue in geolocation. The config that defines the views filter should also define the views filter value.

+++ b/core/modules/views/config/schema/views.filter.schema.yml
@@ -128,6 +111,43 @@ views.filter_value.boolean:
+views.filter_value.numeric:
+  type: mapping
+  label: 'Numeric'
+  mapping:
+    min:
+      type: string
+      label: 'Min'
+    max:
+      type: string
+      label: 'And max'
+    value:
+      type: string
+      label: 'Value'

something along these lines but then geolocation specific.... But that will only start working with this patch applied. So I'd advice to open an issue for geolocation with a link to this issue.

Works for me on a Content Type filter.

@Lendude: Thanks for the info. I opened a ticket: Core patch for views grouped filters results geolocation config error

The generated query for grouped, exposed content type filters looks like this:

SELECT node_field_data.nid AS nid
FROM 
{node_field_data} node_field_data
WHERE (( (node_field_data.type IN  ('typea', 'typeb', 'typec', '0', '0', '0', '0', '0', '0', '0', '0', '0', '0', '0', '0', '0', '0', '0', '0')) ))
ORDER BY node_field_data.changed DESC
LIMIT 51 OFFSET 0

Looks like a zero entry for each type excluded from the group.

@Grayside, you think this is because of the patch or because of an unrelated grouping issue (since this patch is mostly config changes I wouldn't expect this, but who knows)? It looks like the query should work (it doesn't look great, but if it gets the right result set I can live with a follow up issue).

Works good for me

should be fixed in upstream first
we using this patch for a time

Looks like this needs to be re-rolled against 8.2.x

It seems the patch needs reroll because #2686483: Add support for database condition operator "NOT BETWEEN" was committed and changed core/modules/views/src/Tests/Handler/FilterDateTest.php

back to rtbc

This bug exists in 8.1.x as well. Any plans for a backport? This is blocking functionality I need for a client. :-/

@seanr, Once this gets committed to 8.3.x it can be backported assuming there are no string changes that would prevent that. If not, you could still apply the patch manually.

FWIW, the patch applies cleanly to the latest head of 8.3.x; rerunning the tests against 8.3.x.

Unrelated fails, back to RTBC

The testbot for 8.3 passes.

Has anyone tried this with a list of integers or a list of texts?

I am trying to create an exposed filter as a checkbox. So I'm using the trick with grouped filters, allowing multiple selection (to show checkboxes) but only creating 1 group.

When I create a field "field_shower" which is either

a list of text with the values:

shared|Shared
private|Private

or a list of integers with the values:

0|Shared
1|Private

the grouping doesn't work and fails with:

InvalidArgumentException: The configuration property display.default.display_options.filters.field_shower_value.group_info.group_items.1.value.1 doesn't exist. in Drupal\Core\Config\Schema\ArrayElement->get() (line 74 of core/lib/Drupal/Core/Config/Schema/ArrayElement.php).

and

InvalidArgumentException: The configuration property display.default.display_options.filters.field_shower_value.group_info.group_items.1.value.shared doesn't exist. in Drupal\Core\Config\Schema\ArrayElement->get() (line 74 of core/lib/Drupal/Core/Config/Schema/ArrayElement.php).

respectively.

@Jeroen is this after applying the patch? Because for any filter that isn't a straight up string grouping will fail without this patch.

Looks like this patch needs a re-roll after https://www.drupal.org/node/2459289 has been commited

I believe that this would cut it for a re-roll

Gah, forgot to change back to NR.

Manually compared the patches, and looks like a good reroll.

Regarding #101. I tried that on simplytest.me and I couldn't replicate the error.

I asked something similar back in #68, but since there are schema changes, does this need an empty hook_update_N() to force a rebuild so the new schema gets picked up even though it just makes the existing config work (and not require an update path)? TBH, I don't recall these discussions from last year (see #2507899: [policy, no patch] Require hook_update_N() for Drupal 8 core patches beginning June 29 for discussion and jumping off points).

So if it works- why do we have to wait until 8.3 ? Would love to see it in the next core release ...

A committer is going to have to make the decision on whether this can go into 8.2.x (I am tentatively setting it back). The policy is at https://www.drupal.org/core/d8-allowed-changes

We are current in RC, so it is not currently eligible w/o a framework manager marking it eligible. After 8.2 is release, it should be able to go in. Bugfixes are allowed, and there isn't anything disruptive in this.

Back to RTBC.

Given that produces an exception, I think this is actually a major issue.

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -1028,17 +1028,20 @@ protected function buildExposedFiltersGroupForm(&$form, FormStateInterface $form
       $children = Element::children($row['value']);
       if (!empty($children)) {
         foreach ($children as $child) {
-          foreach ($row['value'][$child]['#states']['visible'] as $state) {
-            if (isset($state[':input[name="options[group_info][group_items][' . $item_id . '][operator]"]'])) {
-              $row['value'][$child]['#title'] = '';
+          if (!empty($row['value'][$child]['#states']['visible'])) {
+            foreach ($row['value'][$child]['#states']['visible'] as $state) {
+              if (isset($state[':input[name="options[group_info][group_items][' . $item_id . '][operator]"]'])) {
+                $row['value'][$child]['#title'] = '';
 
-              if (!empty($this->options['group_info']['group_items'][$item_id]['value'][$child])) {
-                $row['value'][$child]['#default_value'] = $this->options['group_info']['group_items'][$item_id]['value'][$child];
+                // Exit this loop and process the next child element.
+                break;
               }
-              // Exit this loop and process the next child element.
-              break;
             }
           }
+
+          if (!empty($this->options['group_info']['group_items'][$item_id]['value'][$child])) {
+            $row['value'][$child]['#default_value'] = $this->options['group_info']['group_items'][$item_id]['value'][$child];
+          }
         }

This code is so unmaintainable :(

+++ b/core/modules/views/config/schema/views.filter.schema.yml
@@ -79,24 +67,6 @@ views.filter.string:
-views.filter_value.numeric:
-  type: mapping
-  label: 'Numeric'
-  mapping:
-    min:
-      type: string
-      label: 'Min'
-    max:
-      type: string
-      label: 'And max'
-    value:
-      type: string
-      label: 'Value'

@@ -128,6 +111,43 @@ views.filter_value.boolean:
+views.filter_value.numeric:
+  type: mapping
+  label: 'Numeric'
+  mapping:
+    min:
+      type: string
+      label: 'Min'
+    max:
+      type: string
+      label: 'And max'
+    value:
+      type: string
+      label: 'Value'

This change is mixing bug fixes with moving stuff around. Making it super hard to review and work out what is a fix and what is just a move. Makes it super hard to review. Is there anyway to split the fixes and moves up into different patches? Afaik the order in the YAML file should not be significant unless there are duplicates.

So the patch attached here makes it much easier to work out what is actually changing in the .schema.yml and we can file a followup to make the ordering more sensible. Fixed a couple of nits too.

+++ b/core/modules/views/config/schema/views.filter.schema.yml
@@ -109,18 +112,20 @@ views.filter.standard:
-views.filter.group_item.numeric:

So this is the only schema removal. This has been debated above (good to see). It'd be nice to confirm this has test coverage.

We have some grouped numeric filter tests:

    // Ensure the string "0" can be used as a value for numeric filters.
    $this->drupalPostForm('admin/structure/views/nojs/add-handler/test_exposed_admin_ui/default/filter', array('name[node_field_data.nid]' => TRUE), t('Add and configure @handler', array('@handler' => t('filter criteria'))));
    $this->drupalPostForm(NULL, array(), t('Expose filter'));
    $this->drupalPostForm(NULL, array(), t('Grouped filters'));
    $edit = array();
    $edit['options[group_info][group_items][1][title]'] = 'Testing zero';
    $edit['options[group_info][group_items][1][operator]'] = '>';
    $edit['options[group_info][group_items][1][value][value]'] = '0';
    $this->drupalPostForm(NULL, $edit, t('Apply'));
    $this->assertUrl('admin/structure/views/view/test_exposed_admin_ui/edit/default', array(), 'A string "0" is a valid value.');
    $this->assertNoGroupedFilterErrors();

    // Ensure "between" filters validate correctly.
    $this->drupalGet('admin/structure/views/nojs/handler/test_exposed_admin_ui/default/filter/nid');
    $edit['options[group_info][group_items][1][title]'] = 'ID between test';
    $edit['options[group_info][group_items][1][operator]'] = 'between';
    $edit['options[group_info][group_items][1][value][min]'] = '0';
    $edit['options[group_info][group_items][1][value][max]'] = '10';
    $this->drupalPostForm(NULL, $edit, t('Apply'));
    $this->assertUrl('admin/structure/views/view/test_exposed_admin_ui/edit/default', array(), 'The "between" filter validates correctly.');
    $this->assertNoGroupedFilterErrors();

So I think that should do for the bit that got removed?

Rerolled @alexpotts patch so that it's passing again locally. A little too much got removed (which is why I reordered it all in the first place, because it was really hard to keep track of what needed to go where).

+++ b/core/modules/views/config/schema/views.filter.schema.yml
@@ -97,7 +97,7 @@
-views.fiter_value.equality:
+views.filter_value.equality:

This override didn't do anything because of the typo in the name and it was just falling back on defaults which were good, without the typo it needs to have a type.

Well the failing tests of #115 and #118 at least prove we have some test coverage!

Discussed with @alexpott on IRC, we could use some more explicit test coverage of buildExposedFiltersGroupForm() changes.

Also we should really have a javascript test for the #states stuff but that could be moved to a follow-up since that would add some major complexity to fixing this.

While we're waiting on getting this fixed is there at least a workaround for this?

@matthew.h applying the patch, thats about all you can do. It should work fine, the only thing missing is additional test coverage.

Since the changes to buildExposedFiltersGroupForm are all about showing the right fields with the right values, the easiest way to check this is with a javascript test after all. So a fairly basic test to check if the right fields are shown with the right values.

It would be better to also set these values in the test, but phantomJS dies when trying to 'Apply' the exposed filter values (see also #2754985: [backport] Add JavaScript test coverage for adding an exposed filter in Views UI):
Zumba\GastonJS\Exception\BrowserError: There was an error inside the PhantomJS portion of GastonJS.
This is probably a bug, so please report it:
click
html.touchevents.details.js body.user-logged-in.path-admin div.ui-dialog.ui-widget.ui-widget-content.ui-corner-all.ui-front.ui-dialog-buttons.views-ui-dialog-scroll.views-ui-dialog.js-views-ui-dialog div#drupal-modal.ui-front.ui-dialog-content.ui-widget-content div form#views-ui-config-item-form--a8Az0vK1h_o div#edit-options--TXBWT3fLHBE.scroll.js-form-wrapper.form-wrapper

so we have to make due with pre-set values.

1. Reproduced the error : Please see the screen.
2. Applied the patch in #125 , it worked cleanly . Now one group have value type "A set..."
3. When I save the view it still gives me the same error.

You need to clear cache after applying the patch or the new config schema won't be used.

If we need to clear caches after the patch is applied, then we need an empty update hook to do the same thing to ensure it happens when users update core.

Also a minor tweak to avoid a legacy assertion in a new test class.

A quick look at the new test looks good to me.

If we need to clear caches after the patch is applied, then we need an empty update hook to do the same thing to ensure it happens when users update core.

Yeah I realised the same thing 5 seconds after posting that comment.

Also a minor tweak to avoid a legacy assertion in a new test class

Heh, didn't know that was on AssertLegacyTrait, not all the individual methods are marked as deprecated, so didn't show up in (my version of) PHPStorm.

@mpdonadio++

After clear cache it worked . Thanks

I'm getting the same issue mentioned by @Jeroen (comment #101) after applying the patch and clearing cache. My issue is when using a grouped filter on a List (text) type of field.

*sigh* yup views.filter.list_field is broken too.

Attempt at a fix for that + test. The test fails but the fix works when manually testing this, so probably still something wrong with the test, but not clear headed enough to get it working right now, so here is the progress so far.

Test and hopefully fix.

Patch in #134 fixed the Exception for me (trying to save a view with grouped exposed filter on a timestamp field).
This seems to be a valid solution, hope to see it get into core soon.

Thanks, Lendud!

Patch #134 is working for me with a grouped filter on a List (text) type of field. Thanks!

Unfortunately, #134 did all sorts of nasties for me, including breaking a custom numeric field extension without min and max values, while still not solving the grouped exposed filter problem (grouped roles filter).

breaking a custom numeric field extension without min and max values

@Andrej Galuf did you have a config schema for that custom field extension? If yes, what did you need to change to get it to work? If no, did adding a config schema resolve your issue?

And I'm sure there are more grouped filters that are not working yet, but we need to get this in to at least have a basis on which to solve those issues.

did all sorts of nasties for me

Anything besides these things? Being specific helps find the issues and solving them...

Patch #134 doesn't work for me.
I've got an exposed grouped filter on an Entity Ref field. I manually built a select list from Label|nid. It works fine until the remember option is set, then it throws a "An illegal choice has been detected. Please contact the site administrator." when the page is refreshed.

@boinkster that is unrelated to what we are trying to fix here, this is about saving the grouped filter and getting a WSOD. Please search the issue queue for your problem and open a new issue for that if you can't find anything related. Did a really quick search but could only find some D7 issues that appear to be the same problem.

Thanks, I'll open another issue (I found the same D7 post). For my use case, I wasn't getting a WSOD even with unpatched core.

Same issue when using a taxonomy field as grouped exposed filter, #134 won't fix that. Guess all numeric field types has to be fixed or we can have some solution can handle all types of content.

Don't have time for this right now, but per @catch:

We shouldn't use hook_update_N() for empty updates to trigger a cache flush - it adds all the potential complexity of schema versions between minor branches, conflicts with other patches, inconsistencies if there's a revert etc. hook_post_update_NAME() achieves the same thing without most of that trouble.

And per @catch the post-update hook can be empty in this case to achieve the same thing.

OK, got my computer back.

Thanks for the update @mpdonadio, nice to know what to do in such cases.

This should fix any filters inheriting views.views_filter.in_operator and views.views_filter.many_to_one as the removal of the fix but passing test from #134 should indicate.

This should fix the grouped filter issues indicated in #137 and #142.

I've tested latest patch, it seemed to fix the roles issue in #134, but for taxonomy, the view could save correctly, however the sql query generated is mistaken. Instead of a where clause with corresponding taxonomy term ids, the sql generated became the group ids.

@youfei.sun the generated sql issue sounds unrelated to what we are trying to fix here, so please look around and see if you can find an issue for that and open one if you don't.

@Lendude you are right, that belongs to here https://www.drupal.org/node/2049603.

I've got a patch uploaded for d8 to this issue.

I've applied the patch in #145 and was able to resolve the issue. I have not completed a thorough review, but from the surface, seems to be OK.

Tested Patch #145 and was able to solve the issue.

Applied patch #145 and was able to fix the related issue Creating a view with exposed filters fails on save

I had a custom query's problem. I found the following file which have force fully added "ONLY_FULL_GROUP_BY"

core/lib/Drupal/Core/Database/Driver/mysql/Connection.php

'sql_mode' => "SET sql_mode = 'ANSI,STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,ONLY_FULL_GROUP_BY'"

I removed the "ONLY_FULL_GROUP_BY" from the file then my module run as expected.

Thanks

I'm facing the same problem!!! I want to add a group filter to my view. Users should be able to filter by content type.
The preview is working fine but when I try to save my view, it runs to an exception error. I have no idea where the problem is. I'm using Drupal 8.2.2

The website encountered an unexpected error. Please try again later.

InvalidArgumentException: The configuration property display.default.display_options.filters.type.group_info.group_items.1.value.acc_grp doesn't exist. in Drupal\Core\Config\Schema\ArrayElement->get() (line 74 of core/lib/Drupal/Core/Config/Schema/ArrayElement.php).

Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters.type.group_info.group_items.1.value.acc_grp', 'acc_grp') (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters.type.group_info.group_items.1.value', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters.type.group_info.group_items.1', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters.type.group_info.group_items', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters.type.group_info', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters.type', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display', Array) (Line: 212)
Drupal\Core\Config\Config->save() (Line: 280)
Drupal\Core\Config\Entity\ConfigEntityStorage->doSave('group_naviagation_view', Object) (Line: 392)
Drupal\Core\Entity\EntityStorageBase->save(Object) (Line: 259)
Drupal\Core\Config\Entity\ConfigEntityStorage->save(Object) (Line: 364)
Drupal\Core\Entity\Entity->save() (Line: 637)
Drupal\Core\Config\Entity\ConfigEntityBase->save() (Line: 986)
Drupal\views_ui\ViewUI->save() (Line: 312)
Drupal\views_ui\ViewEditForm->save(Array, Object)
call_user_func_array(Array, Array) (Line: 111)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object) (Line: 51)
Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object) (Line: 585)
Drupal\Core\Form\FormBuilder->processForm('view_edit_form', Array, Object) (Line: 314)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 48)
Drupal\Core\Entity\EntityFormBuilder->getForm(Object, 'edit', Array) (Line: 226)
Drupal\views_ui\Controller\ViewsUIController->edit(Object, NULL)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
call_user_func_array(Object, Array) (Line: 144)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 64)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 652)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

@remram did you apply the patch in #145 (see Applying patches)? That should resolve your issues.

@Lendude: I'm not the only one, who is working on this project. I will try it in the near future. Thank you so far :)

Another +1 for patch in #145 fixing my issue: I was unable to save a view that had grouped filters on a taxonomy term field.

Cheers!

I don't feel that I'll have the skills to review this, even though I've made a few re-rolls, so can't put it to RTBC.

Any chance that anyone else could, so that this would make it to 8.3?

As talked with @lendude this issue fixes a lot of problems already. There might be more scheme issues in other filters, but creating new issues is easy :)
I documented this in the issue summary as well.

This solves a lot of problems for a lot of people. The SQL errors are unrelated to this issue. Please open up new issues so we can fix them as wel..

Yes, RTBC++

Successfully using the last patch as well.

Unrelated fails

Not sure who triggered a retest, but the latest fail was b/c #2828559: UpdatePathTestBase tests randomly failing, which is unrelated. In this case, we don't need to re-test. We should let a committer decided what to do.

@mpdonadio I triggered the retest, it failed on the same test twice, just wanted to be absolutely sure it was still passing before starting to spam everybody in the issue with more RTBC->'needs work' flip-flop messages.

But it's green now, so I'm happy!

deleted - please ignore

I was having the same issue ( D8 ), unable to save the view having a exposed grouped filter.
Applied the patch and I was able to save the view.

Unfortunately it didn't work as expected.
I'm using a taxonomy in my grouped filter - to limit the terms to be displayed on the dropdown -.
The dropdown displays the titles correctly but the values selected for the grouped option are not going to the query correctly.

As sample on my grouped filter interface added two options:
Option1 equal to 54 (tid of the term selected)
Option2 equal to 69 (tid of the term selected)

But when I try to filter the view using those options, the results are not the correct ones.
Checked the query itself and the condition for that filter uses the values of the dropdown (incremental pointer) instead of the real values:
Option1 --- taxonomy_target_id = 1 (instead of 54)
Option2 --- taxonomy_target_id = 2 (instead of 69)

Also, something else that I noticed is that it is not passing more than one value.

Added a grouped option selecting two values, and when I checked the query still only passing one value and the wrong one.
Attached three images for clarity:
1.- grouping filter interface
2.- kint displaying the filter elements.
3.- View Query with an option selected on the filter. (from my previous comment).

Applied the patch and I was able to save the view.

This patch is about that. This is not about solving all problems with all grouped filters in one patch. Updated the title to clarify what we are trying to fix here.

@Blanca.Esqueda please open follow ups for the issues you describe and postpone them on this issue. We need to get this in first before we can start fixing any underlying problems.

Back to RTBC for the fix for the fatal.

@Lendude, the test ExposedFormUITest::testExposedGroupedFilter() has been created as part of duplicate issue #2595027: View crash on saving having exposed filter with groupings. It would be nice if that will be credited too in this ticket.

@claudiu.cristea++

Yes @maintainers please also take #2595027: View crash on saving having exposed filter with groupings into consideration when assigning credit.

@Lendude - what's the status with filters grouped by Content Type?

Patch 145 - NB: on a "used", rather than "clean", 8.2.5 install, with a rebuilt cache – doesn't seem to fix it for me; I see #80 saying it's fixed and #153 saying it isn't.

Like #153, works fine in the preview, fails to save changes.

I've a simple grouping on Content: Content Type using the 'is one of' operator for both groups.

The missing config property obviously matches the first checked content type in the first group of the filter.

Anything else I can try to help you?

@wturrell patch works for me.

With a filter set up like this for the Content View:

Without the patch I get the fatal, applied the patch, drush cr, reload page, save View, save successful.

Sorry this was my fault - the patch wasn't being applied, now I've done so, 2369119-145.patch works on the old site and also on a clean 8.2.x install (grouping Content Type by articles and basic pages).

[Rest of this is newbie explanation and can be skipped by more clever people..]

On the failing site I was using 'git apply' on a drupalvm install (with drupal-composer/drupal-project 8.x branch) and I was getting no output other than [ok], and then I realised no files were changing and I wasn't getting any output for git apply --verbose, git apply --summary or git apply --check either..

I switched from 'git apply' to patch -p1 after reading the only explanation I found, here.

git apply doesn't even try if it's already in a repository (other than the one for the project the patch is for)

Which I take to mean you can only apply patches to https://git.drupal.org/project/drupal.git, otherwise it will silently skip all the commit ID hashes that don't match? (That said, I'd hoped git apply --check might give me some output, seeing as the help text says it's to check if it's applicable...)

Also, on your screenshot, what is it you're filtering on to get 'node' and 'other' as the only two options?

(Thanks)

I'm just re-uploading the patch from #145 so we have clean test runs, and we have had a ton of comments since then.

I'm triaging this at the NJ Camp. #2474049: [meta] Major issue triage

I went through the instructions in #2474049: [meta] Major issue triage and it gets all the way to adding the triaged for d8 major current state. (has duplicates marked, summary is up to date, patch applies .. oh, 8.4.x vs 8.3.x, it does apply to 8.4.x... flow chart says to retest, since this is rtbc, it has been being retested on .. hm 8.2.x so I added custom test runs for retesting on 8.3.x and 8.4.x. leaving the version in the meta data for this issue on 8.3.x, partly cause the major issue triage doesn't say to evaluate that field.)

Not needed as part of the major triage meta, but I also checked into if this was major.
And according to https://www.drupal.org/core/issue-priority#major-bug

Trigger a PHP error through the user interface, but only under rare circumstances or affecting only a small percentage of all users, even if there is a workaround.

Cause this is a views issue that deals with database things (and cause someone already decided that the issue should be tested (when it was 8.2.x) on other databases, also adding custom test runs for those on 8.3.x and 8.4.x.

----

Taking a break for now. Next, looks like this could use a code review (especially to make sure that the feedback from alex was addressed).

I can confirm that my feedback has been addressed. The additional test coverage has been added and changes have been made to keep the schema in line with class inheritance.

And having a javascript test of this form in core gives us more than we've ever had before.

It's important we get an answer for @Grayside's question in #82

@alexpott yeah that happens:

Steps to reproduce:
- Apply the patch
- Make two content types
- Make a grouped filter for content type
- Add one group 'is one of' for one content type
- Add one group 'is one of' for the second content type
- Save
- Filter using one of these groups and you wil see IN('node_type_1' ,'0', '0') in the query.

This sounds like work for a follow-up though. This doesn't sound like it is caused by this patch, but it looks like this is an existing bug that just gets exposed by fixing grouped filters in general. I actually expect more issues with grouped filters to float to the surface after this is committed because this will actually allow you to create them....

@alexpott quick check, and yeah this is caused by \Drupal\views\Plugin\views\filter\InOperator::opSimple not cleaning the empty checkboxes values:

This:
$this->query->addWhere($this->options['group'], "$this->tableAlias.$this->realField", array_values($this->value), $this->operator);
Should be something like:
$this->query->addWhere($this->options['group'], "$this->tableAlias.$this->realField", array_filter(array_values($this->value)), $this->operator);

But since 0 is a valid value, it needs more logic then that. (and tests of course)

But like I said, I feel this is unrelated and should not hold up this issue.

@Lendude well I think we need to filter the disabled checkboxes before we save the view to config rather than when we are constructing the query - can you open a followup for this?

They are added in Drupal\views\Plugin\views\filter\FilterPluginBase::buildGroupSubmit(), there seems to be a provision for filtering empty values, check: Drupal\views\Plugin\views\filter\FilterPluginBase::hasValidGroupedValue():

$actual_values = count(array_filter($group['value'], 'static::arrayFilterZero'));

so it could be as simple as using it in buildGroupSubmit(), one line fix attached, decide if a separate issue is needed.

On second thought... ;)

Follow up for the InOperator issue: #2857740: Views grouped filters that use InOperator add '0' to the query for every not selected option

@tacituseu please move your proposed fix there, it looks like a great start off point. But it would require tests and it's unrelated to the config change we are doing here, so lets not scope creep it in.

Rebased post array-all-the-things

#189 applies but didn't change the array() notations of the changes in the patch

reroll using https://github.com/thomasbachem/php-short-array-syntax-converter as described in https://www.drupal.org/node/2776975#comment-11969184

Committed 594fdb7 and pushed to 8.4.x. Thanks!

I've discussed this issue with @catch, @cilefen and @xjm and we agreed to set this to patch to be ported and consider for inclusion into the 8.3.0 release. Given the addition of an update function and fixes to configuration schema it'd be nice to not have to consider this for 8.3.1 and the number of comments on the issue reflect the fact that a lot of users are running into this issue.

It is really exciting to have javascript testing of the grouped exposed filter functionality. This is incredibly complex and fragile so it is no surprise we broke it.

Just to make sure, isn't this the only update function?

+ * Rebuild caches to ensure schema changes are read in.
+ */
+function views_post_update_grouped_filters() {
+  // Empty update to cause a cache rebuild so that the schema changes are read.
+}

If so I'm much less concerned about this going into 8.3.x during the rc than if there was an actual data change. The schema changes make me not want to consider it for 8.3.1 though.

@catch yes the actual data that got saved was always correct, it just didn't match the config schema, hence the fatal.