Allow per-field-instance configuration of allowed formats

Awesome!

I wonder whether it wouldn't make more sense to negate the configuration though...?

I.e., if you add 10 text fields, and (with this patch) "allow" the current formats, and you then create a new text format, then that new format won't be available in those 10 text fields until you've specifically updated all of them.

So conversely, if you add 10 text fields, and disallow certain formats for certain fields, and you create a new text format afterwards, then that new format will be available to all. Kinda the behavior you'd expect (without better_formats) after adding a new text format.

It's possible that I'm off here; just wanted to raise the idea.

Perhaps, but perhaps that's over-engineering already, the solution might be to simply make it another field instance setting whether the intention is to "only allow these" or "disallow these", or "nothing/default".

Subscribing

@bayousoft in #drupal expressed interest to test this patch, but also take on this challenge and implement the compromise of #2 and #3 of having a "wrapping checkbox" that controls whether better_formats plugs in for a specific field instance or not. Yay! :)

sub

Subscribe.

The patch tests clean for me so far.

Observation:

IMO An opportunity for improvement with the module in general is for the Default Text Format select to update dynamically without having to save the field. I have found a case where the current functionality gets a little cumbersome.

When a field is saved with (for example Full HTML) only one Allowed Format, the Default select disappears after saving. If I edit the field again and change the Allowed Format to Filtered HTML, for example, the Default Value text area has the following message after saving and re-editing "This field has been disabled because you do not have sufficient permissions to edit it." Basically you have to enable both Full and Filtered to be able to switch Default from Full to Filtered. I guess at a minimum that message could be changed to better describe the situation or the default could be changed automatically.

Anyway, I am now moving on to research the suggested form modifications and will post progress later.

Yup, see filter_form_access_denied(). Can't be adjusted here.

Love to see this proceed! Hope we can push this.

Referring to a different task that is similar. We'll need to decide for one direction and mark the other duplicate.
#1275266: Add field level default format with full Better Format options

At long last I give you my patch. :) Being my first, I would certainly appreciate any feedback.

This patch adds a radio toggle to enable/disable the use of allowed formats checkboxes.

Yes, I wasn't sure about that. Thank you for the input. :) Doing it now.

OK, this patch has all changes combined. It also should have the necessary header to be applied properly. ;)

Some notes on a quick review of just the patch code.

1. There is a debug statement left in better_formats_form_field_ui_field_edit_form_alter().

2. Is there a reason we are limiting this feature to only be used by text_long and text_with_summary fields? I would like to see as many features as possible work dynamically with all entities and fields that we can reasonably support instead of building in specific support. Would it be sufficient to just check for the text_processing setting? That way any field that implements formats in the standard that core does will automatically get support.

Apologies concerning the debug statement.

I will wait for feedback from Crell and Sun concerning the limitation to text_long and text_with_summary before re-rolling.

thx

Ah, I didn't know about those details. I will work on the third radio option. Are you suggesting to use javascript to toggle the display of the checkboxes? If so, Is there a preferred method for implementing that sort of thing?

Thanks for the tip concerning whitespace. I'll be sure to not add it in the future.

Strip trailing whitespace enabled. Thanks for the tip. :)

Is this the same work as #1275266: Add field level default format with full Better Format options ?

Update: Appears #13 already eluded to this.

Note that (after removing the debug statement) the patch doesn't save the field format settings. Also the "Enable allowed formats below" contains no value on load.
So the patch is not functional.

Also after applying the patch i get the following error twice on admin/structure/types/manage/MYTYPE/fields/field_MYFIELD
Warning: htmlspecialchars() expects parameter 1 to be string, array given in check_plain() (line 1464 of /var/www/d7/mod_simplenews/web/includes/bootstrap.inc).

So what about http://drupal.org/node/1275266?

tagging

Crell,

Is this a duplicate issue?

No, this is not a duplicate issue.

This issue concerns itself with "Allowed formats", #1275266: Add field level default format with full Better Format options is about field level "Default formats", essentially an architectural and version update of #350696: Per field format settings with full BF options via CCK widget, and so they are separate however the 2 features should be able to work together.

Okay. Thanks for the clarification. Will get back on it.

