Remove the format delete reassignment "feature"

History: #428296: Filter system doesn't communicate with modules about altered text formats (note in particular #1) and #635212: Fallback format is not configurable. The first patch was not designed to do a complex update and there was no need for that because the fallback was not yet configurable. That's when things got out of hand.

The whole notion of filter format deletion is not a real world thing. Yes, if you experiment then you want to delete. But if you have content in that format??

The biggest argument for this operation was D6->D7 but that's not real world either, CCK will need to have that update path and CCK knows that it's content in SQL, it's not in a NoSQL database, it's not in a remote possibly read only field storage... simple fix there. No need to burden D7 with such an impossible functionality.

Even better.

The reassignment is not "ill conceived" if we go with Barry's mapping patch which I still think is our best option for D7.

OK, so we disagree. It is quite a reasonable user experience for and admin to realize they have 2 similar text formats and to want to delete one and consolidate into the other. Since we already have a patch which supports it, I can't get behind a solution like "we couldn't figure this out for D7".

Even if Moshe disagrees -- he does not take into account that Barry's patch has an insurmountable barrier in the form of sun. We have wasted months debating his patch -- we know it's not a solution either that many will get behind. There are already arguments in this very issue against, database inconsistency issues most importantly.

http://cyrve.com/node/23 Get Real, Get Dirty, and Give up (on perfection). We are in the 'least of all evils' stage. Easy resolutions are unavailable. All software ships with bugs, and sometimes serious ones.

Note the alternative is http://drupal.org/files/issues/drupal.kill-sanity.135_0.patch from http://drupal.org/node/914316 (or the original issue as it is only a repost there.) It makes JOINs impossible on filter_format table impossible (aka breaks FK) and therefore Damien and me are against, sun has, as he usually has, broader, vaguer and harder to understand problems with it.

@Moshe - it makes sense sometimes for people to merge similar formats which have grown closer over time. It also makes sense for people to merge taxonomy terms which have associated content, or node types which end up with the same settings and field structure - that's only supported in contrib or custom updates though.

At least the following modules are ones which would cause information disclosure or or something other issue if they were assigned to one text format which was deleted, and the associated content moved to another one without the same filter:

http://drupal.org/project/restricted_text
http://drupal.org/project/spamspan
http://drupal.org/project/wordfilter
http://drupal.org/project/reptag
http://drupal.org/project/spoiler
http://drupal.org/project/gtspam
http://drupal.org/project/hidden_content
PHP module

In general, I think this approach is good enough to ship in Drupal 7. My only concern is what happens when you edit a node with a disabled format.

It's worth mentioning D6 behaviour.

At the top of the delete-form, text says:

If you have any content left in this input format, it will be switched to the default input format. This action cannot be undone

• When the form is submitted, the form-submit handler resets the filter-format for nodes (body-text), comments, and blocks from the deleted format to the default format.
• If any contrib modules use filters, they would have to add a form-submit handler for the filter module's delete form, and implement their own filter-deletion strategy. I bet they're not doing this.

To me, this approach is broken, because core says if you have any content...it will be switched... but it's making promises it can't keep - it's relying on contrib modules to do the same thing as core.

There are many things I might like to do if I were deleting an input-format:

• Bulk-migrate all content to a new input-format (possibly default, possibly something else)
• Review all content with the given input-format
• Not show the content until it's been reassigned a working input format

And possibly other options. Most of which is overkill for core.

