Default format overrides a previously set format

I change for my projects in filter_default.module:

...
if ($form[$el]['#type'] == 'radio') {
...

to

...
if ($form[$el]['#type'] == 'radio' && $form[$el]['#default_value'] == FILTER_FORMAT_DEFAULT) {
...

it seems that work (at least in my project).

to

...
if ($form[$el]['#type'] == 'radio' && $form[$el]['#default_value'] == variable_get('filter_default_format', 1)) {
...

and i has dummy filter as default (in filter.module settings) that redefined for all roles.

I have noticed this bug too. I found it very frustrating when editing nodes which have a non-default filter, and then submitting without checking the filter because I had set it previously! Thanks for the patch, it works great.

Thanks for the patch. It will be in filter_default-1.1.

This is not an adequate fix for this bug. Consider the following scenario.

- The "global" default input format is set to "Filtered HTML"
- User X's role's default input format is set using "filter default" module to "Full HTML".
- User X creates a page and explicitly sets "Filtered HTML"
- User X then edits the same page and the "Full HTML" format is automatically selected for him, but he does not check it before saving the page.
- The page was saved with Full HTML, but User X didn't know.

Sorry about not being clear about that. I believe that the logic here violates the principle of least surprise. In the scenario I described, the global default input format matches the intended input format for the page. In this case, the logic is unable to discern that the intended input format has already been decided (by the User) for the page.

Also that was a slightly contrived scenario. A better example may be: a malicious user submits a php script in a page along with some objectionable material. A moderator with PHP format permission and defaulted by role goes and edits that page removing the objectionable material, but misses the PHP script.

Perhaps the best way to address this is to only alter the input format radio when the user is first creating a page, but not to alter when the user is editing a page.

Larry, you are absolutely correct in your analysis of the situation. I had the same realization a few days ago. There is no way to tell the difference between "the user selected format X which happens to be his default" and "the user has not selected a format."

I also hit on the idea of "only alter the input format radio when the user is first creating a page, but do not alter when the user is editing a page." That works great for nodes because I can check $form to see if this is a new or existing node, but input formats can be used on non-node forms and there is no way in general to tell if "this form is for a new entity".

This is what I meant in comment #6 on http://drupal.org/node/109019 in which I said this bug may not be fixable. Perhaps this module is just the wrong approach. If neither I, you, nor someone else can propose a workable solution, I'll have to withdraw the module.

Here's a heavyweight hack that could make this work. In function filter_default_form_alter(), you can set a persistant variable by obtaining the node id (or comment id) and call it "filter_default_is_applied_$nid". Before executing _filter_default_form_alter_filters check if $is_applied is true. I called this heavyweight because we are storing one variable per node.

Sample pseudo-code:

function filter_default_form_alter($form_id, &$form) {
>  $id = _filter_default_get_id($form); // obtain some id; node id for example. 
>  $is_applied = variable_get("filter_default_applied_$id", FALSE);
>  if ($is_applied) {
>    return;
>  }
  global $user;
  $roles = user_roles();
  for ($i = 1; $i < count($roles)+1; $i++) {
    list($role, $format) = variable_get('filter_default_'.$i, array());
    if (array_key_exists($role, $user->roles)) {
      _filter_default_form_alter_filters($format, $form);
      break;
    }
  }
>  variable_set("filter_default_applied_$id", TRUE);
}

I don't think the behavior here is a bug.

If an administrator with PHP format permission edits some content and doesn't check the content ... it's his fault. He knows (or should know), he has a default filter allowing php-tags, and should therefore be careful what he does.

If you really want to save "designated filter" somewhere, it should be in an extra table. And I don't think $nid is a sufficient key, since a custom content type could have more than 1 text-area with different input formats used. So you'd have to use a tuple as primary key.

And if a user isn't satisfied with the defaults his role gives him, maybe we need (optional) user-specific default filter?

If an administrator with PHP format permission edits some content and doesn't check the content ... it's his fault. He knows (or should know), he has a default filter allowing php-tags, and should therefore be careful what he does.

Perhaps you are right, but not everyone can be expected to function as a machine, perfectly double-checking everything and never forgetting a click. It is at least a usability issue. I don't want to beat this example to death as this is just one possible scenario.

I do concede that the currently committed fix may work for more than 90% of cases where the filter default module will be applied and it is a pretty good hack.

If you really want to save "designated filter" somewhere, it should be in an extra table. And I don't think $nid is a sufficient key, since a custom content type could have more than 1 text-area with different input formats used. So you'd have to use a tuple as primary key.

We are not actually saving the "designated filter", but only a boolean value to determine whether this is the first time that the form is being created for any particular node (or content object that could be filtered). Sorry this wasn't clear. Maybe I should have named it "filter_default_is_done_$id". We want the module to set the default input format for all text-areas in the form the first time the form is used and never again, so it does not matter how many text-areas there are.

And sure, the implementation could be improved.

And if a user isn't satisfied with the defaults his role gives him, maybe we need (optional) user-specific default filter?

This is an interesting additional way to provide defaults for filtering. There's a discussion about by role and by content type defaults at http://drupal.org/node/11218. So far I think by role is the most compelling.

Yes, I've seen the discussion there, but it's about the future, and 6.x is still some time off ... filter_default is what we have and can use now.
I do agree that there are some problems, like deciding which format to choose if a user has multiple roles, but all that doesn't change the fact that filter_default improves things over the current situation.
If I find the time, I'll take part in the general discussion there too, but in the meantime I still think a drupal-5 release of this module would benefit a lot of people.

Especially since the most common use-case will be something like: anonymous and maybe some low-permission roles get "filtered html" as default (and can't choose any other), while the other roles get full html, and admin can use php if he wants/needs to.

Ray, I totally agree with you. I would apply the 90/10 rule and stipulate that this is good enough for most use cases, including my own. This module provides a great feature, and I don't want to see this module to get trashed just because we can't find a perfect solution. And I'm not going to hold my breath and wait for 6.0. If we simply published this bug as a known issue I'd be 100% satisfied with it and I think most users would be too. I think that the module is usable as is and the bug should be addressed in the future. I'd like to set this status to postponed. But it's up to Barry as to whether he would like to continue further development of the module.
I thought of another more typical scenario that would apply to more users, and I know it's been brought up in the aforementioned discussion here http://drupal.org/node/11218: A low permissioned user with Filtered HTML permision creates a page. A heavily permisioned editor edits that page, the default filter module sets the input format to Unfiltered HTML, and the editor accidentally forgets to set the filter back to Filtered HTML. The original author goes to edit his page, but finds he's been blocked from editing his own page. This happens because the less restrictive input format is not available to the original author and prevents him from editing the page. This is perhaps also a core drupal issue, where the editor is never given any warning that he is inadvertently adjusting permissions on this poor user's page.

I already posted some thoughts in the discussion at http://drupal.org/node/11218, let's see what happens ...

And reading your posts ... maybe there's something I don't understand about the system.
I had the impression, that the input-formats (available and default) always depend on the editing user, and have nothing to do with the way the node (or field) was created. Am I wrong here?
If I am ... is input-format downgrading really a problem?

During editing, the textarea is saved verbatim and filters are not applied at this point, but the selected input format is saved along with the node. The filters are applied just before the node is rendered for viewing.

A user is restricted from editing a page when he does not have access to the currently selected input format for the node. So yes, input-format upgrading or downgrading could be a problem.

I was a little hasty in setting the status on this. Feedback anyone?

Checking the value attribute in the nid element of the form, may be a lighter-weight approach than using the database. I tested this for page, story, book, newsletter (simplenews.module), comments, block, and forum. For comments we check the cid and for blocks we only care about block_box_add; since edits are handled by a different form.

This isn't a perfect solution because it does not account for other types of content other than what's in drupal core. Other modules can call filter_form() but this code is not aware of those modules.

>/**
> * Determines whether the form is creating something new.
> *
> */
>function _filter_default_is_new($form_id, $form) {
>  switch ($form_id) {
>    case 'comment_form':
>      return empty($form['cid']['#value']);
>    case 'block_box_add':
>      return TRUE;
>    default:
>      return empty($form['nid']['#value']);
>  }
>}
>  
function filter_default_form_alter($form_id, &$form) {
  global $user;
  $roles = user_roles();
>  if (_filter_default_is_new($form_id, $form)) {
    for ($i = 1; $i < count($roles)+1; $i++) {
      list($role, $format) = variable_get('filter_default_'.$i, array());
      if (array_key_exists($role, $user->roles)) {
        _filter_default_form_alter_filters($format, $form);
        break;
      }
    }
>  }
}

Thanx for enlightening me ;-)

Patch attached with the changes above.

Changing patch status.

Larry, I appreciate your work on this module. I'd like to suggest that you avoid marking issues "ready to be committed" on other people's modules. It is considered rude in this community. When you submit a patch you think is done, mark it "code needs review." I've made this mistake before and annoyed some people, just trying to save you the same fate.

The idea of checking various different entity types for newness is a good one, but the way you've done it is risky. It assumes anything that isn't a comment or block has a 'nid' field that identifies newness, and we know that is wrong (for example, Views, Panels custom content, and many other things don't fit this model). I'd rather see the is_new test explicitly identify everything the test is known to be safe for. This can include all node types by testing for $form['#id'] == 'node-form' instead of just assuming a 'nid' field means it is a node.

If we go that route, the help text displayed to the admin should identify those kinds of objects the default filters will apply to instead of saying it will apply to all of them (which it no longer will).

And even with that, I'm still uncomfortable having default input filter rules that only work some of the time. I think it will confuse admins, make the system less stable, and give this module a bad rep. I really think this is not the right solution even thought it mostly works most of the time.

I apologize for being hasty with the patch status. I hope that this didn't cause any problems for you. I promise I'll set it to needs review next time.

It should be easy to set this up properly for views, but I can't think of any good way to alter the panels form so that this works. The problem is that custom content panels can be added dynamically, and there's no good way of knowing whether the custom content panel is new. In other words, panel page newness does not necessarily imply panel newness

The idea of checking various different entity types for newness is a good one, but the way you've done it is risky.

You're probably right on this one. I can change the function so that it returns one of { IS_NEW, IS_NOT_NEW, UNKNOWN }.

This makes the logic for IS_NEW and IS_NOT_NEW pretty straightforward.

When we are unable to determine the form type, do we set the filter to the default value or do we leave it as it is? I would argue that you want the fallback behavior in the case of "don't know" to be not to apply any default filter logic, but this could be a settings option.

Either way it is done, there's no getting around the inconsistent behavior. A perfect solution may just have to wait until 6.x. The only solution I can think of involves making Input Format itself an entity, but that would take some modification to the filters core module, and all dependent modules as well.

I just re-read all the open issues on this module.

The bottom line is that I think we should make Filter Default works on nodes and comments and nothing else (and document it that way, of course). This way, we can easily detect "is this a new object?" and only set if the default input format if it is, avoiding the problem of overriding the system default input format if it is explicitly set.

Comments?

I absolutely agree. This would suffice in almost any scenario I would use the module.

It sounds perfectly reasonable to me. I haven't looked at this code for a while now, and I hardly remember how any of it works. If anyone would like to sumbit a patch, feel free to do so. I might not be able take a look at this for a while.