Generally patch form #17 works for me well, but I also think that the Yes/No radio buttons are not necessary. There is also one debug drupal_set_message call and a stray print_r/var_dump somewhere.

I just tried the patch from #17 and if I select Plain Text (the text format) as the only allowed format, the field gets locked, saying that I don't have permission to edit edit. Even as user #1

Update: It seems to only affect fields that had a value prior to me installing the module and applying the patch

Update 2: Ah. It's because the node was originally saved with filtered html as format. Is this something that needs to be adressed?

@Crell: I don't think there should be a permission for it. As you say, this is normal behavior. However, it renders the node completely un-editable unless you change the allowed formats settings again. In my mind, the field on the already created nodes should turn into one of the allowed formats. However, I don't think that's something that's easily done and could cause other problems.

Maybe better_formats could add a friendly warning, so editors know there fields with formats inaccessible to the current user.

I'm not 100% sure what the security question is exactly, can only guess it's this:

the field on the already created nodes should turn into one of the allowed formats.

A clear No to that. The assigned text format should never be manipulated automatically. If the already assigned format is not accessible for the current user, the field must not be editable. This is essentially done in the last lines of filter_process_format().

Since text formats are merely hidden here, but the current user may still have access to the assigned format, you should technically end up with a #default_value for the text format widget that is invalid (because the value does not exist in #options).

If there are multiple formats visible, and the assigned format is not contained, then the select should be #required (which automatically adds the #empty_option "- Select -"), so you cannot simply submit the form (and unintentionally change the format).

If there is only one format visible, and it's not the assigned format, then the situation gets a bit hairy. The assigned format might have been "PHP code" or some other format involving security related filters. In that case, the format would be changed under the hood, potentially leading to security issues. I think the only sane thing to do is to re-implement those lines of filter_process_format() and effectively disallow editing of the field.

In any case, this should be covered by automated functional tests, so we can be sure that there are no security issues.

Yup, switching the text format automatically is definitely a security issue. Besides what @sun said, another good example would be an attempted cross-site scripting attack originally saved as Filtered HTML (and therefore ineffective) which gets accidentally switched to Full HTML when edited by an administrator and suddenly starts working. A human needs to make the decision to switch explicitly.

For background on what core does here, see #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x.

The relevant code from filter_process_format(), which looks like it's already been copied into Better Formats, is basically this:

  $all_formats = filter_formats();
  $format_exists = isset($all_formats[$element['#format']]);
  $user_has_access = isset($formats[$element['#format']]);
  $user_is_admin = user_access('administer filters');

  // If the stored format does not exist, administrators have to assign a new
  // format.
  if (!$format_exists && $user_is_admin) {
    $element['format']['format']['#required'] = TRUE;
    $element['format']['format']['#default_value'] = NULL;
    // Force access to the format selector (it may have been denied above if
    // the user only has access to a single format).
    $element['format']['format']['#access'] = TRUE;
  }
  // Disable this widget, if the user is not allowed to use the stored format,
  // or if the stored format does not exist. The 'administer filters' permission
  // only grants access to the filter administration, not to all formats.
  elseif (!$user_has_access || !$format_exists) {
    ...
  }

I think with some slight modifications to the if() statement (to accommodate the new class of formats that the user has permission to use generally but not on this specific form) that's basically the right behavior here too.

Although actually, it would be much safer for Better Formats to act after the above security-related code runs, rather than trying to modify it. That may or may not be possible. Either way, this issue reminds me that for Drupal 8, we really should think about splitting up filter_process_format() or otherwise making it easier for contrib modules to override the "cosmetic" parts of what it does without having to touch the security-related parts.

I'm a little unclear about the security question here. Drupal 6 core provides the 'administer filters' permission which let you access all formats. What that facility removed in 7? It looks like it from http://api.drupal.org/api/drupal/modules--filter--filter.module/function...

Well, as above #3 basically existed in Drupal 6, but it's unclear to me at this point why you would not simply assign permission to all formats rather than re-introducing something that regularly caused confusion.

So, my vote is with #2, as long as an admin with appropriate permissions can actually fix up the node.

This is not about user permissions to a text format -- that case is already handled by filter_process_format().

