The roles column of the filter_formats table is set to a comma-separated list of role ids that are allowed to use that format except that for format id 1 (Filtered HTML) it is set to the comma-separated list of all role names that are allowed to use it (i.e. all of them).

The difference occurs on this line of filter_admin_format_form_submit():

$roles = ','. implode(',', ($form_values['default_format'] ? user_roles() : $roles)) .',';

$roles is an array of rids, but user_roles() returns an array of rid => name and implode takes the name.

I'm not sure if this is a mistake or intentional. On the mistake side is this line from system.install

db_query("INSERT INTO {filter_formats} (name, roles, cache) VALUES ('Filtered HTML',',1,2,',1)");

which initializes it to a list of role ids, not names. On the "maybe intentional but probably still a mistake" side is the fact that it doesn't matter what this field contains because filter_access() ignores it anyway:

  if (user_access('administer filters') || ($format == variable_get('filter_default_format', 1))) {
    return TRUE;
  }

IMO, the column should contain the same type of data for all rows.

CommentFileSizeAuthor
#2 criticals_shall_be_destroyed.patch765 bytesChrisKennedy

Comments

Steven’s picture

Steven commented
Priority: Normal » Critical

The DB should only contain role id's. If names are being inserted, something is broken.

ChrisKennedy’s picture

ChrisKennedy commented
Status: Active » Reviewed & tested by the community
StatusFileSize
new criticals_shall_be_destroyed.patch765 bytes

Confirmed that there is a problem with filter_admin_format_form_submit(); attached patch fixes it by using array_keys(). The way this bug survived for so long is pretty interesting - I'll post some links in a bit.

The other two things you mention should be separate bugs. I don't think the system.install line you're referencing is an error anyway.

ChrisKennedy’s picture

ChrisKennedy commented

In http://drupal.org/node/27364 (September 17th, 2005) the UI of the input formats interface was reworked. This version created the initial, correct logic.

In http://drupal.org/node/29465 (October 7th, 2005 - less than a month later) Drupal was converted to Forms API over the course of 148 issue comments, a separate SVN repository for the patch developers, and a 413k patch file. That was one big modification to drupal.

It included the change:

-  $roles = ',';
-  foreach ($edit['roles'] as $rid => $value) {
-    if ($value || $edit['default_format']) {
-      $roles .= $rid .',';
-    }
-  }
+  $roles = ','. implode(',', $edit['default_format'] ? user_roles() : array_keys($edit['roles'])) .',';

It got so close to being correct, and even used array_keys() on the non-default option! But it resulted in this error existing for the past 14 months.

@bjaspan: your filter_access() example isn't an error either: it's by design. Right below your quoted excerpt is a call to filter_formats() which does use the filter_formats.roles column.

Steven’s picture

Steven commented
Status: Reviewed & tested by the community » Fixed

Tested and verified. Good job, thanks! Committed to HEAD.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)