Filter out non-selected roles for the remember_roles setting in views

Good idea to store much less! surely it needs update hook to clean-up a lot of core views at least

+1, I have been annoyed by this since D7 and Features exports.

That works for me

Hi,

I have tested this issue manually. It worked for me.

This would be a lovely thing to land. We need a test and an update function in order to land this!

This line has more than 80 chats. Please could you break this array in a second line? E.g.:

From:
$form_state->setValue(['options', 'expose', 'remember_roles'], array_filter($roles));

To:

$form_state->setValue([
  'options',
  'expose',
  'remember_roles'
], array_filter($roles));

Rerolled the patch #4.

That's great. Thank you so much

Still needs a test and update hook as per #12.

Adding a test and update hook for the issue , the update hook finds all view yml files and set the updated configurations. couldn't find anything more effective to update the files , even Yml::dump() dumper has some indentation issues so , I didn't try that.

Hope this works.

Kindly review.

@vakulrai great job! needs a bit of work

+++ b/core/modules/views/views.install
@@ -18,3 +20,29 @@ function views_install() {
+function mymodule_update_9001() {

it should be post update hook and requires test

There is a helper class in Views called ViewsConfigUpdater than can be used to modify Views config entities in update hooks, instead of parsing/rewriting the YAML directly.

@andypost, @longwave I worked on the inputs given by you and have made the changes accordingly to the patch. The patch has following updates,

- .install code has been moved to hook_post_update_name() with some modifications.
- in post_update I have used the ViewsConfigUpdater class to modify the view
- added a test for post_update.

Kindly review.

I missed the tests for hook_post_update in previous patch, re uploading the patch with interdiff. Consider this for review.

Thanks.

Fixed Custom Commands Failed.

Re-rolled the patch, Attached interdiff for same.

It could use to extend \Drupal\views\ViewsConfigUpdater::processOperatorDefaultsHandler() via \Drupal\views\ViewsConfigUpdater::updateAll() but it looks more straight to not extend it

Couple of little nits but this looks almost ready to go.

1. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -665,6 +665,12 @@ public static function trustedCallbacks() {
   +    $roles = $form_state->getValue(['options', 'expose', 'remember_roles']);
   +    $form_state->setValue([
   +      'options',
   +      'expose',
   +      'remember_roles',
   +    ], array_filter($roles));
   

   I think the setValue call should all be on one line if the getValue call is.

2. +++ b/core/modules/views/tests/src/Functional/Update/UserRememberRolesFilterSettingTest.php
   @@ -0,0 +1,45 @@
   + * Tests the role expose filter re4member role settings.
   

   re4member -> remember

3. +++ b/core/modules/views/views.post_update.php
   @@ -68,3 +68,29 @@ function views_post_update_rename_default_display_setting() {
   +            foreach (array_keys($filter_value['expose']['remember_roles'], '0', TRUE) as $role_key) {
   

   Nice! Today I learned you can search with array_keys() alone.

Updated patch in #26 addressing #27, also made a following small change(attached an interdiff):

+++ b/core/modules/views/tests/src/Functional/Wizard/BasicTest.php
@@ -230,8 +230,7 @@
    * Tests user role exposed filter options.
    */
   public function testUserRolesFilter() {
-    // Create a view for user entity and add a role filter.
-    // settings.
+    // Create a view for user entity and add a role filter settings.

Thanks!

Fixed the #28 custom commands.

Interdiff between #28 and #29

On this part

+ foreach ($display_settings as &$display) {
+      if (!empty($display['display_options']['filters'])) {
+        foreach ($display['display_options']['filters'] as &$filter_value) {
+          if (!empty($filter_value['expose']['remember_roles'])) {
+            foreach (array_keys($filter_value['expose']['remember_roles'], '0', TRUE) as $role_key) {
+              unset($filter_value['expose']['remember_roles'][$role_key]);
+              $save = TRUE;
+            }
+          }
+        }
+      }
+    }

We should apply some early return to avoid many level of if and foreach

New patch applied early return on this method

Please discard #32

New patch #33 applied early return on this method

Thanks, no issues remaining, this all looks good now.

I think the update name and displayable title needs review

Random failure

@berdir , I have updated the point #38.1, and for #38.2 I have already tried updating the yml files using the yml parser and dumping the updated data for all the files inside /core, but the dumper has some yml alignment issues, so we went for the hook_post_update approach to update only the view content entities.

Thanks

Here's a fix for all core's view, also updated upgrade path test to use copy of affected views.view.who_s_online.yml

Looks ready to go, probably backportable to next 9.2.x

Sorry for interdiff bigger then patch

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -665,14 +665,18 @@ public static function trustedCallbacks() {
-    $limit_operators = $form_state->getValue(['options', 'expose', 'operator_limit_selection']);
-    $operators_selected = $form_state->getValue(['options', 'expose', 'operator_list']);
+    $limit_operators = $form_state->getValue([

reverted it as out of scope

Removed test view as "files" comes from dump also fixed test

1. +++ b/core/modules/views/tests/src/Functional/Wizard/BasicTest.php
   @@ -226,4 +226,51 @@ public function testWizardDefaultValues() {
   +    $leading_slash_view = [];
   

   Confusing variable name in this context, copy/paste left over, lets find a better name

2. +++ b/core/modules/views/tests/src/Functional/Wizard/BasicTest.php
   @@ -226,4 +226,51 @@ public function testWizardDefaultValues() {
   +    $this->assertEquals($leading_slash_view['page[path]'], $this->cssSelect('#views-page-1-path')[0]->getText());
   

   No need to check that here we have the test that this was copied from for this

3. +++ b/core/modules/views/tests/src/Functional/Wizard/BasicTest.php
   @@ -226,4 +226,51 @@ public function testWizardDefaultValues() {
   +    $expose_settings = [
   +      'options[expose][remember]' => 1,
   +      'options[expose][remember_roles][anonymous]' => 'anonymous',
   +      'options[expose][remember_roles][authenticated]' => 'authenticated',
   +    ];
   

   We don't explicitly add a third role in this test, so if these are the only two available roles in this set up (no idea if that is the case or not...), this would be the same as selecting all the roles. I would add a new role and make sure at the end that this isn't added to the config.

+++ b/core/modules/views/tests/src/Functional/Update/UserRememberRolesFilterSettingTest.php
--- a/core/modules/views/tests/src/Functional/Wizard/BasicTest.php
+++ b/core/modules/views/tests/src/Functional/Wizard/BasicTest.php

Also, not sure this is the best place to add this test, not really related to the Wizard at all

Hi, thanks for the work so far on this.

We have multiple sites built on the same systems but with some custom user roles. Comparing changes to views between sites has always thrown up the differences in these sections even though none have actually made any changes at all.

The latest patch didn't apply for me (D10.3.2), so I have created just a patch for the \Drupal\views\Plugin\views\filter\FilterPluginBase::validateExposeForm method from the latest D11 code.

This works stripping all of the redundant roles from the config.

Sorry - ignore this one - wrong patch file!

Sorry, correct patch file for 10.3+/11.x

I move the patch into a MR and fixes the rejected changes + typehints.

The post update uses now core/modules/views/src/ViewsConfigUpdater.php and moves the test into \Drupal\Tests\views\Functional\Plugin\FilterTest, because there are also other test for \Drupal\views\Plugin\views\filter\FilterPluginBase.

Will probably need upgrade path tests.

Looks good!

The update test is in core/modules/views/tests/src/Functional/Update/UserRememberRolesFilterSettingTest.php.

Thank you, looks ready

Ran the 'test-only' test. It fails as it should. Pipeline otherwise all green.

Corrected one typo then remembered the etiquette so added two threads: code comment nits.

One comment on the post update, overall this looks good though.

Rebased && add missing call to setDeprecationsEnabled.

Looks great now, really nice to see this one ready.

Committed/pushed to 11.x, thanks!

Hello,

I have just upgraded a site to 11.2.2.

When running the DB update script I get this:

 >  [notice] Update started: views_post_update_update_remember_role_empty
>  [error]  The "taxonomy_term_name_into_id" plugin does not exist. Valid plugin IDs for Drupal\views\Plugin\ViewsPluginManager are: taxonomy_term_name, user_name, entity:block, entity:block_content_type, entity:block_content, entity:captcha_point, entity:comment_type, entity:comment, entity:editor, entity:entity_subqueue, entity:entity_queue, entity:field_config, entity:field_storage_config, entity:file, entity:filter_format, entity:image_style, entity:imce_profile, entity:linkit_profile, entity:media_type, entity:media, entity:menu_link_content, entity:menu_position_rule, entity:metatag_defaults, entity:node, entity:node_type, entity:paragraphs_library_item, entity:path_alias, entity:redirect, entity:responsive_image_style, entity:search_page, entity:shortcut, entity:shortcut_set, entity:slick, entity:smart_date_format, entity:smart_date_rule, entity:smart_date_override, entity:menu, entity:action, entity:taxonomy_vocabulary, entity:taxonomy_term, entity:user, entity:user_role, entity:webform_options, entity:webform_submission, entity:webform, entity:pathauto_pattern, entity:xmlsitemap, entity:view, entity:paragraph, entity:paragraphs_type, entity:entity_view_display, entity:entity_form_mode, entity:entity_view_mode, entity:entity_form_display, entity:base_field_override, entity:date_format, none, numeric
>  [error]  Update failed: views_post_update_update_remember_role_empty
 [error]  Update aborted by: views_post_update_update_remember_role_empty
 [error]  Finished performing updates.

Pardon my ignorance, but is this related at all to this issue?

Just added...
I had a misbehaving unused view that caused this error. I deleted the view and I was able to complete the update. Interestingly enough, I could not disable or duplicate the view in the views overview page. When I tried to edit the view I got a WSOD and this error:
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "taxonomy_term_name_into_id" plugin does not exist. Valid plugin IDs for Drupal\views\Plugin\ViewsPluginManager are: taxonomy_term_name, user_name, entity:block, entity:block_content_type, entity:block_content, entity:captcha_point, entity:comment_type, entity:comment, entity:editor, entity:entity_subqueue, entity:entity_queue, entity:field_config, entity:field_storage_config, entity:file, entity:filter_format, entity:image_style, entity:imce_profile, entity:linkit_profile, entity:media_type, entity:media, entity:menu_link_content, entity:menu_position_rule, entity:metatag_defaults, entity:node, entity:node_type, entity:paragraphs_library_item, entity:path_alias, entity:redirect, entity:responsive_image_style, entity:search_page, entity:shortcut, entity:shortcut_set, entity:slick, entity:smart_date_format, entity:smart_date_rule, entity:smart_date_override, entity:menu, entity:action, entity:taxonomy_vocabulary, entity:taxonomy_term, entity:user, entity:user_role, entity:webform_options, entity:webform_submission, entity:webform, entity:pathauto_pattern, entity:xmlsitemap, entity:view, entity:paragraph, entity:paragraphs_type, entity:entity_view_display, entity:entity_form_mode, entity:entity_view_mode, entity:entity_form_display, entity:base_field_override, entity:date_format, none, numeric in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 53 of /home/customer/www/ferniefix.com/public_html/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

@dadderley You have something in your view which is broken.

The plugin ID comes from https://www.drupal.org/project/views_taxonomy_term_name_into_id

@tobiasb
Thanks, I found the error in the view.

Doing updates today and found this in my logs. Searching for views_post_update_update_remember_role_empty leads me to this issue.

Listed as an update to execute
views update_remember_role_empty post-update Clean-up empty remember_roles display settings for views filters.

>  [notice] Update started: views_post_update_update_remember_role_empty
>  [warning] Undefined array key "type" ComponentValidator.php:90
>  [warning] Undefined array key "type" ComponentValidator.php:90
>  [warning] Undefined array key "type" ComponentValidator.php:90
>  [warning] Undefined array key "type" ComponentValidator.php:90
>  [warning] Undefined array key "type" ComponentValidator.php:90
>  [warning] Undefined array key "type" ComponentValidator.php:90
>  [warning] Undefined array key "type" ComponentValidator.php:90
>  [warning] Undefined array key "type" ComponentValidator.php:90
>  [warning] Undefined array key "type" ComponentValidator.php:90
>  [warning] Undefined array key "type" ComponentValidator.php:90
>  [warning] Undefined array key "type" ComponentValidator.php:90
>  [warning] Undefined array key "type" ComponentValidator.php:90
>  [notice] Update completed: views_post_update_update_remember_role_empty
 [success] Finished performing updates.

I cannot speak to the issues that cause or come from this; I am only reporting the message.