Core cannot re-assign filter-formats for contrib modules (because core doesn't know which table/field the format-id is stored in), so the case of trying to display content with an invalid format has to be handled anyway (and check_markup is already doing this). Returning empty-text for a deleted input-format seems reasonable.

If migrating content for core fields is seen as important, I'd like it to be optional and opt-in (e.g. a checkbox on the input-format delete page: [ ] Change content using this format to the default input-format). I'd prefer to see this in a contrib module though. If this behaviour were to go into core, then docs should stress to module-developers that their module should also reset filter-format IDs on hook_filter_format_delete, so this behaviour is consistent (although catch points out some modules where this would be undesirable).

The patch in #6 disables old input-formats by setting a flag, rather than deleting them from the database. Effectively we're saying that input-formats can never ever be deleted, only disabled, which to me feels like an error waiting to happen.

Like Moshe, I don't understand the point of this. It would be one thing if we didn't have other solutions, but we do. All this does is take the existing broken behavior and makes it equally broken everywhere, and leaves the site admin out in the cold.

A very real scenario where the replacement feature is needed is sites updating from Drupal 6 to Drupal 7. We add a plain text format during the upgrade, but if the D6 site already had a plain text format in use, they'll likely want to delete it so as to not to have two of them.

My only concern is what happens when you edit a node with a disabled format.

That's being handled currently in #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x.

Big deal! If you used a D6 plain text and now we add another that added another has no content to it and so you can disable it safely. Done.

@manarth:

To me, this approach is broken, because core says if you have any content...it will be switched... but it's making promises it can't keep

In Drupal 6, the promise is kept fine (at least from the site admin's perspective), as a result of the fix that went in SA-2008-073.

@chx: The new one is the fallback format, and so cannot be disabled. (Although it could be with custom code or a contrib module.)

I need to think about the consequences of this patch.

At the very least, the patch does not update filter_process_format() accordingly, so needs work. Its behavior needs to be defined first though.

A very real scenario where the replacement feature is needed is sites updating from Drupal 6 to Drupal 7. We add a plain text format during the upgrade, but if the D6 site already had a plain text format in use, they'll likely want to delete it so as to not to have two of them.

To me that says that there is an issue called " upgrade creates a plain text format when one already exists"

I have to agree with Damien, simply put, core should deal with the fall back content-types, otherwise its contrib

didn't mean to change tags in the above post ( sorry)

Agreed with Damien in #23. Or rather, there is a need to update filter_process_format(), but that's an existing need, and I believe the code already being worked on for that at #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x would work equally well with the approach here?

***

@verbosity: We can't assign an existing format to be the fallback format because it would lock them in to a fallback format they might not want. It needs to be a new format so they are free to configure it after the upgrade however they want (e.g., change or replace filters) without affecting any existing content on their site. Also, if you can figure out how to programmatically detect which existing format on a site is "plain-text-y" enough that it duplicates the one we want to add, I'd be impressed :)

Sorry but what's the actual harm caused by having a 'check plain' format from D6, and the new check plain format created by D7? How many sites have will have this problem? Will it be entirely equivalent to check_plain() or will it use the line break or link filter? Also the non-existence of a filter-reassign module in contrib is not a reason to keep features in core, sometimes the lack of a module in contrib is because there's no pressing need for one.

In above comments, everyone assumes that the replacement format would be different from the deleted format. That's not necessarily the case. Replacing a format with a format that is identical or almost identical to the deleted format is a perfectly valid use-case and operation and does not break anything.

Yes, it's valid. So is:

* Merging two similar taxonomy terms
* Merging two or more identical node types
* Moving a taxonomy term between vocabularies.
* Merging two vocabularies.
* Splitting comments from one node to a new one when the discussion goes off-topic
* Reassigning content from one user account to another

However, core does not have support for any of these, because 1. they're hard problems to solve 2. They can be solved via contrib modules (and some of the above are, some aren't) 3. No-one has come up with an acceptable solution for core yet. 4. while I'm sure some of these are more common than wanting to merge text formats (certainly I've wanted to do all of the above except for the user account one), they're not such common tasks that there are hordes of people working on getting them into core.

Just because we have some help text in core claiming that we support text format replacement doesn't mean it should be subject to different criteria to those features. If someone opened a critical issue for merging taxonomy vocabularies or node types (irreversible decisions too) this week, how would you treat it? Now what if there were 5-6 competing approaches to the issue, all with some kind of drawback and with varying support or disagreement from numerous core developers?

Didn't have much time to focus on this yet, but so far, I think we can do it.

However, I have the following change requests:

