Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The feature delete reassignment feature (mass-reassigning all the rich text pieces stored everywhere in the database to a different format when a text format is deleted):
- is ill-conceived: it is doubtful that in most cases you would want all the text pieces to be reassigned to a different format when you need to delete one... generally, you would rather *convert* the text pieces from one format to the other, before reassigning.
- cannot work in all cases: as we saw in #556022: When a text format is deleted, some modules (like text.module) don't update their data, even though we say they do, we cannot make it work reliably (ie. without relying on the Batch API) in all cases.
- belongs in contrib: deleting a text format basically means migrating data from one format to the other. This is a complex operation that core shouldn't have to tackle.
As a consequence, I suggest we remove the feature and let contrib figure out the best solution here.
Comment | File | Size | Author |
---|---|---|---|
#41 | disable-text-format-914458-41.patch | 845 bytes | David_Rothstein |
#36 | 914458-remove-text-format-reassignment.patch | 24.61 KB | Damien Tournoud |
#36 | diff-32-36.patch.txt | 2.9 KB | Damien Tournoud |
#32 | filter_disable.patch | 23.34 KB | chx |
#29 | filter_disable.patch | 22.05 KB | chx |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedStarter patch.
Comment #2
chx CreditAttribution: chx commentedHistory: #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.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedObviously, we need to figure out what to do with text formats that are deleted. Actually, if we take the stand of not trying to migrate data, every text attached to a deleted text format will simply be rendered as an empty string by
check_markup()
: the code is already there.The only remaining issue is that (1) the text format ID could potentially be reused because of the way autoincrement columns work, (2) just removing the row from {filter_format} theoretically breaks the foreign keys constraint to every other tables that use this text format.
The simplest solution is to simply mark a text format as "disabled" instead of completely removing the row, by adding a status column to {filter_format}. This is what this new patch does.
Comment #4
chx CreditAttribution: chx commentedThe 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.
Comment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedFixing the two tests that do not apply anymore.
Comment #7
chx CreditAttribution: chx commentedEven better.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedThe reassignment is not "ill conceived" if we go with Barry's mapping patch which I still think is our best option for D7.
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedBarry's mapping patch can make the replacement work, but it doesn't change the fact that it is an ill-conceived features. Most of the time, reassigning all the text content from one format to the other is not what you want. This type of conversion is nothing more then a migration of data (in the sense of the migrate module) from one structure to the other. It's not something core should be in the business of.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedOK, 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".
Comment #11
chx CreditAttribution: chx commentedEven 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.
Comment #12
chx CreditAttribution: chx commentedNote 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.
Comment #13
catch@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
Comment #14
pwolanin CreditAttribution: pwolanin commentedIn 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.
Comment #15
manarth CreditAttribution: manarth commentedIt's worth mentioning D6 behaviour.
At the top of the delete-form, text says:
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:
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.
Comment #16
David_Rothstein CreditAttribution: David_Rothstein commentedLike 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.
That's being handled currently in #358437: Filter system security fixes from SA-2008-073 not applied to Drupal 7.x.
Comment #17
chx CreditAttribution: chx commentedBig 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.
Comment #18
David_Rothstein CreditAttribution: David_Rothstein commented@manarth:
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.
Comment #19
David_Rothstein CreditAttribution: David_Rothstein commented@chx: The new one is the fallback format, and so cannot be disabled. (Although it could be with custom code or a contrib module.)
Comment #20
sunI 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.
Comment #21
verbosity CreditAttribution: verbosity commentedTo 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
Comment #22
verbosity CreditAttribution: verbosity commenteddidn't mean to change tags in the above post ( sorry)
Comment #23
Damien Tournoud CreditAttribution: Damien Tournoud commentedI don't see any need to update filter_process_format(). Could you clarify?
Comment #24
David_Rothstein CreditAttribution: David_Rothstein commentedAgreed 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 :)
Comment #25
catchSorry 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.
Comment #26
sunIn 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.
Comment #27
catchYes, 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?
Comment #28
sunDidn'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:
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.
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.
Comment #29
chx CreditAttribution: chx commentedDavid, 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.
Comment #31
sunThanks! Much better and cleaner.
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.
Comment #32
chx CreditAttribution: chx commentedTweaked both to mention deletion too and made the test pass.
Comment #33
sunThanks.
Comment #34
David_Rothstein CreditAttribution: David_Rothstein commented@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:
This hook doesn't exist anymore; it should be text_filter_format_disable().
"Disable" should be lower-case (just like 'configure' is).
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.
Minor: should be "lead to data corruption".
Comment #35
chx CreditAttribution: chx commentedNote that we are fixing #915168: Foreign key support is missing from text and file module so that contrib can know where filters are stored.
Comment #36
Damien Tournoud CreditAttribution: Damien Tournoud commentedImproving the UI texts and docblocks via David #34.
See diff-32-36.patch.txt for the changes since #32.
Comment #37
ksenzee#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.
Comment #38
Dries CreditAttribution: Dries commentedGlad 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. :)
Comment #39
David_Rothstein CreditAttribution: David_Rothstein commentedComment #40
David_Rothstein CreditAttribution: David_Rothstein commentedLet'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).
Comment #41
David_Rothstein CreditAttribution: David_Rothstein commentedMy suggestion would be something like the attached.
Comment #42
David_Rothstein CreditAttribution: David_Rothstein commentedI also created a HEAD to HEAD issue for those who care about such things:
#916414: Update for #914458 (filter_format schema change)
Comment #43
David_Rothstein CreditAttribution: David_Rothstein commentedIt'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:
Even though "Full HTML" doesn't exist anymore on your site.
Fixing that would of course require even more schema changes.
Comment #44
chx CreditAttribution: chx commentedFixing 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)
Comment #45
sunThe 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
Comment #46
Damien Tournoud CreditAttribution: Damien Tournoud commentedRTBC for the UI text patch #41.
Comment #47
Dries CreditAttribution: Dries commentedGreat improvement to the warning. Committed to CVS HEAD. Thanks.
Comment #48
sunBack to needs work for #43, which requires #903730: Missing drupal_alter() for text formats and filters
Comment #49
q0rban CreditAttribution: q0rban commentedsubscribe
Comment #50
moshe weitzman CreditAttribution: moshe weitzman commented#43 is a bug, but non critical.
Comment #51
David_Rothstein CreditAttribution: David_Rothstein commentedI 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?
Comment #52
sunAlready documented in http://drupal.org/node/224333#text_format_updates
Comment #53
rfayI see this marked as an API change. Can anybody describe it and suggest whether it should be announced?
Comment #54
David_Rothstein CreditAttribution: David_Rothstein commentedI think it would be a good idea to announce, yes. The important changes I can think of are:
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...