Thus, the user editing the node/field actually has access to the previous format - it is merely not exposed in the UI.

Because of that, I'd say it is acceptable to expose the field as usual and let the user edit it -- as long as you ensure that the format is not unintentionally changed under the hood without explicit re-assignment of a format by the user.

As already mentioned in #38, that should be easily doable if there are multiple formats.

Only in the case where there is just one format exposed and thus no text format selector at all, the format would be changed implicitly without notice, and thus in that case, the behavior should be 2) "no edit for you; find someone who has access", i.e., as if the user had no access to the previously assigned format.

But of course, 2) is only possible if better_formats actually has an option or permission which disables the format drill-down for certain privileged users in the first place... (d'oh ;)

Right, either #2 or #3 is OK from a security perspective, but so is @sun's option (call it #4) and that one seems pretty reasonable.

Only in the case where there is just one format exposed and thus no text format selector at all, the format would be changed implicitly without notice...

Even in the case where there is just one format, it should be possible to force the text format selector to be exposed and thus still require the user to "choose" a new format (though it's a choice with only one allowed answer), which is actually what core does in this situation via part of the code I pasted in #39, right?

In any case, this patch will by definition allow people to configure themselves into nonsensical situations, and "caveat configurator" seems like a pretty good policy there :) They can always unconfigure themselves out of it via the Better Formats settings page, even if the node edit page winds up unusable.

The biggest issue remaining (from a security perspective) is in the implementation, to make sure not to step on any of the existing security checks that filter_process_format() is doing. I'm looking at the current Better Formats code now and it's not obvious to me why it needs to copy that entire core function and modify it, rather than running after it. If we changed that here, then it would be a lot simpler to come up with a patch for this functionality that is guaranteed not to interfere with the existing core security checks... If I have time I'll see if I can get that working.

Here's a revised patch that works as described above; better_formats_filter_process_format() now runs after filter_process_format() rather than in place of it, and thus we can guarantee we aren't stepping on any of the core security checks, just adding our own modifications at the end. Also results in less overall code :)

$ diffstat 1295248-per-field-restriction-46.patch 
 better_formats.module |  302 ++++++++++++++++++++++----------------------------
 1 file changed, 136 insertions(+), 166 deletions(-)

When editing a field whose current assigned text format is no longer supposed to be available, it uses the method @sun described above, which is borrowed from Drupal core (marks the field as #required with no #default_value so that "-Select-" shows up and the user is forced to make a new choice). If the user doesn't have access to any of the text formats that are currently supposed to be available for that field (e.g. if "filtered HTML" and "full HTML" are configured as the acceptable formats but the current user only has permission to use "Plain text"), it prevents the field from being edited by the user entirely; it's probably possible to come up with nicer behavior there, but it's kind of an edge case anyway.

The patch may still need work for some of the UI feedback above; I didn't really touch that part of the patch at all.

@David_Rothstein: Awesome, this looks great! :)

