`user_filters()` shows module and permission machine names rather than their human-readable labels, and tries to translate them

[avpaderno]

This issue is the equivalent of user_filters() shows module and permission machine names rather than their human-readable labels, and tries to translate them for Drupal 7.

user_filters() contains the following code.

  $options = array();
  foreach (module_implements('permission') as $module) {
    $function = $module . '_permission';
    if ($permissions = $function()) {
      asort($permissions);
      foreach ($permissions as $permission => $description) {
        $options[t('@module module', array('@module' => $module))][$permission] = t($permission);
      }
    }
  }

$permission contains the permission machine name; it cannot be translated. The code should use the permission title, which is already translated when returned from the hook_permission() implementations.
Furthermore, instead of the module machine name, that code should use the module name, similarly to what already shown in the form listing all the enabled modules.

[avpaderno]

I need to understand why _testImageStyleUrlAndPath() fails.

[avpaderno]

I do not see any relation between ImageStylesPathAndUrlUnitTest::_testImageStyleUrlAndPath() (the failing test) and database_test_theme_tablesort() (the function that indirectly calls user_filters()).

[avpaderno]

All checks have passed. The previously failing test has been fixed in another issue.

[klonos]

Thanks @kiamlaluno 🙏🏼 ...the PR code looks good, and it matches the code in the respective issue in Drupal. Just trying to understand where this function is being used, in order to be able to review this properly. Also, why we are adding the translated strings as keys in the $options array 🤔 ...and what does this mean?:

/**
 * List user administration filters that can be applied.
 */

...what user administration filters and for what? ...and where might they be applied?

[avpaderno]

@klonos To answer to where user_filters() is used in Backdrop, it is only called from database_test_theme_tablesort() (indirectly, via user_build_filter_query()). In Drupal 7, it is used in admin/people, but in Backdrop admin/people is a view.

It seems that both user_filters() and user_build_filter_query() can be removed. database_test_theme_tablesort() (which tests theme_tablesort() using data read from the database) could be rewritten to use different data (and avoid to call user_build_filter_query()).

I opened this issue thinking that user_filters() was used to populate the options available in some form, for which the bug was to ask translators to translate something they could/should not translate (a machine name). It is now clear that user_filters(), although it passes to t() a machine name, it is not asking to translators to translate something they are not supposed to translate, because that function is only used in tests.

[avpaderno]

As a side note, views_handler_field_user_permissions, the class that shows a permission field in views, already does the Right Thing™️: It uses the permission title instead of the permission machine name.

foreach ($all_roles[BACKDROP_AUTHENTICATED_ROLE]->permissions as $permission) {
  $authenticated_permissions[$permission]['permissions'] = $permissions[$permission]['title'];
}

foreach ($user_role->permissions as $permission) {
  if (isset($permissions[$permission])) {
    $this->items[$user->uid][$permission]['permissions'] = $permissions[$permission]['title'];
  }
}

What I reported here is only valid for user_filters(), which can be marked as deprecated as per #6534.

[avpaderno]

I apologize: This issue has been replaced by #6534.

[avpaderno]

I apologize again: Since the functions are marked deprecated, their code still needs to be changed. user_filters() can still be called from contributed modules, so what this issue reports should be fixed.

I am going to expand the issue since there are two changes that needs to be done to $options[t('@module module', array('@module' => $module))][$permission] = t($permission);.

Since the line is one, it is not possible to open two different issues.

[klonos]

@kiamlaluno I don't think that we need 2 separate issues/PRs for this. Here's a PR that addresses all issues, and adds some comments and @todos in a few places: https://github.com/backdrop/backdrop/pull/4810

As I also mentioned in #6534, we can't add @deprecated at this point, since there is no 2.x branch available (the line to be added should be @deprecated since 2.0.)

[avpaderno]

the line to be added should be @deprecated since 2.0.

No, it should not. A function is first deprecated and then removed. When it is deprecated, any call done by Backdrop core is removed, and the function code changed to call watchdog_deprecated_function(). Then, the function is removed from the 2.x branch.

[klonos]

Since the UI that allowed testing this is not available in Backdrop any longer, you can install that devel module, and then use the following code in the "Execute PHP code" that the module provides:

$filters = user_filters();
dpm($filters);

In the output, check the array under -> permission -> options:

• in vanilla Backdrop, the module names and permission names are using machine names
• with the PR, both module and permissions are using the human-readable names

@kiamlaluno, since you are more active than me in the Drupal core issue queue, can you please provide a patch/PR there if you can spare some time? Show our Drupal brethren some ❤️

[klonos]

No, it should not. ...

Right, so should I add @deprecated since 1.0 then? (plus the watchdog call)

[avpaderno]

since you are more active than me in the Drupal core issue queue

Truly, the MR on the Drupal 7 issue was created by me. 😉 I just need to update it.

[klonos]

Thanks @kiamlaluno 🙏🏼 ...in the meantime, I've removed all the deprecation-related changes from the PR here, so ready for review again.

[quicksketch]

Thanks @avpaderno and @klonos! I merged https://github.com/backdrop/backdrop/pull/4810 into 1.x and 1.28.x.