Make it possible to configure exposed filter operators

Moving to the right queue, and +1 on this.

I worked on this patch long time ago for views #873872: Allow to limit which exposed operators apply

This is an starting patch. Maybe someone want to continue during this weekend sprint.

Here is the patch with tests. I had to modify the test_filter_in_operator_ui to include a filter with more operators to play with.

1. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -552,6 +561,35 @@ public function buildExposeForm(&$form, FormStateInterface $form_state) {
   +          '#description' => $this->t('Limit the operators available to choose to the selected on the list.'),
   

   This is not grammatically correct. It could be better if it is as follows:
   Limit the operators available to choose from to be shown on the list.

2. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -552,6 +561,35 @@ public function buildExposeForm(&$form, FormStateInterface $form_state) {
   +          '#description' => $this->t('Select which operators will be available to be selected by the end user. Leave emtpy to allow select between all of them'),
   

   The second sentence will sound much better if it is as follows:
   Leave empty to show and choose from the complete list.

3. +++ b/core/modules/views/src/Tests/Plugin/FilterTest.php
   @@ -163,4 +163,32 @@ public function testInOperatorSelectAllOptions() {
   +   * Tests the limit exposed operator functionality.
   

   Grammatically incorrect. It should be 'Tests the limit of the expose operator fonctionality.

Thanks @abi_cima.

This patch includes the corrections proposed in #9 and the modifications to hide the min and max fields when there are not operators selected that use them.

This works really nicely.

Patch looks great.

Any way to position the 'limit checkbox' and select list underneath the 'expose operator' checkbox?
Currently it aligns next to it and that makes it unclear to me that these things are tightly related.

Should we add an upgrade path for this? I don't think it's strictly necessary since we provide defaults, but it does make the change cleaner.

Any way to position the 'limit checkbox' and select list underneath the 'expose operator' checkbox?

I've been playing with the views-ui-expose-filter-form.html.twig template. It seems it could be possible to organize the settings. However at this moment there is a mix of logic between the templates, views_ui module and the showOperatorForm, buildOptionsForm and other methods from FilterPluginBase.

Therefore the solution if much more complex that the code this patch provides.

I created #2860990: Move logic to configure exposed form into a template. as a tentative follow-up.

The followup sounds like a good idea.

Manually ran through this again and currently the limiting is also done in the 'Add' dialog, not just on the exposed filter.

In the dialog I would always expect to see all operators. And maybe we need a check that when limiting the operators, the default operator is still available?
And tests for this.

In the dialog I would always expect to see all operators.

Done.

And maybe we need a check that when limiting the operators, the default operator is still available?

Done.

And tests for this.

Done.

Also I'm hiding the operators list if the user decide to not expose the operator, that was failing before.

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -305,6 +307,24 @@ protected function operatorForm(&$form, FormStateInterface $form_state) {
+   *   The exposed form bein built.

The word 'being' is misspelled as 'bein'.

Fixed and attached new patch.

@dagmar, this looks really nice. Bit of nitpicking (sorry still not happy with the descriptions :):

1. +++ b/core/modules/views/src/Tests/Plugin/FilterTest.php
   @@ -199,6 +199,13 @@ public function testLimitExposedOperators() {
   +    // Select by default a not valid operator.
   

   Set the default to an excluded operator.

2. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -552,6 +572,36 @@ public function buildExposeForm(&$form, FormStateInterface $form_state) {
   +          '#description' => $this->t('Limit the operators available to choose from to be shown on the list.'),
   

   Limit the available operators to be shown on the exposed filter.

3. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -552,6 +572,36 @@ public function buildExposeForm(&$form, FormStateInterface $form_state) {
   +          '#description' => $this->t('Leave empty to show and choose from the complete list.'),
   

   Only the selected operators will be shown on the exposed filter.

Thanks @Lendude.

I really like this. Thanks @dagmar

Unrelated fail apparently

1. We have new config keys / values - this means we're going to need an upgrade path
2. Also we should have a change record.

3. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -305,6 +307,24 @@ protected function operatorForm(&$form, FormStateInterface $form_state) {
      /**
   +   * Limits the available exposed operators.
   +   *
   +   * @param array $form
   +   *   The exposed form being built.
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   +   */
   +  protected function limitOperatorOptions(&$form, FormStateInterface $form_state) {
   +    if (empty($this->options['expose']['operator_limit_selection']) ||
   +        empty($this->options['expose']['operator_list'])) {
   +      return;
   +    }
   +
   +    $options = $this->operatorOptions();
   +    $operator_list = $this->options['expose']['operator_list'];
   +    $form['operator']['#options'] = array_intersect_key($options, $operator_list);
   +  }
   
   @@ -843,6 +904,7 @@ public function buildExposedForm(&$form, FormStateInterface $form_state) {
   +      $this->limitOperatorOptions($form, $form_state);
   

   I think given we're adding this functionality to a base class I'm not sure we should risk a method. If we inline the function then there is no risk of method name clash.

4. +++ b/core/modules/views/src/Plugin/views/filter/NumericFilter.php
   @@ -191,7 +191,22 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
   +        if (in_array($operator, $this->operatorValues(2))) {
   

   Let's make this a strict check ie in_array(,,TRUE)

Change record: https://www.drupal.org/node/2869168

Added upgrade path and the suggestions by @alexpott.

Here is the interdiff between #19 and #25 to make easier the review.

Looks good and addresses all feedback from @alexpott

one nitpick:

+++ b/core/modules/views/src/Tests/Update/LimitOperatorsDefaultsTest.php
@@ -0,0 +1,42 @@
+   * Tests that boolean filter values are updated properly.

copy/paste leftover

Thanks @Lendude

All feedback has been addressed, CR has been created, I think this adds a really great feature.

Unrelated fail, https://www.drupal.org/pift-ci-job/649976.

Opened #2870146: Even more random fails in \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest

Same unrelated fail.

needs a reroll for #2863267: Convert web tests of views

Before post a re-roll. @Lendude, do we need to put the update in a post_update hook? Can we use hook_update_N instead? We are not using the entity api there, it should be safe... What do you think?

@dagmar I would always prefer a post_update when possible, no need to reroll when a different hook_update_N gets committed.

Thanks @Lendude. Here is the new patch with the tests updated.

Anything missing here @Lendude?

@dagmar thanks for the poke, dropped of my radar.

Looking at this again, we need to take care of existing module config. So follow #2870724: Define and document best practices for configuration entity updates/bc layers. Sorry, should have thought of that earlier. But since the update here is pretty straight forward, it shouldn't be to hard in this case I hope.

So, this is blocked by the other issue? Postponing this on the other issue.

No it just need an upgrade path. I forgot this ticket sorry. Let me see if I can finish the remaining work in the next days/week.

Added support to #2870724: Define and document best practices for configuration entity updates/bc layers.

Re-saved all the views to include the default configs in them.

There is one more thing I would like to test. Date provides a custom implementation of the NumericFilter plugin.

1 method overrides NumericFilter::valueForm()

Date::valueForm in core/modules/views/src/Plugin/views/filter/Date.php
    Add a type selector to the value form

@mpdonadio @jhedstrom where do you think we could add this test?

Rerolled after #2949351: Add a helper class to make updating configuration simple

Added test coverage for datetime filter.

#2865344: Exposed date filters 'empty' and 'not empty' are broken includes some test coverage for exposed filters too.

Anything else missing for this? @Lendude

Fixed the failing tests.

Re-rolled for 8.8.x. Re-saved all demo_umami views.

Installed media_library and re-saved the view.

Looks like (at least) the upgrade path was omitted from the reroll on #68

Since we are needing an update path, I suppose we'll need an update path test too

@Manuel Garcia thanks! Indeed some files where out the original patch due some gitignore rules.

Excellent we do have an upgrade path test already :D

Changes since #73 look good to me, and we have a green patch again, yay!

Read through the patch, only found a few nits/suggestions:

1. +++ b/core/modules/views/src/Plugin/views/filter/NumericFilter.php
   @@ -258,7 +258,22 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
   -    if ($which == 'all' || $which == 'minmax') {
   +    // If operators are limited, only show minmax fields if there is an
   +    // available exposed operator in the list.
   +    $two_value_operators_available = ($which == 'all' || $which == 'minmax');
   

   A bit of a nit, and perhaps it's because I'm new to the patch, but to me reading this comment doesn't make it clear clear why we need to do this, can we explain also the why in the comment? It'll help a lot when/if we need to modify this code in the future :)

2. +++ b/core/modules/views/tests/src/Functional/Update/LimitOperatorsDefaultsTest.php
   @@ -0,0 +1,48 @@
   +  public function testViewsPostUpdateLimitOperatorsDefaultValues() {
   +    $this->runUpdates();
   

   For sanity's sake, let's assert that before the updates run, the view does not have the configuration we're adding in the upgrade path.

3. +++ b/core/modules/datetime/tests/modules/datetime_test/test_views/views.view.test_exposed_filter_datetime.yml
   --- /dev/null
   +++ b/core/modules/datetime/tests/src/Functional/DateFilterTest.php
   
   +++ b/core/modules/datetime/tests/src/Functional/DateFilterTest.php
   @@ -0,0 +1,107 @@
   +  public static $modules = ['datetime_test', 'node', 'datetime', 'field', 'views_ui'];
   

   These should be in separate lines.

4. +++ b/core/modules/datetime/tests/src/Functional/DateFilterTest.php
   @@ -0,0 +1,107 @@
   +    $this->adminUser = $this->drupalCreateUser(['administer views']);
   

   We're not defining the adminUser property but we're assigning it.

5. +++ b/core/modules/datetime/tests/src/Functional/DateFilterTest.php
   @@ -0,0 +1,107 @@
   +
   

   Nit: Extra line

6. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -623,6 +655,15 @@ public function buildExposeForm(&$form, FormStateInterface $form_state) {
   +        $this->t('You selected an operator as the default value that is not included on the list of limited operators.'));
   

   Nit: $this->t('You selected the %operator operator as the default value but is not included on the list of limited operators.', ['%operator' => $selected_operator]);

Thanks for the review @Manuel Garcia!

Great work on this, thank you! Went through this again and this looks great.

We have test coverage for:
- the normal functionality
- the min max changes
- the validation

We have an upgrade path for existing sites and new views getting installed and a test for that.

Thanks @dagmar, latest changes look great. RTBC++

The failure is due #3046697: \Drupal\Tests\views\Kernel\Entity\RowEntityRenderersTest::testRevisionBaseTable() can randomly fail once that issue is fixed this can be back to RBTC.

That got in yesterday.

1. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -559,6 +561,36 @@ public function buildExposeForm(&$form, FormStateInterface $form_state) {
   +          '#description' => $this->t('Only the selected operators will be shown on the exposed filter.'),
   

   Should we say 'selecting none will make all of them available' which is a common pattern?

2. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -623,6 +655,15 @@ public function buildExposeForm(&$form, FormStateInterface $form_state) {
   +    if ($limit_operators && !in_array($selected_operator, $operators_selected)) {
   

   These are strings here, so should we use the third argument?

Comments from @larowlan makes sense to me. Here is the new patch.

Back to RBTC. Random test failure #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest

Back to RTBC.

adding issue credits for reviews

Fixed on commit

diff --git a/core/modules/datetime/tests/modules/datetime_test/test_views/views.view.test_exposed_filter_datetime.yml b/core/modules/datetime/tests/modules/datetime_test/test_views/views.view.test_exposed_filter_datetime.yml
index cf4af75e41..9a9140b616 100644
--- a/core/modules/datetime/tests/modules/datetime_test/test_views/views.view.test_exposed_filter_datetime.yml
+++ b/core/modules/datetime/tests/modules/datetime_test/test_views/views.view.test_exposed_filter_datetime.yml
@@ -126,4 +126,3 @@ display:
         - url.query_args
         - 'user.node_grants:view'
       tags: {  }
-
diff --git a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_filter_in_operator_ui.yml b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_filter_in_operator_ui.yml
index eaaa828359..cc73429982 100644
--- a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_filter_in_operator_ui.yml
+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_filter_in_operator_ui.yml
@@ -153,4 +153,3 @@ display:
         - url.query_args
         - 'user.node_grants:view'
       tags: {  }
-

Published change record

Committed 09fdae1 and pushed to 8.8.x. Thanks!

If anyone wants to backport this to D7 I'd be happy to consider it for the next Views release *hint* *hint* :-)

If anyone wants to backport this to D7 I'd be happy to consider it for the next Views release *hint* *hint* :-)

Done as well :) #873872: Allow to limit which exposed operators apply

Duh, yeah, thanks for the reminder.

This issue is tagged for the 8.8.0 release notes, but does not have a release note.

Additionally, the change record here seems to describe a feature addition, not a disruptive change. If there are disruptive impacts from this issue, the release note and change record should be updated to describe those disruptive impacts.

Otherwise, the issue should be tagged for the 8.8.0 highlights instead. Thanks!

I would say this is a highlight and not disruptive (if it is disruptive, we messed up that upgrade path ;)

If this needs a snippet too, let me know and I'll make something up.

3 years ago this needed a release note, and it was changed to a highlight, so 👋 bye bye irrelevant tag.