administer filters permission should not affect text format widget

Do we really have to use filter_permission_name(2); here? Looks really, really odd and makes our core test reliance on default.profile worse. Otherwise looks fine though.

Fair enough. We're not going to change the default proile dependency for tests in D7 either.

Hm. I guess I'm one of those core developers, because I don't quite understand this patch from the description. In every other case "administer X" means "exempt from permission checking." This patch seems to break that mold?

Hmmm. Yeah, I guess I can see that argument. I think it will cause its fair share of WTFs, but then again we have several hundred thousand documented cases of people screwing up text format configuration, and getting your site compromised is a much bigger WTF. :P~

I gotta run to supper, but I'll commit this later.

Yeah, I think this is a good idea. In addition to security, there is also a convenience aspect - just because I have "administer filters" doesn't necessarily mean I want every single text format on the site showing up in my dropdown with no way to remove them.

Comments on the patch:

1. Don't we need an upgrade path here? It seems like users with "administer filters" in D6 should get all the text format permissions explicitly assigned to them during the upgrade to D7.

2. +    // Create and login administrative user having access to Full HTML text
   +    // format in standard install profile (id 2).
   +    $admin_user = $this->drupalCreateUser(array(
   +      'administer blocks',
   +      filter_permission_name(2),
   

   I believe this test (and the others like it) will fail if run on a machine that has a database auto-increment not set to 1. Rather than hardcoding the integer, can't we do something like what the PHP module tests do in this situation? They do this:

       $php_format_id = db_query_range('SELECT format FROM {filter_format} WHERE name = :name', 0, 1, array(':name' => 'PHP code'))->fetchField();
   

   Yes, it's ugly also :) But it works and is more readable.

3. -    'title' => t('Administer and use any text formats and filters'),
   -    'description' => drupal_placeholder(array('text' => t('Warning: This permission may have security implications depending on how the text format is configured.'))),
   +    'title' => t('Administer text formats and filters'),
   

   Why remove the security warning? This is still a very, very dangerous permission.

   OK, so actually, the security warning there is totally wrong to begin with - it should say the standard "Warning: Give to trusted roles only; this permission has security implications." (See my comment at #620446-16: Rewrite permission titles and descriptions.) We could probably just fix it as part of this patch - but either way, I don't see why it should be removed.

4.   * @param $format
   - *   An object representing a text format.
   + *   An object representing a text format or an internal text format id.
   

   What is the reason for this change? In general I thought we try not to do this kind of thing - we either do object or ID, but not both.

   I don't see why it's needed here except that the tests in the patch do filter_permission_name(2); however, they could just as easily do filter_permission_name(filter_format_load(2)) which is what is already done in other tests.

5. To ease the usability concerns here, maybe we can make it so that on the form for adding a new text format, if your site has an administrator role, the checkbox for that role is pre-checked? This would help preserve the most likely workflow ("a site administrator creates a new text format because they want to use it").

I'll have some time to test this tomorrow sun. Subscribing.

Looks good. Committed to CVS HEAD.

1. +    'description' => drupal_placeholder(array('text' => t('Warning: This permission has security implications.'))),
   

   The wording here is inconsistent with the other permission warnings in core.

2. +    $fullhtml_format = db_query_range('SELECT * FROM {filter_format} WHERE name = :name', 0, 1, array(':name' => 'Full HTML'))->fetchObject();
   

   Here and elsewhere, the variable name shouldn't run words together; i.e., it should be $full_html_format.

3. 5) I don't support auto-granting permissions. For one, we have a shiny new admin role feature in D7 already. Second, when creating a new text format, the first visible option is a list of user role checkboxes - more than sufficient.

   Hm? What I am suggesting is that we actually USE the shiny new admin role feature here, rather than letting it be ignored :) And auto-checking a checkbox is not the same as auto-granting a permission...

   To clarify, the admin role only gets new permissions automatically when a module is enabled, not when new dynamic permissions appear. That is a technical limitation, but from a user experience perspective it is not good - the average site administrator is not going to want to remember to have to check boxes for their admin role in some cases but not others. (Content type permissions theoretically have this issue too, when adding a new content type, but it's mitigated via the 'bypass node access' permission; here, though, we have removed the equivalent of the 'bypass text format access' permission, which is why we need to do something else or the experience of adding a new text format won't make sense.)

   We need to make sure that administering a site using the admin role simple - but the patch committed here just made it harder. Auto-checking the checkbox fixes that for the 90% use case (and the other 10% can easily and conveniently uncheck the box).

I've addressed the above three issues in the attached patch.

Committed to CVS HEAD. Thanks.