Even in the case where there is just one format, it should be possible to force the text format selector to be exposed and thus still require the user to "choose" a new format (though it's a choice with only one allowed answer)

Yeah, even if the behavior might be a bit "senseless" for the user, I think that's the proper way to go, as the user editing the content is made fully aware that the text format is going to change.

My only remark for anyone who's going to take this patch on:

+++ b/better_formats.module
@@ -65,196 +65,119 @@ function better_formats_menu() {
+        // If there is no text in the field, it is safe to automatically assign
+        // a new default format. We pick the first available option to be
+        // consistent with what filter_default_format() does.
+        if (empty($element['value']['#default_value'])) {

Should be if (!isset(#default_value) || #default_value === '')

Patch in #46 works great, haven't found any issues, this should have been in Drupal Core tbh!

Works great here.
The UI might be improved with some stateful enable/disable.
I would love to see this fixed soon.

Patch in #46 does the trick and allows me to restrict formats as promised. A much needed feature that I hope will be committed to the dev version soon.

Only two things I noticed is:

• The UI could be improved. The Yes/No option plus the list of text filters are confusion. Replace the Yes/No with a check box "Limit which text filters that can be used" and only if its ticked, the text filter list will be shown.
• When only having one text filter allowed, there is a broken light-gray box under the editor on node edit pages where the previous drop-down menu for selecting format previously was. Not sure if that is due to a bug with this code or not though.

However, these two things can be dealt with after the code been committed so we don't have to continue patching.

Cross-post: This patch and #1275266: Add field level default format with full Better Format options are aiming for adding a similar feature improvement. Would be great if the authors could agree and work together on a solution so we can get this much needed improvement committed thanks.

Hu? They aim to do exactly the same thing in my book. Just with different UI and that this patch also works properly as the other doesn't manage to not show CKEditor if I only allow Plain text for example.

I definitely like the draggable UI the other patch has, except that I have to check those to disable which I find unintuitive, should be the opposite.

They are not the same, see my comment in #29.

Any statement as to when/if this will make it into DEV? Is the patch in #46 against the 2011-Sep-16 dev version? Thanks for all the work on this, folks! This is a huge UX boon.

@dragonwise: OK, take your word for that they are not the same.

However, it would be nice to get some idea of what the ETA is for this making into the module. Its seems to me that both patches are pretty stalled right now for some reason. I think it would avoid a lot of unsertainy if you could let us know your plans for this. Hopefully that will then help guiding the patch authors to complete their work.

Providing a new patch here. This time including states support to display format selections only if format limitation enabled.
Changed text descriptions, switched to checkbox to enable format limitation. Much better usability i think.

Would be great if someone can test and we can make it RTBC again. Hope we're now one step more towards committing this.

In addition, added suns inputs.

+      '#description' => t('Select the input formats allowed for this field. Note that not all of these may appear if a user does not have permission to use them.'),

Minor issue: "input formats" should presumably be changed to "text formats". (I noticed it earlier, but forgot to add it in my patch above.)

I been testing it today and is limiting the input formats but there is something wrong with order detecting as example if I have (order in admin/config/content/formats):

*Filtered HTML
*Custome
*Plain

and I limit to display only Custom one it is show me Filtered HTML instead of Custom. In any other order it is showing me always the first one.

This is a feature request!
ShadowMonster - Please provide steps to reproduce that in a clean installation..

Tested #58. Behave as expected except for on field type text (char 255) where it does nothing. Looking in function better_formats_form_field_ui_field_edit_form_alter() and we enable it for only these:
if (in_array($form['#field']['type'], array('text_long', 'text_with_summary'))){}

But is would prefer we also does it for text:
if (in_array($form['#field']['type'], array('text', 'text_long', 'text_with_summary'))){}

Yeah, we need to be able to select the 'text' type too :)

I also tested #58 and it seems working fine. There is a minor issue where the fieldset below the textarea is displayed even if there is only one allowed format and therefore empty.

And although the fieldset appears to be empty when only one format is allowed, there is a hidden <input> element in it, so we should hide the fieldset by adding a class ("element-invisible") or style ("display: none") instead of removing it.

Of course, the fieldset would not be empty if format tips were enabled, in which case it need not be hidden.

To reproduce #63 do you need these settings in admin/people/permissions

Attaching a patch that add betterformats to text-fields.

Known issues

If you use the http://drupal.org/project/title field could you run into problems. Could prob. need some more testing.

Documenting that #1419016: Allow text fields to enforce a specific format is also out there. Perhaps we can join efforts?

Shameless plug: #1423244: #allowed_formats property for #type 'text_format' to filter available formats

Wow, there are apparently a lot of closely-related Drupal core issues on this topic! The one I knew about is actually #784672: Allow text field to enforce a specific text format (which is a couple years old and even originally had a bit of an effort to get it into Drupal 7 although that didn't happen).

I'm going to take a look at all those and see if I can consolidate them a bit... In the meantime, I agree that for Drupal 7 the current issue we're in is the best place to focus for now. (Even if part of the core code could be backported to Drupal 7, it would still need to go in Drupal 8 first anyway.)

Committed. Thanks to everyone.

Thanks @dragonwize. You just increased the usability of this module exponentially by not forcing users to maintaining a patched version.

Great job everyone! :)
Commit: http://drupalcode.org/project/better_formats.git/blobdiff/9106bcf3475eed...