Posted by Damien Tournoud on January 15, 2010 at 8:57am
10 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | filter.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | D7 upgrade path, Needs Update Documentation |
Issue Summary
filter_update_7003() hardcodes a list of (module, delta) -> (filter name) migrations, and just after that adds a unique key on the table. If the {filter} table contains any non-core (thus non migrated) filters, adding the unique key fails horribly.
Comments
#1
confirm this bug, additionally I got warnings for filter tips after migration
#2
http://api.drupal.org/api/function/filter_update_7003/7
http://api.drupal.org/api/function/filter_update_7004/7
What can we do here? I see two options:
1) Don't add a unique key.
2) i) Change 'delta' to 'name'. ii) Prefix all values in 'name' with 'module' + '_'. iii) Perform the update based on the renamed 'name' instead of 'delta'. iv) Contrib modules will have to update from those renamed 'name' columns, too ($module_$delta).
Any other ideas?
#3
Tagging.
#4
What about duplicating the table before we move it, then document that D6 modules need to check {filter_old} for their updates? We could also change the core update to follow this so it serves as an example.
#5
In working with the update process, i think I found why 7003 and below fails on D6 -> D7 updates
In filter install // $Id: filter.install,v 1.32 2010/02/18 04:36:38 webchick Exp $
Line 185 states
db_drop_unique_key('filter', 'fmd');
in all my D6 databases fmd is a index. think the code should be
db_drop_index('filter', 'fmd');
#6
Same here. A vanilla D6 -> D7 upgrade will work, but if you have non-core filter, you won't be able to upgrade.
Sound. Each contrib update function should in the end check if {filter_old} is empty and remove it. Could core provide such functionnality maybe, like a helper update function contrib could call?
@ctmattice1: db_drop_unique_key('filter', 'fmd'); looks ok to me: in D6, 'fmd' is a unique index and 'list' is an index, hence this code in filter_update_7003():
<?phpdb_drop_unique_key('filter', 'fmd');
db_drop_index('filter', 'list');
?>
#7
@scor: if contrib modules don't remove it, we can always add an update in Drupal 8 - can open issue now so we find it later.
#8
instead of {filter_old}, I'd rather use a more meaningful name explaining where it came from. How about something like filter_update_6_to_7? looks like the old files table (upload.module) will also need to remain so contrib modules can do their own upgrade. We might want to use some convention here, so people know in several years when upgrading for 7 to 8 or 8 to 9 what these old tables are there for, and whether they can be dropped as part of a manual cleanup.
#9
A new db_drop_empty_table() function would totally make sense. Idea: Stuff some invocations of that into update.php's batch API finished callback, so any obsolete tables are automatically removed over time.
#10
@scor: A simple suffix of "6" (i.e. filter6) would sufficiently explain the situation, IMO.
EDIT: A prefix instead of suffix might even be preferable, so all old/obsolete tables are nicely grouped together.
#11
The 6filter feels like a hack but it seems there's no better alternative. Here is an experimental attempt.
#12
Patch looks good but I can't live with 6filter. Even if SQL allows table names beginning with an integer that just looks horrible. Would prefer d6_upgrade_filter or something.
#13
same as #11 with 'd6_upgrade_filter'.
#14
Looks great.
#15
Hmmm. This is ugly as hell, but catch just explained (patiently ;)) to me why this is needed. Basically, we can't drop the table because contrib modules still have their data in it, and need something to reference. This is similar to what we had to do with Actions in D6.
Therefore, committed to HEAD. Another critical issue bites the dust! :)
#16
Automatically closed -- issue fixed for 2 weeks with no activity.
#17
Sorry Re-opened,
I still see a problem with a borked table. If this is not the place then feel free to tell me and I'll create a new issue.
What is happening is this:
upgrade from d5 to d6
upgrade from d6 to d6.16
d6 filters table completely borked, multiple entries with same data only difference is the fid.
When getting the data from d6_upgrade_filter then writing to filter table it bombs here
foreach ($result as $record) {db_insert('filter')
->fields(array(
'format' => $record->format,
'module' => $module,
'name' => $new_name,
'weight' => $record->weight,
))
->execute();
}
on duplicates.
example D6.16 filters table below:
format module delta weight fid
4 urlfilter 0 0 1
2 php 0 0 2
1 urlfilter 0 10 3
1 urlfilter 0 10 4
1 filter 1 1 5
1 interwiki 0 10 6
4 filter 1 1 7
5 textile 0 0 8
4 urlfilter 0 0 9
4 filter 1 1 10
5 textile 0 0 11
4 urlfilter 0 0 12
4 filter 1 1 13
5 textile 0 0 14
3 footnotes 0 10 15
3 filter 1 0 16
1 filter 3 11 17
3 filter 3 11 18
There needs to be a way to query the filter table to determine if a filter has already been written, if so then just delete the duplicate in the d6_upgrade_filter table. Attached a Screenshot of D6 filter table
#18
One line fix
added
$query->distinct();patch attached
#19
Should really have been a new issue but seems fine to me.
#20
+++ modules/filter/filter.install 30 May 2010 02:30:09 -0000@@ -258,7 +259,7 @@ function filter_update_7003() {
- ->execute();
+ ->execute();
let's not break this indentation. The second hunk needs to be removed.
#21
Ouch, re-rolled.
#22
#23
Committed to HEAD. Thanks!
#24
Automatically closed -- issue fixed for 2 weeks with no activity.
#25
While working in contrib I suddenly realized that my upgrade path was missing. I remembered this issue and just sort of copied the code.
It seems many fully upgraded contrib filters have ignored this upgrade path. Perhaps because of missing update docs. Hence adding the tag.
#26
Something like this for the update docs?
Core duplicates the {filters} table since core cannot take care of the potential contributed module filters. Contrib can follow the example set in filter_update_7003() to rename and re-enable filters.
#27
Thanks for reopening this issue! I've added
http://drupal.org/update/modules/6/7#filter_update
which I think should take care of it (reviews welcome, but marking fixed in the meantime).
#28
Automatically closed -- issue fixed for 2 weeks with no activity.