Make views exposed filter operator labels configurable

Hi @Manuel Garcia this will be a good option for customization.

I applied the patch and It works flawlessly. Thanks for the patch.

Generally +1 to this feature. It's always hard to justify adding yet more settings to the Views UI. ;) But this does seem like the sort of thing that folks want/need to customize (I certainly have), and forcing them to use hook_form_alter() for it is a burden.

However, the RTBC is premature. At the bare minimum, we need a test of the new feature. We should probably also have a test for the post_update function (thanks for adding that!).

I'll closely review the rest of the patch later (time permitting).

Thanks,
-Derek

1. +++ b/core/modules/views/views.post_update.php
   @@ -436,3 +436,29 @@ function views_post_update_remove_core_key(&$sandbox = NULL) {
   +  \Drupal::classResolver(ConfigEntityUpdater::class)->update($sandbox, 'view', function ($view) {
   +    /** @var \Drupal\views\ViewEntityInterface $view */
   

   Nit: you can use a type declaration on the closure parameter to have the language enforce it.

2. +++ b/core/modules/views/views.post_update.php
   @@ -436,3 +436,29 @@ function views_post_update_remove_core_key(&$sandbox = NULL) {
   +          $view->set("display.$display_name.display_options.filters.$filter_name", $filter);
   

   I don't think you can set nested values in this way on the config entity itself; that's provided by \Drupal\Core\Config\ConfigBase::set(), ie you can use it with config returned from the config factory.

Thanks!

Thanks @Manuel Garcia!

I based this on views_post_update_limit_operator_defaults which does something very similar, so I assume it is the correct way to do it?

Ooh, er yeah, good point! I've done a little digging, and I actually wonder if that update really works. I commented out the following line locally from views_post_update_limit_operator_defaults() and yet \Drupal\Tests\views\Functional\Update\LimitOperatorsDefaultsTest::testViewsPostUpdateLimitOperatorsDefaultValues() still passes, so I wonder if that's just a bad model to be copying?

$view->set("display.$display_name.display_options.filters.$filter_name", $filter);

FWIW I made the attached little script and it seems that test2() and test3() successfully update the view.

function test(string $view_name) {
  $config_entity = View::load($view_name);

  foreach ($config_entity->get('display') as $display_name => &$display) {
    if (!isset($display['display_options']['filters'])) {
      continue;
    }

    foreach ($display['display_options']['filters'] as $filter_name => $filter) {
      if (!isset($filter['expose']['operator_label'])) {
        $filter['expose']['operator_label'] = t('Operator');
        $config_entity->set("display.$display_name.display_options.filters.$filter_name", $filter);
      }
    }
  }
  $config_entity->save();
}

function test2(string $view_name) {
  $config_object = \Drupal::configFactory()->getEditable('views.view.' . $view_name);
  foreach ($config_object->get('display') as $display_name => &$display) {
    if (!isset($display['display_options']['filters'])) {
      continue;
    }

    foreach ($display['display_options']['filters'] as $filter_name => $filter) {
      if (!isset($filter['expose']['operator_label'])) {
        $filter['expose']['operator_label'] = t('Operator');
        $config_object->set("display.$display_name.display_options.filters.$filter_name", $filter);
      }
    }
  }
  $config_object->save();
}

function test3(string $view_name) {
  $config_entity = View::load($view_name);

  $displays = $config_entity->get('display');
  foreach ($displays as $display_name => &$display) {
    if (!isset($display['display_options']['filters'])) {
      continue;
    }

    foreach ($display['display_options']['filters'] as $filter_name => $filter) {
      if (!isset($filter['expose']['operator_label'])) {
        $display['display_options']['filters'][$filter_name]['expose']['operator_label'] = t('Operator');
      }
    }
  }
  $config_entity->set('display', $displays);
  $config_entity->save();
}

test('content');
test2('block_content');
test3('comment');

Thanks

@Manuel, #14 applied cleanly and works has required. Moving to RTBC.

Mostly looks good, thanks! A few nits, and a few concerns of real substance:

1. +++ b/core/modules/views/tests/fixtures/update/exposed-operators-labels.php
   @@ -0,0 +1,19 @@
   +/**
   + * @file
   + * Test fixture.
   + */
   

   One could complain this doesn't really tell us much. ;)

2. +++ b/core/modules/views/tests/src/Functional/Plugin/ExposedFormTest.php
   @@ -357,6 +363,40 @@ public function testExposedSortAndItemsPerPage() {
   +    $this->assertText($default_operator_label);
   ...
   +    $this->assertText($new_operator_label);
   

   Raw assertText() always makes me nervous, since there's a chance we'll get false positives if that text appears anywhere else on the page for any other reason. I always prefer more targeted assertions if possible. E.g. an xpath that finds exactly the label we're expecting...

3. +++ b/core/modules/views/tests/src/Functional/Plugin/ExposedFormTest.php
   @@ -357,6 +363,40 @@ public function testExposedSortAndItemsPerPage() {
   +    // Assert the Operator label field is displayed with the default value.
   +    $this->drupalLogin($this->rootUser);
   +    $this->drupalGet('admin/structure/views/nojs/handler/test_exposed_operator_label/default/filter/type');
   +    $assert_session->fieldValueEquals('options[expose][operator_label]', $default_operator_label);
   +
   +    // Change it to a different value and ensure the form saves correctly.
   +    $this->submitForm([
   +      'options[expose][operator_label]' => $new_operator_label,
   +    ], 'Apply');
   +    $assert_session->statusCodeEquals(200);
   +    // Save the view itself.
   +    $this->submitForm([], 'Save');
   +    // Assert the Operator label field is displayed with the new value.
   +    $this->drupalGet('admin/structure/views/nojs/handler/test_exposed_operator_label/default/filter/type');
   +    $assert_session->fieldValueEquals('options[expose][operator_label]', $new_operator_label);
   

   I guess all this is okay. It's mostly testing that the Views UI works, not that this feature works. ;) Many (most?) views tests directly twiddle the view config. E.g. something like this:

       $view = Views::getView('test_display_feed');
       $display = &$view->storage->getDisplay('feed_2');
       $display['display_options']['row']['options']['link_field'] = 'nid';
       $view->save();
   

   I don't feel super strongly about it, and what's here is more test coverage (which is almost always welcome), but it's also sort of out-of-scope testing, and perhaps duplicate (sort of) with existing tests (#CitationNeeded). /shrug

4. +++ b/core/modules/views/tests/src/Functional/Update/OperatorLabelsDefaultsTest.php
   @@ -0,0 +1,59 @@
   + * Tests the upgrade path for the operator labels feature.
   

   s/upgrade/update/

5. +++ b/core/modules/views/tests/src/Functional/Update/OperatorLabelsDefaultsTest.php
   @@ -0,0 +1,59 @@
   +   * Tests that default settings for limit operators are present.
   

   Copy/paste error, that's not what this test is testing.

6. +++ b/core/modules/views/tests/src/Functional/Update/OperatorLabelsDefaultsTest.php
   @@ -0,0 +1,59 @@
   +  public function testViewsPostUpdateOperatorLabelsDefaultValues() {
   +    // Load and initialize our test view.
   +    $view = View::load('test_exposed_filters');
   

   This test is now depending on the fact that core/modules/views/tests/fixtures/update/views.view.test_exposed_filters.yml does *not* define the new 'operator_label' key. Someone might regenerate that view for other tests (e.g. tests that the view was originally added for, etc). Seems a bit fragile and dangerous to go this route. We should either:

   A) Add some comments to views.view.test_exposed_filters.yml explaining that this test now depends on this fact so that it's less likely someone will accidentally "fix" the view in the future, breaking this test's assumptions.

   B) (Probably safer): Add another default view specifically for this test, something like "views.view.test_exposed_operator_label_update.yml" or something that explains it's a legacy view to test the update path. Then there's no chance someone will "fix" it, since it'll be a dedicated view only used by this test.

7. +++ b/core/modules/views/views.post_update.php
   @@ -436,3 +437,28 @@ function views_post_update_remove_core_key(&$sandbox = NULL) {
   +    foreach ($displays as $display_name => &$display) {
   

   Doesn't look like we ever directly modify $display, so I don't think we want to iterate with references here. Unless PHP is weird and the fact that we're getting references to $filter below requires references here, too. Would be curious if this works with just $display...

Thanks,
-Derek

p.s. Re: #14: Yeah, that seems like a reasonable spot for where to put this test. The layout of Views tests is a bit weird (some make sense, some do not), so it's not easy to make good decisions based on prior art. But +1 to your choice of \Drupal\Tests\views\Functional\Plugin\ExposedFormTest. Since this feature is entirely UI-centric, I think we're going to need Functional tests for it (lots of Views can be tested via Kernel tests, which are preferred if possible, but not for this), and since it's about the exposed form, we should keep it close to the other tests for that functionality.

Thanks! Looking really close. Sorry I didn't notice these before, but a few more minor nits/concerns:

1. +++ b/core/modules/views/tests/fixtures/update/views.view.test_exposed_operator_label_update.yml
   @@ -0,0 +1,271 @@
   +      filters:
   +        status:
   +          value: '1'
   +          table: node_field_data
   +          field: status
   +          plugin_id: boolean
   +          entity_type: node
   +          entity_field: status
   +          id: status
   +          expose:
   +            operator: ''
   +          group: 1
   

   Do we want the test to ensure that the operator label is set on this (non-exposed) filter, too? If not, maybe we don't want it in this view at all? Or maybe we want to change the post_update to ignore non-exposed filters? TBD.

2. +++ b/core/modules/views/tests/fixtures/update/views.view.test_exposed_operator_label_update.yml
   @@ -0,0 +1,271 @@
   +      sorts:
   +        created:
   +          id: created
   +          table: node_field_data
   +          field: created
   +          order: DESC
   +          entity_type: node
   +          entity_field: created
   +          plugin_id: date
   +          relationship: none
   +          group_type: group
   +          admin_label: ''
   +          exposed: false
   +          expose:
   +            label: ''
   +          granularity: second
   

   This is irrelevant to the test, and should probably be removed.

Re: #22
19.1: Sounds good, thanks!
19.2: 👍
Also.2: Seems like the right thing, yeah. I need to read #2989745: views_update_8500() inlines configuration changes instead of this being done on config save for bc
Also.2: /shrug. Generally test views don't need complete config, only the config for the stuff they care about. The point of this fixture is a known pre-update starting point. But whatever, I don't care much either way. This is fine as-is.

Re: #25: Oh right, good point about updating all the default views that core ships. ;)

A few final concerns before I can sign off on this. I'd fix these myself, but then I couldn't RTBC, so I have to set NW:

1. +++ b/core/modules/views/tests/fixtures/update/exposed-operators-labels.php
   @@ -0,0 +1,21 @@
   diff --git a/core/profiles/demo_umami/config/install/views.view.featured_articles.yml b/core/modules/views/tests/fixtures/update/views.view.test_exposed_operator_label_update.yml
   
   diff --git a/core/profiles/demo_umami/config/install/views.view.featured_articles.yml b/core/modules/views/tests/fixtures/update/views.view.test_exposed_operator_label_update.yml
   similarity index 69%
   
   similarity index 69%
   copy from core/profiles/demo_umami/config/install/views.view.featured_articles.yml
   
   copy from core/profiles/demo_umami/config/install/views.view.featured_articles.yml
   copy to core/modules/views/tests/fixtures/update/views.view.test_exposed_operator_label_update.yml
   

   Something about the git diff settings here are confusing git into thinking your new view is a copy of the demu_umami view, which is making this patch hard to review. What if you do git diff -C95% or something? Then it shouldn't consider this a similar file to be copied and will list the new thing as a whole new file (which should be a lot easier to review/read).

2. +++ b/core/modules/views/views.post_update.php
   @@ -10,6 +10,7 @@
   +use Drupal\views\ViewEntityInterface;
   

   Now unused. https://www.drupal.org/pift-ci-job/1641713 shows:

   /var/lib/drupalci/workspace/jenkins-drupal_patches-41128/source/core/modules/views/views.post_update.php ✗ 1 more
   line 13	Unused use statement
   

3. +++ b/core/modules/views/views.post_update.php
   @@ -396,3 +397,14 @@ function views_post_update_field_names_for_multivalue_fields(&$sandbox = NULL) {
   + * Define default operator label values on all exposed filters.
   

   Not just 'all exposed filters' anymore...

Thanks/sorry!
-Derek

Sweet, thanks! I can't find anything else to complain about. 😉 Let's see what the core committers think. 🤞

Nice. Big patch but the actual change is quite small.

+++ b/core/modules/views/src/ViewsConfigUpdater.php
@@ -21,6 +22,8 @@
@@ -249,6 +252,10 @@ protected function processOperatorDefaultsHandler(array &$handler, $handler_type
         $handler['expose']['operator_list'] = [];
         $changed = TRUE;
       }
+      if (!isset($handler['expose']['operator_label'])) {
+        $handler['expose']['operator_label'] = $this->t('Operator');
+        $changed = TRUE;
+      }
     }
 

Interesting. Weaving this into the config update for a different issue will make this helper class VERY hard to clean up let alone ever remove. The hope would be that we can clean/remove this once all the associated update hooks have been removed, but weaving updates into each other would make this very hard to track.

I think we would need to give this its own method that is tightly coupled to the update hook, but this is new ground, so not sure how others feel.

@Lendude re: #29: 👍Now that I've read commit 663762f38d from #2989745: views_update_8500() inlines configuration changes instead of this being done on config save for bc, a separate protected method for this is probably more true to the design intentions of ViewsConfigUpdater, and will indeed make it easier to untangle later. Thanks for raising that.

@Manuel Garcia yeah that looks great.

This will make it easier to remove the right things in D10. Since all these updates are probably going away in D10, I don't think it is a big deal right now, but when we start adding more updates to this in D9 (some of which might be removed in D11) we want do have the right pattern for this set so we can just remove methods and not have to refactor the whole class to find code that is still relevant.

We might want to think about adding some @see comments to that class to make it clearer which method is coupled to which update hook, but that is way out of scope here :)

Yup, #31 definitely addresses #29/#30. Back to RTBC.

Thanks!
-Derek

Eek, whoops, was looking at the interdiff, not the patch. You forgot the git diff -C95% so the weird diff returned. Would you be willing to re-roll for that?

Sorry/thanks!
-Derek

Rerolled the patch and added interdiff

Thanks for the re-roll, @Neslee Canil Pinto. I haven't fully verified the re-roll, but a few concerns with the interdiff you posted:

1. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -113,6 +113,9 @@
   @@ -215,7 +218,7 @@
   
   @@ -215,7 +218,7 @@
      }
    
      /**
   -   * Add additional settings to the entity link field.
   +   * Add additional settings to filters operators.
       *
       * @param \Drupal\views\ViewEntityInterface $view
       *   The View to update.
   

   Not sure what this has to do with this issue. Seems like an out-of-scope (but legit) documentation fix for an existing comment that's wrong?

2. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -252,6 +270,26 @@
   diff -u b/core/modules/views/tests/fixtures/update/views.view.test_exposed_operator_label_update.yml b/core/modules/views/tests/fixtures/update/views.view.test_exposed_operator_label_update.yml
   
   diff -u b/core/modules/views/tests/fixtures/update/views.view.test_exposed_operator_label_update.yml b/core/modules/views/tests/fixtures/update/views.view.test_exposed_operator_label_update.yml
   --- b/core/modules/views/tests/fixtures/update/views.view.test_exposed_operator_label_update.yml
   
   --- b/core/modules/views/tests/fixtures/update/views.view.test_exposed_operator_label_update.yml
   +++ b/core/modules/views/tests/fixtures/update/views.view.test_exposed_operator_label_update.yml
   
   +++ b/core/modules/views/tests/fixtures/update/views.view.test_exposed_operator_label_update.yml
   +++ b/core/modules/views/tests/fixtures/update/views.view.test_exposed_operator_label_update.yml
   @@ -59,8 +59,8 @@
   
   @@ -59,8 +59,8 @@
                offset: false
                offset_label: Offset
              tags:
   -            previous: ‹‹
   -            next: ››
   +            previous: ‹‹
   +            next: ››
          style:
            type: default
          row:
   

   This shouldn't be here, either, I don't think...

Re-reviewed #37. LGTM. I dunno about #36.1. I tend to prefer Just Fix It Already(tm), but core committers tend to get really set on scope management. I guess we'll see what happens. 😉🤞Hopefully we can leave it as-is, but maybe we'll have to split that out to a trivial follow-up. /shrug

Thanks!
-Derek

Nice work on this feature! I think it could use a usability review.

The latest patch also doesn't apply to 9.1.x, so it needs a reroll.

https://www.drupal.org/pift-ci-job/1672597 is indeed a legit failure. Looks like perhaps the test is relying on an update fixture that's now gone in D9?

Re: UX review, added this to the agenda for #3131774: Drupal Usability Meeting 2020-05-05

Interdiff looks great. Bot is now happy. RTBC once the UX review is satisfied.

Thanks!
-Derek

We discussed this issue at the #3131774: Drupal Usability Meeting 2020-05-05.

Making the operator name configurable seems like a good idea. I guess the usability issue is whether it is clear what the new "Operator label" text field affects.

Before giving a usability review, we would like to see some further updates to the issue summary, so I am setting the status to NW for that.

• In the Proposed Resolution, give more detail about how the feature will work.
• Under User interface changes, add a screenshot showing how this affects the filter when the view is displayed.

Besides adding the highlighted text field, the patch seems to move the "Expose operator" checkbox from the first column to the second. That puts it closer to the additional controls that are unhidden when it is selected, which is a good thing. But it should be called out in the issue summary, either as I have just described it or with a "before" screenshot.

The screenshot shows "Operator for content type" in the new textfield, and the list of options has the label "Your momma". Are these two supposed to match? If so, please describe and/or illustrate that in the issue summary. Please also come up with a more useful example. If the two are supposed to match, have you considered keeping the default label: that is, "Operator (Your momma)" instead of just "Your momma"?

Looking at this screenshot reminds us how crowded the Views UI can get. Personally, I wonder if it would improve things if the "Expose operator" checkbox and the additional controls that it enables were put inside a fieldset. Both of these are out of scope for the current issue, but worth keeping in mind.

If you can update the summary, then I will do my best to review it promptly.

Sweet, thanks @Manuel Garcia. Summary looks great. Removing that tag.

Also adding #2625136: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements as related, since both of these issues are dealing with the UI of the views exposed filter form. Even the 'After' screenshots here are still kinda whack, which is why we desperately need to fix #2625136, too. ;)

@benjifisher Asked for screenshots where both this and #2625136: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements are applied. Glad they asked! ;) I forgot that #2625136 is doing this anytime there's an exposed operator:

$form[$operator]['#title_display'] = 'invisible';

;) It visually hides the label for the operator (although leaves it for assistive tech), because the whole filter (label, operator, value(s)) is now wrapped in a fieldset (see below). Therefore, that bug fix potentially renders this feature request obsolete. :/ Whoops!

I added 2 exposed time filters to the /admin/content view on a local test site, both with exposed operators. Here's just #3120627:

Better than just "Operator" for both (raw core), but still confusing and weird.

Here's what you see once you apply #2625136-129:

Now that there's a labeled fieldset for the whole filter, we probably don't need a custom label for the operator at all.

Some possible paths forward:

A) Close this as "won't fix" in favor of #2625136. :(

B) Postpone this on #2625136, and once that's landed, modify this feature so that it can peacefully co-exist:

B.1) Add a 'Display operator label' checkbox (defaults to false to keep the 'invisible' behavior above) but that you can enable if you still want a custom operator label in there for something.

B.2) Change this feature so the default value is an empty string (for 'invisible') but if folks fill in a value, the code that's setting 'invisible' from #2625136 doesn't fire. Would probably want to change the label for the setting, and add a description.

C) Other?

Thanks/sorry,
-Derek

This is really a screenshot for #2625136, but here's the same view once you select an operator that requires 2 values (e.g. 'Is between'):

Per @benjifisher in Slack, formally postponing this on #2625136: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements. Once that lands, we can decide what to do with this feature.

Thanks/sorry,
-Derek

#2625136 was committed, so reopening this.

i refer some screenshot after use of exposed filter and changment in operator lable on drupal 10.1.x.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I don't think its postponed by anything, but needs a reroll.

Amending attribution.