Add description to "administer filters" permission

Tagging 'novice' for review and manual testing.

The patch applies, follows the coding standards and implements the t() function as it's supposed to

The link is a good idea. Lets try to write this in a way that doesn't almost literally repeat the label. Something like this maybe:

Define how text is handled by combining filters into [link]text formats[/link]

Tested and confirming it works

Nice, thanks all.

Committed and pushed to 8.x. Thanks!

Applies and does what it says. RTBC.

On second thought, let's make it read the same as D8 ...

Note to committer: Commit credit goes to Amontero (& possibly Yoroy for the wording suggestions).

I don't mean to nitpick, but if we're going to add a new string to Drupal 7 I only want to do it once and not change it later, so here's the nitpick...

1. Is this description accurate enough? The permission lets you do more than just combine filters into text formats; you can also grant roles access to use them. Would adding something like "and granting permission to use them" at the end be a good idea, or too much?
2. The description does not check access before linking to admin/config/content/formats. This is pretty minor, but it's inconsistent given that all the other permissions defined in filter_permission() do that.

I also find this issue ironic given the history in #620446: Rewrite permission titles and descriptions - there used to be a description here before, but it was unceremoniously removed before Drupal 7 was released. One step forward, one step back, one step forward again, or something like that :)

In any case, this certainly makes sense to me, and it should be fine to add to Drupal 7. Per http://drupal.org/node/1527558 we should ideally do it early in a release cycle, but we're still probably far enough from the next Drupal 7 point release that it's OK to do now if we do it soon.

The following test steps were performed:

1. access the permission page via admin/people/permissions#module-filter
2. Viewed the message to be updated.
Message before patch
3. Applied the patch
4. Viewed the message after the update.
Message after patch
5. Message is now the same as in D8
6. Patch worked as expected

I still want to have a discussion about #17 before committing this to Drupal 7.... (Because I don't want to introduce a new string and then have to change it again later, breaking translations in the process.)

I'm aiming to get a core bug fix release out during the next release window (November 7), in which case at this point it's probably too late to commit new translatable strings intended for that release... However, we can definitely aim to get it committed right after that (so it goes into the following release).

And I guess if we are going to discuss possible changes to this, it should be against Drupal 8 for starters.

Are you actually sure that you

+++ b/core/modules/filter/filter.moduleundefined
@@ -369,9 +369,10 @@ function filter_admin_format_title($format) {
 function filter_permission() {
+  $perm_text = user_access('administer filters') ? '<a href="@url">text formats</a>' : 'text formats';

It feels wrong to check access in an info hook,
as this maybe could cause issues in the future.

Yeah sure, it does totally work because user_access never calls hook_permission but I just said it feels wrong,
as this for example would block hook_permission to be cached.

Other places in which there are links in permissions:

• node_permission: no access check
• system_permission: no access check
• user_permission: no access check

Totally agreed with #25. Please reroll without the access check.

As for the first question in #17, I don't think we need to add the part about assigning text formats to roles. The usability gain is in facilitating that discovery by providing a link to the relevant configuration page, we don't have to explain that up front.

I tried applying the #22 patch and then removing the access check. But then it's functionally the same as current Drupal 8.x-dev, is it not?

The rerolled version would be:

  $perm_text = user_access('administer filters') ? '<a href="@url">text formats</a>' : 'text formats';
  $perms['administer filters'] = array(
    'title' => t('Administer text formats and filters'),
    'description' => t('Define how text is handled by combining filters into ' . $perm_text . '.', array(
      '@url' => url('admin/config/content/formats'),
    )),
    'restrict access' => TRUE,
  );

While the module's current code is:

  $perms['administer filters'] = array(
    'title' => t('Administer text formats and filters'),
    'description' => t('Define how text is handled by combining filters into <a href="@url">text formats</a>.', array(
      '@url' => url('admin/config/content/formats'),
    )),
    'restrict access' => TRUE,
  );

It's definitely possible that I am misunderstanding what is desired to be done here.

If we're not changing the text then I think it's OK to put this back to Drupal 7. But the discussion of doing the permission checks should be split to a different issue (it's now inconsistent in multiple ways in Drupal 8).

For Drupal 7 we should definitely do the user_access() check to be consistent with the rest of the function, especially since we don't have the luxury of continually breaking these translations whenever we change our mind :)

-    'description' => t('Define how text is handled by combining filters into <a href="@url">text formats</a>.', array(
+    'description' => t('Define how text is handled by combining filters into ' . $perm_text . '.', array(

This is wrong though - we can't concatenate strings with variables like that or it won't be properly translatable. We should probably just have two different strings (one with the link and one without).

Ok. I created #2283717 to address the inconsistency in using user_access in hook_permission functions.

Moving this issue then to the backport to Drupal 7. There were already some patches for drupal 7 in the queue, maybe they still apply.

12: drupal--1684930-12-add-description-to-administer-text-formats-permission.D7.patch queued for re-testing.

I'm not sure why the issue is stuck at Needs Work when #12 passed its last retest.

It's a minor change which is nearly identical to what went into D8. It looks good to me. The new text is appearing on the permissions page. It's RTBC from me.

I'm re-uploading #12. I am not the author and this should not be attributed to me. The issue won't stay RTBC because #22 gets re-tested when the status is set.

35: drupal--1684930-35-add-description-to-administer-text-formats-permission.D7.patch queued for re-testing.

Unrelated test failure. Still RTBC.

And still a worthy little text tweak. RTBC indeed.

...More random test failures.

This was already RTBC

This is still inconsistent with the user_access() check used elsewhere in the same function - see discussion above.

In other words, something like this?

With slightly better code style this time...

But didn't we just removed those checks in Drupal 8?

Should we removed them also from Drupal 7? Open another issue for that? Or just stick with what's already there?

Backporting #2283717: Remove user_access function calls on hook_permission functions to Drupal 7 is definitely a possibility (I will change the status of that issue).

That issue came to a resolution a fair amount faster than I thought it would, so doing one thing here and another thing there does seem a little backwards/conflicting now... however, there's still a good chance there will be a Drupal 7 point release in between the two, so I think we should keep the Filter module permissions self-consistent in the meantime (hence my patch). If the other issue winds up getting in also, it will have to undo a bit of what we do here, but not that big of a deal; better that we always have the code in a state that behaves consistently whenever we release it.

I just committed #2283717: Remove user_access function calls on hook_permission functions, so I guess that means this one goes back to... "Needs work"?

Please review.

Hi rpayanm

As discussed in #2283717: Remove user_access function calls on hook_permission functions, we shouldn't use user_access on hook_permission, so, to be consistent, you should remove that check from the patch and just link to the admin page.

Updated as per #54

Seems good =).

Committed to 7.x - thanks!