Delayed off-shoot from #11218: Allow default text formats per role, and integrate text format permissions:

1) filter_format_save() should also save/change user role permissions.

2) Installation profiles assume hard-coded ids for filter formats when granting permissions.

3) #197: API documentation explaining the logic and system of "default" and "fallback" format.

4) #197 + #200: Improve the initial fallback format, especially when upgrading. After "upgrading" my dev site HEAD-HEAD, anonymous users had access to 2 formats, not good. But not sure

- whether the fallback format shouldn't come first for fresh installed Drupal sites.
- whether the fallback format shouldn't replace "Filtered HTML".
- whether the fallback format shouldn't be the previously configured 'default format' on sites that are upgrading to D7.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

Thanks for starting this!

For some of (2) and (4), I wonder if the right solution isn't just to simply move the creation of text formats (besides the fallback) out of system.install and into the install profile? Let the profile itself decide which formats to create and what order to put them in... Then if it creates them, it also automatically has access to the correct format ID so it can use the API to properly grant permissions and solve (2).

I also think that as long as we enable the URL filter for plain text, it might make sense for the default install profile to put that one first. (The only reason we did not do that in the previous issue was that it was originally thought that perhaps this format would not be configurable at all and so would need to be completely locked down to protect against spambots, etc, but that isn't true anymore.)

For the last point of (4), the problem I see with that is that the fallback format cannot be deleted or switched. A Drupal site that's being upgraded will have tons of content that uses filtered HTML, let's say, so if we set that to the fallback format and they later decide they want to reconfigure the fallback to be something else (e.g., plain text), they won't be able to do that without ruining their existing content.

David_Rothstein’s picture

Status: Active » Needs work
FileSize
1.62 KB

If we're doing the cleanup all in one issue, then here is a start at a very simple patch that doesn't touch any of the things above, but just a couple no-brainer cleanups that are too small to bother deserving their own issues :)

(I noticed these while re-reviewing the original patch that was committed to figure out what changes needed to go in the module upgrade docs.)

sun’s picture

Thanks!

For some of (2) and (4), I wonder if the right solution isn't just to simply move the creation of text formats (besides the fallback) out of system.install and into the install profile? Let the profile itself decide which formats to create and what order to put them in... Then if it creates them, it also automatically has access to the correct format ID so it can use the API to properly grant permissions and solve (2).

Yes, absolutely. I even did not know they were there. Text formats need to be installed by the profile.

However, not sure about the fallback format... Do we have a kind of "clean-up"/sanitisation phase during installation, so we could check whether the installation profile added a fallback format, and in case it didn't, we fix that by adding one?

I also think that as long as we enable the URL filter for plain text, it might make sense for the default install profile to put that one first. (The only reason we did not do that in the previous issue was that it was originally thought that perhaps this format would not be configurable at all and so would need to be completely locked down to protect against spambots, etc, but that isn't true anymore.)

I think the entire security issues of the fallback format no longer apply after #562694: Text formats should throw an error and refuse to process if a plugin goes missing has been committed. Originally, the idea of the "fallback" format was a real "fallback" for users not having access to any other format, so they could at least post plain-text. However, I really think that this is what 99% of all Drupal sites use the "Filtered HTML" format for. In case that format gets lost, vanishes, or whatever, the filter system must fail and just output nothing.

Given that, I even wonder whether the fallback format shouldn't be configurable -- which would also solve...

For the last point of (4), the problem I see with that is that the fallback format cannot be deleted or switched. A Drupal site that's being upgraded will have tons of content that uses filtered HTML, let's say, so if we set that to the fallback format and they later decide they want to reconfigure the fallback to be something else (e.g., plain text), they won't be able to do that without ruining their existing content.

...upgrade path. Currently, there is no way to reset/alter the fallback format to "Filtered HTML", which means there is no way to NOT present anonymous users a text format widget - aside from deleting the "Filtered HTML" format and renaming the new "Plain text" to "Filtered HTML", which, as you may know, will lead to a bloat of other issues on your site (see also security hardening issue above ;).

sun’s picture

Issue tags: +D7 API clean-up

Tagging absolutely critical clean-ups for D7. Do not touch this tag.

sun’s picture

Category: task » bug
Issue tags: -API clean-up, -D7 API clean-up +upgrade, +Upgrade path

There's no real API clean-up in here (despite the user roles perhaps), so I'm removing those tags and turning this issue into a critical bug report we need to revisit for proper upgrading.

sun’s picture

Assigned: Unassigned » sun

For now, assigning to myself to not loose track of this issue. @David: Feel free to take over, please.

sun’s picture

Reconciling the list in the original post:

1) Bogus. @see http://api.drupal.org/api/function/user_role_change_permissions/7

2) Has been fixed as part of #626024: filter_list_format() hits database too often / filter_format_save() doesn't save all filters. Text formats are now also created by installation profiles. This, however, also means that installation profiles are responsible for setting up the fallback format variable, which I really felt uneasy about. Would it make sense to implement some magic here? I.e. assign the first saved text format on a site as fallback format?

3) API docs about 'default' and 'fallback' still required.

4) Anonymous users still get 2 formats after upgrading.

5) After working with this new facility some more, I strongly believe we should convert a) make Filtered HTML the default format and b) make the default format the fallback format upon upgrade from D6.

sun’s picture

6) And the fallback format needs to be configurable.

sun’s picture

David_Rothstein’s picture

Assigned: sun » David_Rothstein

OK, I can work on this sometime this week.

