Problem/Motivation

Steps to reproduce:

  1. Add a numeric field to user
  2. Edit the admin/people view
  3. Add a filter for the age field
  4. Expose it and group it (you'll have to click apply and re-edit the filter due to #2356259: Can not expose a filter immediately after adding a filter)
  5. Set up some groups (because of double escaping and a bug you'll have to apply #2356443: Grouped numeric filters can not have a value set and #2356297: Double escaping in views ui grouped filters)
  6. Apply the filter
  7. Save the view

This breaks because the value schema is incorrectly defined.

Proposed resolution

We fix numeric filter and filter group items config schema to match the configuration. This is not simple because the numeric filter value can either be a mapping or value at the moment. This type of polymorphism is not supported by config. Also the views_filter schema data type is incorrect because views.filter.group_items.[plugin_id] is never actually defined.

Remaining tasks

Commit it

User interface changes

None

API changes

Views config structure for views with numeric filter

CommentFileSizeAuthor
#44 2357145.44.patch8.6 KBYesCT
#44 interdiff.2357145.42.44.txt567 bytesYesCT
#42 2357145.42.patch8.63 KBolli
#33 2357145.33.patch8.63 KBolli
#28 interdiff.txt1.41 KBolli
#28 2357145.28.patch8.71 KBolli
#24 2357145.24.testonly.patch4.77 KBYesCT
#24 1879930.24.patch8.45 KBYesCT
#24 interdiff.2357145.23.24.txt1.64 KBYesCT
#23 2357145.23.patch8.34 KBYesCT
#22 2357145.22.patch105.83 KBalexpott
#22 21-22-interdiff.txt3.37 KBalexpott
#21 interdiff.txt3.06 KBolli
#21 2357145-check.patch7.3 KBolli
#19 2357145.19.patch10.37 KBalexpott
#19 11-19-interdiff.txt2.66 KBalexpott
#11 2357145.11.patch8.52 KBalexpott
#11 2357145.test-only.patch1.89 KBalexpott
#8 2357145.8.patch6.63 KBalexpott
#8 4-8-interdiff.txt2.99 KBalexpott
#4 2357145.4.patch4.36 KBalexpott
#2 2357145.2.patch312.96 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Title: Views can not be saved with a numeric filter » Views can not be saved with a numeric (grouped) filter

Adapting the title a bit. Thanks for filling all those issues!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
2357145.2.patch312.96 KB

Here's a first cut at a patch

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Active » Needs review
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
2357145.4.patch4.36 KB

Ignore #2

The last submitted patch, 2: 2357145.2.patch, failed testing.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented 
  1. +++ b/core/modules/views/config/schema/views.data_types.schema.yml
@@ -779,8 +779,12 @@ views_filter:
+            - type: views.filter.group_item.[%parent.%parent.%parent.plugin_id]

@@ -788,21 +792,19 @@ views_filter:
+views_filter_group_item:

    makes sense. 

  2. +++ b/core/modules/views/config/schema/views.filter.schema.yml
@@ -31,12 +31,12 @@ views.filter.combine:
-views.filter.date:
-  type: views.filter.numeric
+views.filter_value.date:

    Why are we removing filter? we can add views.filter_value.date, if it missing.

Status: Needs review » Needs work

The last submitted patch, 4: 2357145.4.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
4-8-interdiff.txt2.99 KB
2357145.8.patch6.63 KB

@vijaycs85 #2 - because the only thing that is different about the numeric filter is the value so falling back to views.filter.* is fine

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/views/src/Plugin/views/filter/Numeric.php
@@ -184,9 +184,7 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
-      // When exposed we drop the value-value and just do value if
-      // the operator is locked.
-      $form['value'] = array(
+      $form['value']['value'] = array(
         '#type' => 'textfield',
         '#title' => !$exposed ? $this->t('Value') : '',
         '#size' => 30,
@@ -231,7 +229,7 @@ protected function valueForm(&$form, FormStateInterface $form_state) {

@@ -231,7 +229,7 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
 
       if (!isset($form['value'])) {
         // Ensure there is something in the 'value'.
-        $form['value'] = array(
+        $form['value']['value'] = array(

This adds a regression, previously the URL for exposed filters looked like ?operator=value, now it contains the value as well. (yeah I manually tested this)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Re #9 yeah but we already have this in Drupal\views\Plugin\views\filter\Numeric::valueForm() when the form is not exposed or the operator not locked. This makes the configuration consistent.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Priority: Major » Critical
FileSize
2357145.test-only.patch1.89 KB
2357145.11.patch8.52 KB

Added a test that exposes the fatal error - actually because of that this is a critical. The interdiff is the test only patch :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

It'd be great to add far more test coverage of the numeric filter ui

The last submitted patch, 11: 2357145.test-only.patch, failed testing.

olli’s picture

olli CreditAttribution: olli commented

I agree with #9, I don't see why we need to change the exposed form. How can I reproduce the problem?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

re #14 to reproduce the problem the issue is solve follow the steps in the issue summary.

re #9 we can not have polymorphic variable types in config the numeric filter value can not be both a number or a mapping to value, min and max. The following two variants can not be described by the same schema:

value: 42

value:
  value:
  min: 41
  max: 43
olli’s picture

olli CreditAttribution: olli commented

I applied #11 and two patches from step 5, reverted changes in #9, followed steps 1-7 and didn't see a problem. Maybe I did something wrong but the way I see #9, that affects only (single) exposed filters when they are rendered at admin/people.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

re #16 you're not seeing a problem because there is no test to ensure that a view complies with its schema data type. And the value key exists - you're saving a number and the schema says it should be an array but that does not cause an error at the moment. What does cause an error is if you try to save value.value and the schema does not exist for that.

olli’s picture

olli CreditAttribution: olli commented

And the value key exists - you're saving a number and the schema says it should be an array but that does not cause an error at the moment.

This is what I was trying to reproduce by checking the single export page for the view I created with the setup #16. What kind of filter I need to add so that it would save a number rather than an array with value, min and max, or is it just impossible to reproduce this with the UI?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
11-19-interdiff.txt2.66 KB
2357145.19.patch10.37 KB

This is the situation in HEAD that would generate configuration where value is a scaler and not an array.

+    // Change the filter to a single filter to test the schema when the operator
+    // is not exposed.
+    $this->drupalPostForm('admin/structure/views/nojs/handler/test_view/default/filter/age', array(), t('Single filter'));
+    $edit = array();
+    $edit['options[value][value]'] = 12;
+    $this->drupalPostForm(NULL, $edit, t('Apply'));
+    $this->drupalPostForm('admin/structure/views/view/test_view', array(), t('Save'));
+    $this->assertConfigSchemaByName('views.view.test_view');
vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented 
+++ b/core/modules/views/src/Plugin/views/filter/Numeric.php
@@ -184,9 +184,7 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
+      $form['value']['value'] = array(

@@ -231,7 +229,7 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
+        $form['value']['value'] = array(

not sure why we need to have multiple of 'value'. but worth adding a comment why.

otherwise, +1 to RTBC.

olli’s picture

olli CreditAttribution: olli commented
FileSize
2357145-check.patch7.3 KB
interdiff.txt3.06 KB

Me too. Maybe bot can help us.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue tags: -Needs tests
FileSize
21-22-interdiff.txt3.37 KB
2357145.22.patch105.83 KB

I finally get what's happening here - the numeric form can be completely different if it is exposed on an actual view instead of the views ui. So when saving a filter it will always have the correct config structure. The key thing I missed was that:

      // When exposed we drop the value-value and just do value if the operator
      // is locked.

Will never occur in the views ui whilst configuring the filter.

Thanks @olli for pushing until I understood this :)

Improved the test to actually test filtering too.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
2357145.23.patch8.34 KB

this is just the patch from #21 with the interdiff from 22 applied on it.
(patch in #22 looks like it has unrelated changes in it.)

YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
interdiff.2357145.23.24.txt1.64 KB
1879930.24.patch8.45 KB
2357145.24.testonly.patch4.77 KB

just a couple nits (copy and paste error, and looked to see how white space was used usually in schema.)

to see why this test works, check out ViewTestData.php which has ages.

I'm not sure if I made the tests only patch correctly.

Status: Needs review » Needs work

The last submitted patch, 24: 2357145.24.testonly.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review

Test only patch failed as expected

olli’s picture

olli CreditAttribution: olli commented

Thank you for fixing this. I checked out ViewTestData.php and the test looks good to me. Two questions:

  1. +++ b/core/modules/views/config/schema/views.filter.schema.yml
@@ -121,17 +118,16 @@ views.filter.standard:
+views.filter.group_item.numeric:
+  type: views_filter_group_item
   label: 'Group items'
+  mapping:
+    value:
+      type: views.filter_value.numeric

    Should we add something similar for date here or in another issue? 

  2. +++ b/core/modules/views_ui/src/Tests/FilterNumericWebTest.php
@@ -0,0 +1,90 @@
+    $edit['options[group_info][group_items][3][title]'] = 'Not 25';
+    $edit['options[group_info][group_items][3][operator]'] = '!=';
+    $edit['options[group_info][group_items][3][value][value]'] = 25;

    Looks like this is not used in the test? Maybe add one min-max value?

olli’s picture

olli CreditAttribution: olli commented
FileSize
2357145.28.patch8.71 KB
interdiff.txt1.41 KB

Added a min-max value and a check for default values.

I'll open another issue for the date filter.

olli’s picture

olli CreditAttribution: olli commented

Here's the issue to improve grouped date filter #2369119: Fatal error when trying to save a View with grouped filters using other than string values.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Status: Needs review » Reviewed & tested by the community

Looks good.

+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view.yml
@@ -38,7 +38,7 @@ display:
+      pager_options: false

intersting that DefaultCofigSchema.test didn't fail :(

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 2357145.28.patch, failed testing.

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 commented
Issue tags: +Needs reroll
olli’s picture

olli CreditAttribution: olli commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2357145.33.patch8.63 KB
olli’s picture

olli CreditAttribution: olli commented

Re #30 that might be related to #2331793: Changing pager settings for this display only also changes pager settings for other display. The pager_options exists in schema, but not sure it should.

olli’s picture

olli CreditAttribution: olli commented

Sorry, ignore #34. It's not related to DefaultConfigTest. The changed file is only tested by the test added here because the file is in sub-dir /test_views, not /config/install?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/views/config/schema/views.filter.schema.yml
index bfcc681..d9c96e6 100644
--- a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view.yml

--- a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view.yml
+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view.yml

+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view.yml
@@ -35,7 +35,7 @@ display:
-      pager_options: {  }
+      pager_options: false

Are we really sure that its a sane idea to allow false for a map?

olli’s picture

olli CreditAttribution: olli commented

#36: pager_options belongs to pager plugin now, similar to style_options?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

---

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

#36: pager_options belongs to pager plugin now, similar to style_options?

Got it, what you meant, so yes, the pager options are now stored below pager.options, so we can remove that entry completly.
Sorry for that previous answer!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/views/config/schema/views.filter.schema.yml
@@ -121,17 +118,16 @@ views.filter.standard:
diff --git a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view.yml b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view.yml

diff --git a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view.yml b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view.yml
index bfcc681..d9c96e6 100644

index bfcc681..d9c96e6 100644
--- a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view.yml

--- a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view.yml
+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view.yml

+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view.yml
+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_view.yml
@@ -35,7 +35,7 @@ display:

@@ -35,7 +35,7 @@ display:
         options:
           offset: '0'
         type: none
-      pager_options: {  }
+      pager_options: false
       sorts:
         id:
           field: id

You could also do that directly on the commeit.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 33: 2357145.33.patch, failed testing.

olli’s picture

olli CreditAttribution: olli commented
Status: Needs work » Needs review
FileSize
2357145.42.patch8.63 KB

reroll

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

... you could have just just the pager_options bit

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Status: Reviewed & tested by the community » Needs review
FileSize
interdiff.2357145.42.44.txt567 bytes
2357145.44.patch8.6 KB

Is this what you meant?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Yeah absolute!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Yeah absolute!

Dries’s picture

Dries CreditAttribution: Dries commented
Priority: Critical » Major
Status: Reviewed & tested by the community » Fixed

In my mind we wouldn't hold back the Drupal 8 release for this bug. I don't think it is a critical so I'm downgrading it to major, as per our issue priority guidelines for major bugs: "Issues that have significant repercussions but do not render the whole system unusable (or have a known workaround) are marked major". I committed the patch to 8.0.x. Thanks!

  • Dries committed ff4d10b on 8.0.x 
    Issue #2357145 by alexpott, olli, YesCT: Views can not be saved with a...

Status: Fixed » Closed (fixed)

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