+++ modules/filter/filter.module
@@ -258,15 +259,15 @@ function filter_format_save(&$format) {
  * Delete a text format.
  *
+ * This will actually set the filter format as disabled, so that any reference
+ * to this text format will never be reused.
...
+function filter_format_delete($format) {

@@ -274,11 +275,7 @@ function filter_format_delete($format, $fallback_id = NULL) {
+  module_invoke_all('filter_format_delete', $format);

I'm not really happy with this API function and hook name still be named "delete" when it is not actually deleting the format. To ensure that modules are properly updated and also allow to re-introduce the removed hook_filter_format_delete() hook, I think I'd prefer to rename the existing stuff to filter_format_disable(), including the UI text + help. Additionally, the added phpDoc explaining the DX WTF that the function does not actually delete, despite being named delete, is no longer needed then.

+++ modules/filter/filter.module
@@ -258,15 +259,15 @@ function filter_format_save(&$format) {
   db_delete('filter')

The {filter} rows have to be kept, or otherwise, the format no longer works (which is not the intention of this patch; i.e., the intention of only disabling/hiding a format instead of deleting).

Lastly, I agree that filter_process_format() doesn't need to be updated, as we'll already take care for this scenario in #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x

Powered by Dreditor.

David, sun: sorry if I said any harsh words. I got a bit short tempered -- but that's just because I want to get Drupal 7 out ASAP. I still like both you, OK? :)

Here is the patch reroll.

Thanks! Much better and cleaner.

+++ modules/filter/filter.module	2010-09-17 18:46:18 +0000
@@ -488,7 +484,7 @@ function filter_default_format($account 
  * The fallback text format is a regular text format in every respect, except
  * it does not participate in the filter permission system and cannot be
- * deleted. It needs to exist because any user who has permission to create
+ * disabled. It needs to exist because any user who has permission to create
  * formatted content must always have at least one text format they can use.

@@ -503,12 +499,12 @@ function filter_default_format($account 
 function filter_fallback_format() {
   // This variable is automatically set in the database for all installations
-  // of Drupal. In the event that it gets deleted somehow, there is no safe
+  // of Drupal. In the event that it gets disabled somehow, there is no safe
   // default to return, since we do not want to risk making an existing (and

The first text here needs a bit more tweaking (it neither can be deleted nor disabled), and the second text should be reverted to "deleted".

Powered by Dreditor.

Tweaked both to mention deletion too and made the test pass.

Thanks.

@chx: No worries, apology accepted. And I appreciate your passion for Drupal :)

Technically, this looks fine. The other objections I and others have to this are already noted above and don't need to be repeated. We can let some core committers review and decide.

However, I see these remaining issues:

  * Implements hook_filter_format_delete().
- *
- * @todo D8: Properly update filter format references in all fields. See
- *   http://drupal.org/node/556022 for details.
  */
 function text_filter_format_delete() {

This hook doesn't exist anymore; it should be text_filter_format_disable().

     $form['formats'][$id]['configure'] = array('#type' => 'link', '#title' => t('configure'), '#href' => 'admin/config/content/formats/' . $id);
-    $form['formats'][$id]['delete'] = array('#type' => 'link', '#title' => t('delete'), '#href' => 'admin/config/content/formats/' . $id . '/delete', '#access' => !$form['formats'][$id]['#is_fallback']);
+    $form['formats'][$id]['disable'] = array('#type' => 'link', '#title' => t('Disable'), '#href' => 'admin/config/content/formats/' . $id . '/disable', '#access' => !$form['formats'][$id]['#is_fallback']);

"Disable" should be lower-case (just like 'configure' is).

   return confirm_form($form,
-    t('Are you sure you want to delete the text format %format?', array('%format' => $format->name)),
+    t('Are you sure you want to disable the text format %format?', array('%format' => $format->name)),
     'admin/config/content/formats',
     NULL,
-    t('Delete')
+    t('Disable')
   );
 }

The word "disable" could be very confusing here by itself, because I don't think people are going to understand that once it is disabled it cannot be reenabled (at least not by Drupal core). Modules, for example, can always be re-enabled. I think this confirm form needs a description that explains to people exactly what will happen to their content when the text format is disabled.

+ * id is never reused. As there might be content using the disabled format,
+ * this would lead data corruption.

Minor: should be "lead to data corruption".

Note that we are fixing #915168: Foreign key support is missing from text and file module so that contrib can know where filters are stored.

I think this confirm form needs a description that explains to people exactly what will happen to their content when the text format is disabled.

#36 adds the standard warning that "this action cannot be undone." We could possibly do better than that with a week of bikeshedding, but it's certainly clear enough now to go forward with.

Everything else David mentions is taken care of, so back to RTBC.

Glad to see there is some level of consensus. I think this patch is a step forward. I also think this patch allows us to move on to other critical issues. In other words; committed to CVS HEAD. Thanks everyone for all your hard work and persistence. :)

Let's not say there was consensus: What happened was that Barry got so frustrated that he gave up and stopped participating in the issue, and I (and I think Moshe too) just got tired of it...

This was an unnecessary API change this late in the cycle. But if we keep it, we need to figure out a way to explain it so we don't get flooded with support requests from people who can't figure out why they can't find their disabled text formats anywhere in the UI and also why their content is disappearing. "This action cannot be undone" is not sufficient, and is not consistent with what we do elsewhere when a confirm form has an unexpected side effect (see, for example, the form for deleting a content type when content of that type already exists).

My suggestion would be something like the attached.

I also created a HEAD to HEAD issue for those who care about such things:
#916414: Update for #914458 (filter_format schema change)

It's also IMO unacceptable that once you disable a text format you can't create one with the same name. If you try you get:

Text format names must be unique. A format named Full HTML already exists.

Even though "Full HTML" doesn't exist anymore on your site.

Fixing that would of course require even more schema changes.

Fixing that would of course require even more schema changes. <= it would only require changing the name to contain the id. as the id is unique it could be removed later if there is a revival functionality. so, change Plain text to 1:Plain text when disabling. (Of course, Plain text can't be disabled. Juts for example)

The currently enforced uniqueness on a text format's title is not required at all. Instead, we want to do #903730: Missing drupal_alter() for text formats and filters, which depends on #902644: Machine names are too hard to implement. Date types and menu names are not validated

Great improvement to the warning. Committed to CVS HEAD. Thanks.

Back to needs work for #43, which requires #903730: Missing drupal_alter() for text formats and filters

subscribe

#43 is a bug, but non critical.

I moved #43 to its own issue - seems like it's better off being dealt with there: #949220: Text format names can never be reused (Possible solution: Allow disabled text formats to be re-enabled)

However, if I'm not mistaken this one is still "needs work" for documentation in the module upgrade guide?

Already documented in http://drupal.org/node/224333#text_format_updates

I see this marked as an API change. Can anybody describe it and suggest whether it should be announced?

I think it would be a good idea to announce, yes. The important changes I can think of are:

1. The API function filter_format_delete() is now filter_format_disable(), and its behavior has changed. Instead of actually deleting the text format, it now just marks it as disabled in the database (via setting its "status" to 0).
2. Similarly, hook_filter_format_delete() is now hook_filter_format_disable(). With the old hook, every module that stored text format data was supposed to implement it, and replace references to the deleted format with the replacement format (which was passed in as the hook's second parameter). Now, there is no second parameter, and modules no longer need to do anything when a format is disabled (they can leave the out-of-date references in their database tables). The hook is only there for modules that need to respond to a text format being disabled for other reasons - e.g., to clear caches.
3. Anyone who is doing a database query against {filter_format} needs to be aware of the new status column in that database table, and the fact that rows with a status of "0" represent disabled formats that are effectively deleted and not allowed to be used anymore.

Something like the above is what I thought needed to be updated in the module upgrade docs, but looking at them again it seems that because filter_format_delete() and hook_filter_format_delete() were new in D7 and never mentioned in the module upgrade docs in the first place, there is nothing to update...