Rollback: Use 'Filter' for exposed filter button text in new views and new installations (instead of 'Apply')

This won't change installed sites. Can we live with that?

This won't change installed sites. Can we live with that?

I think we should not touch existing sites, maybe besides the core views like /admin/content?

I would prefer a more generic solution than renaming individual the label for the button in individual views, and change the label of that button in general.

Whenever a user adds one or several exposed filter to a view, the fields are displayed with their labels and a general button "Apply" which basically stands short for "Apply this filter" - but nobody does it say that the fields above it are filter options.

It gets even more confusing when there are other buttons or fields around it, such as primary actions or bulk operations. (As on the Custom Block Library page, bur for example also at the admin page for the Media module.)

So, can we just _by default_ label the button that is placed with exposed filter "Filter"? That would solve the issue both for views provided by core, as well as for any other view build by a user or provided by a module.

ViewsExposedForm.php also has:

    $form['actions']['submit'] = array(
      // Prevent from showing up in \Drupal::request()->query.
      '#name' => '',
      '#type' => 'submit',
      '#value' => $this->t('Apply'),
      '#id' => Html::getUniqueId('edit-submit-' . $view->storage->id()),
    );

I think the rationale behind 'Apply' is that changing the filters may in fact result in a bigger result set and therefore not "filter" the results you are seeing but expand them (if more restrictive filters were applied earlier). So 'Apply' makes more sense in a general way. However it causes problems when there is also operations, which are also 'Applied' as in:

So sounds like using 'Filter' generally is better given the typical combination with bulk operations being confusing otherwise.

I agree it would be nice to apply (see what I did there :) this change to the default settings for exposed filters.

Following a short discussion on IRC:

There can be indeed cases where the "exposed filter" actually expands the results, or where - technically speaking - the user does not filter the results any further, but replaces one filter with another (for example switching from only published to only unpublished content).
In general language use, even this "un-filtering" or switching filters would work with "filter" as a label, and it would more clearly distinguish the function of this button from the bulk operations button (Apply/Apply to selected items).

In the rarer cases where "Filter" is not be appropriate, it's still possible to change the label.

I am all in favour of changing the default value of this to be "Filter" instead of "Apply". For other existing views out there though I would argue, that we should just change it for the examples in the admin UI for ourself, so /admin/content /admin/people ...

So:

1. Change the default value in \Drupal\views\Plugin\views\exposed_form\ExposedFormPluginBase::defineOptions
2. (optionally) Change the value in \Drupal\views\Form\ViewsExposedForm::buildForm
3. Write an update path for the core admin views

Hello everyone, I've had a go at doing a patch for this.

Thanks @michaellenahan - nice work.