For (2), yeah, I am a little uneasy about that too. It seems that we should strive to make it so that there is nothing "required" that an install profile absolutely must do, but right now they are required to create a text format and assign it correctly. Maybe we can just create the fallback format as part of system.install? Then every Drupal site will have it (even if their installation profile does nothing). And an install profile which wanted to configure the fallback format differently could still load it and then re-save it with different settings, right?

For (5), I assume you meant "make Filtered HTML the fallback format"... hm, I am still a bit uneasy about this one. Need to think more about it.

For (6), isn't that already the case? The only thing you can't do is delete it.

David_Rothstein’s picture

And by "system.install" I most likely meant "filter.install" ... just remembering that it used to be in system.install, but not sure there's any reason it needs to be.

David_Rothstein’s picture

OK, this patch contains most of what we want, I think, but it doesn't actually work correctly :( This is due to the bug at #620298: Schema not available in hook_install().

So still "needs work" for now - but it should work OK if that other issue is fixed.

  1. Moved the plain text fallback format creation to filter.install and added a note that installation profiles can modify it.
  2. Added the URL filter to the plain text format to make it more useful.
  3. Added documentation for filter_fallback_format(); the existing documentation for filter_default_format() seems pretty complete as it is.
  4. Also fixed and updated the filter_format_save() documentation.
  5. Fixed a few small related bugs.

I didn't do anything with changing which format is the fallback or who has access to filtered HTML since that is better dealt with at #635212: Fallback format is not configurable.

I also did not make any changes to which format is assigned to be the fallback during D6->D7 updates, because it's still not clear what to do with that. The problem with assigning the current default to be the fallback is that then reconfiguring it becomes problematic. This is not a technical issue (they are able to reconfigure it if they want to), but the problem is its effect on their existing content. For example, if they want their fallback format to be plain text but were given their (existing) filtered HTML format during the upgrade, then any attempt to reconfigure the fallback format the way they want will break all the HTML tags that were used in their existing content. I'm still not quite sure what the best thing to do on upgrade is, but creating a separate plain text format is for now still the best option, since there's at least a clear known path to configure that the way they want to.

sun.core’s picture

Thanks, this patch looks very good already!

+++ modules/filter/filter.module	14 Dec 2009 04:44:06 -0000
@@ -475,6 +478,28 @@ function filter_default_format($account 
+ * text). When content that does not have a text format assigned needs to be
+ * filtered, the fallback format should be used, and when a text format is
+ * deleted, all content that previously had that format assigned should be
+ * switched to the fallback format.

However, this paragraph needs to vanish, it is wrong. We actually need to allow the site administrator to select the text format that all content should be re-assigned to.

Powered by Dreditor.

sun.core’s picture

Let's get this fixed. Pretty, pretty, please.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
9.22 KB

OK, here is an updated patch. I did some rewording to address #13, so see what you think, but I'm not totally sure I understand what you meant? Currently, we don't give the site administrator this freedom to choose a different format to reassign to, and changing that would probably be a different issue. The current wording reflects the current behavior.

Also, the main challenge to get this working was #620298: Schema not available in hook_install(), but I've now posted a patch over there (unfortunately more complicated than I hoped it would be). By itself, I don't expect the testbot to like the attached patch, but when combined with the patch at #620298: Schema not available in hook_install() it should work correctly, so we may need to get that one in first.

Status: Needs review » Needs work

The last submitted patch, 588882-fallback-format-cleanups-15.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
9.22 KB

Reuploading the patch. Since #620298: Schema not available in hook_install() was committed, this one should work as intended now.

Status: Needs review » Needs work

The last submitted patch, 588882-fallback-format-cleanups-15.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
13.18 KB

I think this should fix all the failing tests. Some were due to tests making bad assumptions about the text format IDs in the database, and others were due to a subtle caching bug in the filter module revealed by this patch. The fix here for that isn't 100% perfect, but it should be good enough to pass the tests, and it might be the best we can do with the hooks that core provides. Discussing that issue more at #727876: Enabling modules one at a time works differently than enabling them all at once.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

+++ modules/filter/filter.module	27 Feb 2010 23:13:26 -0000
@@ -439,6 +460,31 @@ function filter_default_format($account 
+ * When a text format is deleted, all content that previously had that format
+ * assigned should be switched to the fallback format. To facilitate this,
+ * Drupal passes in the fallback format object as one of the parameters of
+ * hook_filter_format_delete().

I still disagree with this, but I'm ok with leaving this text in here for now. We want to create a separate, critical issue to introduce a text format selector on the text format deletion form. Hence, when deleting a format, the site admin is able to choose to which format all affected content is re-assigned. Simply re-assigning to the fallback format leads to plenty of security issues.

I also did not make any changes to which format is assigned to be the fallback during D6->D7 updates, because it's still not clear what to do with that. The problem with assigning the current default to be the fallback is that then reconfiguring it becomes problematic. This is not a technical issue (they are able to reconfigure it if they want to), but the problem is its effect on their existing content. For example, if they want their fallback format to be plain text but were given their (existing) filtered HTML format during the upgrade, then any attempt to reconfigure the fallback format the way they want will break all the HTML tags that were used in their existing content. I'm still not quite sure what the best thing to do on upgrade is, but creating a separate plain text format is for now still the best option, since there's at least a clear known path to configure that the way they want to.

In addition to the above, we want to introduce a select list on the format overview page, so people can define the fallback format. Users upgrading from earlier versions have to be able to do this.

Perhaps we can re-purpose #635212: Fallback format is not configurable for both issues.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Looks good. Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

Assigned: David_Rothstein » Unassigned