Posted by pfrenssen on August 27, 2010 at 3:48pm
15 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | taxonomy.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | D7 upgrade path |
Issue Summary
Starting with a clean D6 install, I generated a number of taxonomy terms and vocabularies and then disabled the taxonomy module. Upgrading to D7 results in this error:
Update #7004
Failed: FieldException: Attempt to create a field of unknown type <em class="placeholder">taxonomy_term_reference</em>. in field_create_field() (line 297 of /virtualhosts/d7_taxopatch/modules/field/field.crud.inc).This prevents the D7 upgrade for users that have used / experimented with taxonomies in the past but have currently disabled it.
Note: I have applied the patches in #706842: Improve comments for the taxonomy upgrade path.
Comments
#1
I don't think taxonomy terms need to be in the database at all. The upgrade fails with a vanilla D6 install if you disable the taxonomy module before upgrading. If the taxonomy module has been installed in D6 (which it is by default), it will be upgraded. Actually, I think it may be upgraded regardless of weather it was installed or not, since it is a core module.
The problem as far as I can tell, is that the taxonomy module update process, relies on
hook_field_info()being able to pull information about the taxonomy field types (specifically taxonomy_term_reference) which is defined intaxonomy_field_info(), which of course does not run if the taxonomy module is disabled, since it is a hook.So, the taxonomy upgrade process probably needs to ensure that
taxonomy_field_info()is called before attempting to update the module. Perhaps it could do this by checking to see if the taxonomy module is enabled, and if not, enable it before running the updates, and then disable it again when done. I suspect this may apply to other core modules too?I'm upgrading this to critical, since in the current state, anyone who has disabled the taxonomy module, will not be able to upgrade their site.
#2
#3
I have not been able to replicate this with a vanilla Drupal 6.19 install. I disabled taxonomy module, then went through the upgrade procedure with Drupal 7 HEAD. No errors were logged during the install. After the upgrade, taxonomy module is disabled, as expected.
I do see a notice in the log, however:
Notice: Undefined variable: field in taxonomy_update_7005() (line 428 of /Library/WebServer/Documents/drupal-core/drupal7-cvs/modules/taxonomy/taxonomy.install).I will try this with generated content in the taxonomy tables.
#4
Copied from #706842-94: Improve comments for the taxonomy upgrade path by mrfelton:
+ // Allowed values for this extra vocabs field is every vocabulary.+ foreach (taxonomy_get_vocabularies() as $vocabulary) {
+ $allowed_values[] = array(
+ 'vid' => $vocabulary->vid,
I don't think we can use taxonomy_get_vocabularies() here. Not with the current state of things anyway. If someone has disabled taxonomy.module, the upgrade results in the following error:
Or at least, we need explicitly to load that function first.
Powered by Dreditor.
#5
I can confirm that the error in #1 occurs when upgrading from a clean Drupal 6.19 install, with taxonomy terms created and taxonomy then disabled.
#6
So, This isn't ideal, and is probably not usable in the long run, since it seems we we decided to just write all the sql manually instead of using api calls - but I did get the taxonomy update working by:
1) check to see if the module is enabled
2) if not, check to see if the database table is there (ie. the module is installed, but disabled)
3) enable the module
4) do all the update crap
5) disable the module again at the end.
...
function taxonomy_update_7004() {
$taxonomy_enabled = module_exists('taxonomy');
if (db_table_exists('taxonomy_term_data')) {
module_enable(array('taxonomy'));
drupal_load('module', 'taxonomy');
}
...
if (!$taxonomy_enabled) {
db_update('system')
->fields(array('status' => 0))
->condition('name', 'taxonomy')
->execute();
}
}
Note: I couldn't easily generate a patch, since I already had the taxonomy patches from #706842 applied
#7
Updating title to reflect code sprint discussion.
Conclusion was that disabled modules should be automatically upgraded when update.php is called in D7.
As a result, hook_install functions must not fire hooks (note that many API functions call API functions indirectly) nor call functions in modules.
In general, code that does fire hooks must be replaced by simple SQL statements.
#8
I believe (see e.g. http://drupal.org/node/211182) that modules which are disabled (as opposed to uninstalled) are *are* automatically updated.
Indeed, in that issue, taxonomy module is specifically discussed. Obviously there's still something missing in getting this working...
#9
Here is a patch for @mrfelton's fix in #6.
I suspect the same issue will arise for forum and comment modules, unless these have been fixed already.
I will investigate these two modules.
#10
#11
That's exactly the path we agreed not to take. Read again what we came up with yesterday: we are not calling anything in taxonomy module. The upgrade, after the taxo patch gets in, will need a rewrite.
#12
Agreed. Thus, waiting for http://drupal.org/node/706842.
Should this issue be set up as a meta issue, then? Looks like we will have similar concerns with other modules. Or should we set the title back to the original title, and create a separate meta issue?
#13
I haven't read this issue carefully yet, but based on our conversation at the end of the CPH code sprint, I think we need to ponder some more and write down the assumptions, requirements, and process we plan to impose for site upgrades before making significant changes anywhere.
#14
There is already an issue about the general problem: #773828: Upgrade of disabled modules fails
And fixes for other individual update functions have gone in elsewhere, e.g. #788370: taxonomy_update_7002() fails when taxonomy module is disabled and #794184: contact_update_7002() fails when the contact module is not enabled. I don't know if there are a lot of others left besides this one - hopefully not.
#15
I think the task is to get this patch pass.
#16
The big question is, if we agree on #15 then do we agree on using file_save? It fires hooks but does not rely on them. Or do not try to come up with any exceptions and replace with db_insert?
#17
At least this approach needs review.
#18
The last submitted patch, update_new.patch, failed testing.
#19
Yes it does. I was wondering, do we want to load field_sql_storage for the sake of simplicity? Not the rest of field API I think.
#20
I agree on the strategy here.
We definitely want to load Field SQL Storage, it's a big pile of code that is completely independent of the rest of the field API, so it should work correctly.
#21
This approach might make sense for sites that only use core and contrib modules. But as soon as you add custom code into the mix, I think it will cause problems.
For example, I've written a lot of one-off update functions for sites that e.g. do a node_save() to add some content during the update. When I do that, I expect to get an actual complete node, not a partial one. In other words, all the modules enabled on my site need to be able to respond to its creation via whatever hooks they implement.
#22
Anyway, that kind of general proposal really belongs at #773828: Upgrade of disabled modules fails, not here. I'm restoring this to its original title.
One thing we might want to use here from that idea, though, is to disable the modules in the tests. I believe you guys wrote some really awesome D6->D7 upgrade tests, but maybe they would become even more awesome if they ran on a D6 database that had all non-required modules installed but disabled? Having those tests in place would catch bugs like this one, so it might be a good thing to introduce here.
#23
#15: update_new.patch queued for re-testing.
#24
The last submitted patch, update_new.patch, failed testing.
#25
After reading this and a few other on simular issue I still have one question left. How do we handle 3 part modules that are swollowed up by D7? These are nomally turned off before attemting an upgrade?
#26
Same old story we need to remove calls to API functions in the taxonomy upgrade path: field_attach_create_bundle(), field_create_field(), field_create_instance(), taxonomy_get_vocabularies(), node_type_get_types(), field_info_fields(), field_delete_field(), field_info_instance(), field_delete_instance().
#27
Starter patch.
It's incomplete, I'm not completely sure how to deal with field_delete_field() and field_delete_instance().
#28
The last submitted patch, 895386-taxonomy-upgrade-path.patch, failed testing.
#29
Now with the exact amount of magical fairy dust necessary.
#30
If you ever need to reroll this please change the order of
+ *
+ * @ingroup update-api-6.x-to-7.x
+ * @param $field_name
+ * The field name to delete.
+ */
to
+ * @param $field_name
+ * The field name to delete.
+ *
+ * @ingroup update-api-6.x-to-7.x
+ */
But this shoudln't stop this patch as I want D7 out :)
#31
You forgot big red flaming warnings that these functions delete field data. Do we even need that? Can't we verify the table to be empty and drop only then and not delete in the instance delete? Or rename the table perhaps as the SQL storage does? Nuking user data like that makes me cringe
#32
Added an exception to _update_7000_field_delete_field() if there is data not deleted, big red warnings to _update_7000_field_delete_instance().
#33
The last submitted patch, 895386-taxonomy-upgrade-path.patch, failed testing.
#34
Tweaked comments.
#35
Made
_update_7000_field_delete_fieldcheck the correct table, thanks tobiasb.#36
If the update helpers weren't so transient, I'd say they need tests. But since they're kind of a one-off, and we have upgrade path tests, I guess it's okay. And that was the only thing that gave me pause here. So RTBC.
#37
Looks fine here too. Upgrade path tests are plenty IMO as long as they cover those functions.
#38
Committed to CVS HEAD. Thanks.
#39
Automatically closed -- issue fixed for 2 weeks with no activity.