+++ b/core/modules/views/views.install
@@ -408,3 +408,32 @@ function views_update_8200() {
+        if ($display['display_options']['exposed_form']['options']['submit_button'] != 'Filter') {

Should we instead be checking === 'Apply'?

Should we be using t() here (I have no idea)?

This just needs corresponding changes in the Drupal\views\Tests\Plugin\ExposedFormTest test class.

1. +++ b/core/modules/views/views.install
   @@ -408,3 +408,32 @@ function views_update_8200() {
   +function views_update_8300() {
   

   Note: This should be a post_update function, see views.post_update.php, see https://www.drupal.org/node/2563673

2. +++ b/core/modules/views/views.install
   @@ -408,3 +408,32 @@ function views_update_8200() {
   +        if ($display['display_options']['exposed_form']['options']['submit_button'] != 'Filter') {
   +          $display['display_options']['exposed_form']['options']['submit_button'] = 'Filter';
   

   I agree with lendude, we should just change the value, when the default wasn't changed, actually, its a tough decision, due to sites with translations. This is a user facing string, maybe it would be better to not change it at all, but rather let just new sites get it.

Thanks for your reviews, @larowlan and @dawehner.

I forgot about the test class, I've made the changes there now.

This is a user facing string, maybe it would be better to not change it at all, but rather let just new sites get it.

OK, I have left out the update function now.

Although, can I ask what the implications of changing "Apply" to "Filter" on a translated site would actually be? On a French site the buttons will say "Filtrer" so that should be fine.

Ahh. I wonder if this is what the problem is: in cases where someone has already *manually* translated the string "Apply" on their own site, they will get "Filter" (or their language version of "Filter"), and their manual string translation work will be blown away.

Are we not changing existing views anymore? Are all of them using the new wording so we only need to update the PHP generating new views?

@Gábor Hojtsy
My thought process is written in #14. Any opinion about it?

@dawehner: if it is in views yml files too then new sites will not get it either since the patch does not change those. New sites will only get it for new views (much as old sites will get it for new views too). Is this what you are referring to?

@dawehner: if it is in views yml files too then new sites will not get it either since the patch does not change those. New sites will only get it for new views (much as old sites will get it for new views too). Is this what you are referring to?

I agree with lendude, we should just change the value, when the default wasn't changed, actually, its a tough decision, due to sites with translations. This is a user facing string, maybe it would be better to not change it at all, but rather let just new sites get it.

Right, but let's be clear, these strings, on sites with translations for example, could suddenly appear in english.

@dawehner:

Right, but let's be clear, these strings, on sites with translations for example, could suddenly appear in english.

Ok maybe I was not clear. I meant the prior version of the patch which also changed shipped default views. Changing those should not change existing site views. It would change their translatability a bit (given #2428045: Comparing active storage to install storage is problematic, install storage may change anytime) but we cannot do much about that.

Ok maybe I was not clear. I meant the prior version of the patch which also changed shipped default views. Changing those should not change existing site views. It would change their translatability a bit (given #2428045: Comparing active storage to install storage is problematic, install storage may change anytime) but we cannot do much about that.

Right, so you argue to change the views located in node/optional<code> like <code>views.view.content.yml for example?

@daweher: yes, if we don't change those default views then new sites will not get the improvement for their installed views, only for their admin-created views.

@daweher: yes, if we don't change those default views then new sites will not get the improvement for their installed views, only for their admin-created views.

And well, I don't have much a problem with that. If we want, we could write an update path for just those views in the admin area. Keep in mind that there might be a lot of views on the actual site out there, which better don't be touched, IMHO.

Hi, I want to be sure that I'm on the right track here.

We need a patch which changes all the views.view.*.yml files in core, changing exposed_form.options.submit_button from Apply to Filter.

Is that correct? This patch does that (77 changes, including the test yml files.)

In addition, the patch changes the wording from "Apply" to "Filter" in:

core/modules/views/src/Form/ViewsExposedForm.php
core/modules/views/src/Plugin/views/exposed_form/ExposedFormPluginBase.php
core/modules/views/src/Tests/Plugin/ExposedFormTest.php

To make it easier to review, I have attached these small changes as exposed_filter_button-2561115-26.patch-non-yml-file-changes.txt

What I haven't done in this patch is any update path. I'm not sure how to only include those files which are in the admin area. What we have so far is this:

EDIT - changed to post_update as per #14

function views_post_update_exposed_form_submit_button_label_filter() {
  $config_factory = \Drupal::configFactory();
  foreach ($config_factory->listAll('views.view.') as $view_config_name) {
    $view = $config_factory->getEditable($view_config_name);

    foreach ($view->get('display') as $display_id => $display) {
      if (!empty($display['display_options']['exposed_form']['options']['submit_button'])) {
        if ($display['display_options']['exposed_form']['options']['submit_button'] === 'Apply') {
          $display['display_options']['exposed_form']['options']['submit_button'] = 'Filter';
          $view->set("display.$display_id", $display);
          $view->save(TRUE);
        }
      }
    }
  }
}

Don't know why I am getting a test failure. Locally it works fine. I've attached output from my local pc.

Worked out why the test is failing. Facepalm. There was a further test method in the class where I forgot to change "Apply" to "Filter".

This patch is #33 plus the upgrade path (see interdiff for the upgrade path alone).

For the upgrade path, I have targeted core views which have a path beginning with 'admin/'.

On the current codebase, the only view that was changed in this upgrade was 'views.view.block_content'.

1. +++ b/core/modules/views/views.post_update.php
   @@ -205,3 +205,45 @@ function views_post_update_serializer_dependencies() {
   +        if (!empty($display['display_options']['path']) && substr($display['display_options']['path'], 0, 6) === 'admin/') {
   

   That seems to be an okay condition to deal with. Note: We could strpos($path, 'admin/') == 0, IMHO easier to read.

2. +++ b/core/modules/views/views.post_update.php
   @@ -205,3 +205,45 @@ function views_post_update_serializer_dependencies() {
   +    foreach ($view->get('display') as $display_id => $display) {
   +      if (!empty($display['display_options']['exposed_form']['options']['submit_button'])) {
   +        if ($display['display_options']['exposed_form']['options']['submit_button'] === 'Apply') {
   +          $display['display_options']['exposed_form']['options']['submit_button'] = 'Filter';
   +          $view->set("display.$display_id", $display);
   +          $view->save(TRUE);
   +        }
   

   We could move this view->save() one level up, so we just save views once, when there are multiple displays.

This patch works as intended: For new custom views, the button for exposed filters is labeled "Filter", and the existing button on the Custom block library page is also renamed to "Filter".

Dawehner, should this issue be set to "Needs work", or can it be RTBCed?

Yeah adapting the second point would be great!

Ok so lets add the feedback from #37. Added some comments to the update function.

Thank you @lendude

I think "Filter" is okay as a new default, but I'm not sure if we should include the upgrade path. As a general principle, sites own their configuration. The site owner could specifically want the text to be "Apply".

I pinged @alexpott for a second opinion.

The #41 patch is more ambitious than what is described in the issue summary. Can we get an update please?

I think "Filter" is okay as a new default, but I'm not sure if we should include the upgrade path. As a general principle, sites own their configuration. The site owner could specifically want the text to be "Apply".

I pinged @alexpott for a second opinion.

Personally I'd like to move this discussion into a follow up and decide that there and provide an potential update path there. Personally I don't care much about those existing sites. IMHO the site owners would have fixed it already, if they cared.

@dawehner, @xjm. Yep the upgrade debate is secondary. I don't think we should be changing existing sites here - let's file a followup for that.

I am Trying to review patch #41 but i am not able to display any changes after apply patch.

@amit.drupal
Did you executed update.php? Did you reinstalled Drupal form scratch?

@dawehner

Step For Review Patch

1 - Installed Drupal form scratch
2 - Take screenshot pages [/admin/structure/block/block-content, /admin/content, /admin/content/files, /admin/people ) .
3 - Apply Patch #41
4 - Run update.php and clean cache
5 - Again test all listed pages.
But there is no change .

Here is the patch without the update path.

@dawehner has opened the follow up #2831017: Potential update path for changing exposed filter button text to 'Filter'

I updated the issue summary as requested by @cilefen to indicate we are updating more then just one label on one View.

Hi,

I applied the patch. It worked cleanly. Since its without update path, I cleared the cache, but can't see the text change. Please let me know if I am missing any step.

@lomasr without the update path, this will only take effect for new Views. Existing Views will need to be updated manually.

@Lendude, Thanks I will test with new Views.

Hi ,
I created a new Views , but still I can't see the text change.

Thanks

I updated the issue summary to make this clear ...

@lomasr Apply patch, clear cache, create new View. Works for me.....

Hi, Thanks it worked for me too. Previously I created new page in an existing View. Now I have created a new View . Sorry about the confusion.

Thanks

Go with this!

unrelated fail

diff --git a/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_filter_node_access.yml b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_filter_node_access.yml
index 6663fa8..546d041 100644
--- a/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_filter_node_access.yml
+++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_filter_node_access.yml
@@ -37,7 +37,7 @@ display:
       exposed_form:
         type: basic
         options:
-          submit_button: Apply
+          submit_button: Filter
           reset_button: false
           reset_button_label: Reset
           exposed_sorts_label: 'Sort by'
diff --git a/core/modules/rest/tests/modules/rest_test_views/test_views/views.view.test_excluded_field_token_display.yml b/core/modules/rest/tests/modules/rest_test_views/test_views/views.view.test_excluded_field_token_display.yml
index e95f953..bbed087 100644
--- a/core/modules/rest/tests/modules/rest_test_views/test_views/views.view.test_excluded_field_token_display.yml
+++ b/core/modules/rest/tests/modules/rest_test_views/test_views/views.view.test_excluded_field_token_display.yml
@@ -40,7 +40,7 @@ display:
       exposed_form:
         type: basic
         options:
-          submit_button: Apply
+          submit_button: Filter
           reset_button: false
           reset_button_label: Reset
           exposed_sorts_label: 'Sort by'
diff --git a/core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_argument_taxonomy_vocabulary.yml b/core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_argument_taxonomy_vocabulary.yml
index a90e618..b206af5 100644
--- a/core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_argument_taxonomy_vocabulary.yml
+++ b/core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_argument_taxonomy_vocabulary.yml
@@ -37,7 +37,7 @@ display:
       exposed_form:
         type: basic
         options:
-          submit_button: Apply
+          submit_button: Filter
           reset_button: false
           reset_button_label: Reset
           exposed_sorts_label: 'Sort by'
diff --git a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_glossary.yml b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_glossary.yml
index e2e143e..b687bb9 100644
--- a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_glossary.yml
+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_glossary.yml
@@ -38,7 +38,7 @@ display:
       exposed_form:
         type: basic
         options:
-          submit_button: Apply
+          submit_button: Filter
           reset_button: false
           reset_button_label: Reset
           exposed_sorts_label: 'Sort by'
diff --git a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_taxonomy_glossary.yml b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_taxonomy_glossary.yml
index 8e4182d..3b396cc 100644
--- a/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_taxonomy_glossary.yml
+++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_taxonomy_glossary.yml
@@ -35,7 +35,7 @@ display:
       exposed_form:
         type: basic
         options:
-          submit_button: Apply
+          submit_button: Filter
           reset_button: false
           reset_button_label: Reset
           exposed_sorts_label: 'Sort by'

There are some missing (maybe new) test views. Uploading a patch just to ensure fixing them does not break tests.

Committed ce7cf17 and pushed to 8.3.x. Thanks!

We overlooked that the button is not only used for exposed filters, but also for exposed sort criteria. They are both in the same form, so it looks like we've created a new UI problem.

That's why the original "Apply" made sense as the default option: it works for both.

We should roll back this change then. The label mismatch this patch introduced for exposed filters outweighs the win of more consistent labelling for the "filter" use case.

Let's get committer attention on this.

This will need a patch because I'm pretty sure patches have gone in since using the word "Filter" in a test to click on a button. I'm sure because I broke HEAD because of this.

Here's a patch that does the revert - let's see of something fails.

Yep I knew we would have a fail :) yay for breaking HEAD before.

Putting this in the RTBC queue, but need to wait for tests to run.

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

I'm going to mark this as 'fixed', because we fixed the regression. We could try to change 'apply' to something else, but that should probably happen in a new issue linking back to this one.

Thanks a lot.