Upgrade from 4.7 to 5.0 fails - node_type already exists

Nice catch.
As a wrote in a comment to your other 'update issue' http://drupal.org/node/100742, I think there's a way to specify "branched update functions", but I'm still trying to figure it out...

As a (semi-)related (and minor) issue : content_update_9 drops the node_type_content table but the table is still created in the install function, so the table is created on fresh 5.0 installs. I'm waiting to know how to number the update function to roll another content.install without this table. Any hint welcome...

The problem is that the renaming *is* done in the 4.7 branch, if the user keeps their database up to date, but if they don't get the 4.7 branch current before updating to 5.0 it creates a big mess, as you reported. I've been out of town and unable to work on CCK issues, but I'm back and also trying to figure out how you rewrite the install file so it works correctly both for users that have already done the update in 4.7 and for those who haven't, and it's not immediately clear how to make it work.

BTW, in another thread there was a concern about dropping the old table, but dropping it is consistent with the way previous CCK database changes have been done (create a new table, copy info to it, drop the old table). The problem, of course, is that we have not fixed all possible probelms that can keep the update from working correctly.

Yched, if you have something started as a patch for the install file, why don't you post whatever you have and we can collaborate on how to get it working correctly.

There is actually one other related, but not critical, problem. We are unnecessarily creating and then dropping a table for fresh installs. Why create the table at all.

How about removing the node_type_content from the install function and creating it in update_1() instead? Then add to the install function a check for the current update level, and if it is not set (a fresh install) set it to 9 so none of the updates are needed. That way fresh installs will work correctly with no need to update anything.

Then we need to eliminate update_8 since we don't want to rename any tables in the 5.0 branch.

Attached is a patch that I hope will work correctly for new installs, updates from tables that are current, and updates from tables that are not current. Feedback appreciated.

That patch is not quite right. If someone comes to this point without having done update_8, we want to skip renaming the table, but we also want to skip update_9, so we need to update the schema version to 9 in update_8. The question is, will updating the system table at that point keep update_9 from running or will the system already have determined that it is needed and run it anyway? In other words, how could we selectively keep update_9 from running?

Or maybe this is the way to do it. I moved the creation of the node_content_type table out of the install and into update_1 as before, so it is skipped on fresh installs, then on update_8, instead of skipping the step, I left the rename in there, then copied in the system_update_1005() that would have failed so it gets run again after the tables were renamed. Then update_9 should be OK for everyone after that, since the tables have been corrected.

Still would be good to add more notification messages once we have something that is working correctly. The question is, does this handle all the database situations correctly? What if someone was back at a really early update level, like 4 or 5??

I have little time for a proper review right now, and I'm still having a hard time figuring these update issues correctly, but I think we should seriously consider using the core convention of separating updates for branches (1-999 for 4.7, 1000-2000 for 5...). Drewish does that for audio module, for instance.

It should allow us to add subsequent updates to 4.7 branches, and to include them in the 5 branch as well (for handling 4.7 to 5 migration) without the update number being already taken by a 'strictly 5' branch update. Dunno if i'm being clear...

Well, having said that, I'm not helping with the table renaming / dropping issue...

We can use the 1000 series for 5.0 updates, but that doesn't really change anything about the way this is going to work. The problem is that if someone has not done update_8 before the update to Drupal 5, they will get the node_type already exists error and the new node_type database with its new fields will never get created. So we need to rerun that upate after renaming the table and before running the final update that copies that info back to the table that the node module created. We also still IMO need to get the creation of the extra table out of the install function so it is never created at all on a fresh install by moving it into update_1.

I do see a couple of cleanup items. I put the version check for a fresh install in two places instead of just in the one place it is needed (the install function). Also, instead of copying system_update_1005() It seems it could be done by just executing that function (can one update function call another one?? Apparently so) Then we also need to update the system table to indicate that the node module has been updated to that level.

The one remaining problem is that anyone who has not run update_8 before updating to 5.0 is going to get the 'node_type already exists' error message since the node module updates are going to run before the content module updates since core updates get executed first. With the patch, we go ahead and fix the problem, so maybe we just need another message saying something like 'The failure to create the node_type table has been corrected and the table is now properly updated for 5.0'.

To properly test this, we need a database that has not had update_8 run on it, and I haven't quite figured out how to create a non-current database to use for testing, so if anyone has a database that has not had the node_type table name changed yet and can take a few minutes to make a test copy and try to perform a 4.7 to 5.0 update, it would be helpful. In the meantime, I'll try to cobble together a non-current database to test.

Karen : I don't understand this part in hook_install :

$version = db_results(db_query("SELECT schema_version FROM {system} WHERE name = 'content'"));
if ($version < 2) {
  db_query("UPDATE {system} set schema_version = 1001 WHERE name = 'content'");
}

This will only get executed on fresh (5.0) installs,
For a fresh install, the install system runs _hook_install, and sets the schema version to the highest available update function (1001 here), without executing any. So it seems this code does what install system will do anyway.
Side note : there's probably a typo, function db_results does not exist.

Other thing :
Removing node_type_content from hook_install is the right thing to do of course.
But I don't get why we should stick it in a content_update_1. It will only be executed for people out there (if any ?) with schema version = 0 (that's march 2006). I don't understand why we'd want to create a node_type_content table for those people specifically. Plus after update_1, they will come to an error when _update_8 is run, and tries to rename to an existing table.

Please forgive me if it's just me plain not getting it (which is highly possible) - and if so, don't feel obliged to spend too much time explaining :-)

Hem, sorry about that, [pre] tags don't like < chars in code

Karen : I don't understand this part in hook_install :

$version = db_results(db_query("SELECT schema_version FROM {system} WHERE name = 'content'"));
if ($version < 2) {
  db_query("UPDATE {system} set schema_version = 1001 WHERE name = 'content'");
}

This will only get executed on fresh (5.0) installs,
For a fresh install, the install system runs _hook_install, and sets the schema version to the highest available update function (1001 here), without executing any. So it seems this code does what install system will do anyway.
Side note : there's probably a typo, function db_results does not exist.

Other thing :
Removing node_type_content from hook_install is the right thing to do of course.
But I don't get why we should stick it in a content_update_1. It will only be executed for people out there (if any ?) with schema version = 0 (that's march 2006). I don't understand why we'd want to create a node_type_content table for those people specifically. Plus after update_1, they will come to an error when _update_8 is run, and tries to rename to an existing table.

Please forgive me if it's just me plain not getting it (which is highly possible) - and if so, don't feel obliged to spend too much time explaining :-)

Other thing :
running system_update_1005() again might be our only way out, here, since, as you pointed out, system.install updates will be run before we have a chance to do anything.
I'm not sure about the part where you manually update the schema version to 1005, though :

Even if system_update_1005 fails the first time, I the following 1006, 1007 etc still get executed, and the schema version gets set to a higher number (it's currently 1016). So setting it back to 1005 means that on next update, 1006 to 1016 get run again, and I'm not sure they're supposed to.

Plus even if anything should be done, it's on 'system' module, not 'node' :-)

OK, after talking a lot, I finally gave your patch a try with an 'outdated' (pre content_update_8) 4.7 db.
I manually "built" this outdated db simply by
- taking my current 4.7 test install db
- renaming back the table 'node_content_type' to 'node_type' (revert content_update_8)
- setting content.module schema version to 7
- dropping this db in place of my 5.0 test install db
- running update.php on my 5.0 test install

There were two typos in content_update_1000 that prevented it from working.
The attached patch corrects this, plus removes the manual "set schema version to 1005" (see my previous comment)

Result (with this updated patch) :
- First run, I'm proposed only with system updates, starting from 1000 - OK
- As expected, update 1005 fails, 1006 to 1016 get executed - OK (system schema version is now 1016)
- Going back to update.php, I'm proposed with 'other modules' updates, starting from 8 for CCK - OK
- update 8 renames node_type to node_content_type - OK
- update 1000 creates the "new" node_type table - OK
- update 1001 moves my content types from node_content_type to node_type
Browsing through my site after that, everything seems OK...

Unless my "outdated 4.7 db building" process is wrong, pre-update_8 migration looks OK.
So, apart from the points I mentioned in comment #10, which I still don't get, this seems good to go :-)
We might still want to merge drewish's patch in http://drupal.org/node/100742.

I also finally got an un-updated database configured and ran the update and it seems to work.

I agree with your changes. You're right about resetting the schema version in both places I tried to do it. The first one is unnecessary and the second one was my confusion (I was thinking the individual core modules were tracking their own schema version number). As for putting the database creation script into update_1, it's mostly for documentation, otherwise there is nothing to indicate what this database is supposed to look like and you just have a bunch of updates with no indication what database configuration they originated from. I am not sure if it's the right thing to do or not, I've just never seen any install files that don't have the original database creation script in them somewhere.

The final problem I had is that I can't figure out a way to get a message to display at the end to tell the user to ignore all those error messages because the database has been fixed. I tried drupal_set_message, but it didn't seem to trigger. I don't think there's any way to surpress the error messages, so I was hoping to find a way to tell the user not to worry about them so they don't go and mess things up by trying to manually alter the database, thinking that the process failed. Can you see a way to get a message like that working?

If we can get a "don't worry about the errors" message in here, and decide what to do about the database creation script in update_1, I think the patch is RTBC.

About the message : Yes, that's a problem.
Obviously we'd have to stick a big red something on the project page and in the README (well, not red in the readme...)
As for what the user actually sees when he runs the updates, I have an ugly idea : if you add a simple drupal_set_message('hello'); at the end of content.install, it shows up on every page in the update process.
Maybe if we enclose it in an appropriate conditional (something like if content scheme version < 8), we could manage something ?

About content_update_1 for documenting : ah, yes, why not. To me, database schemes live in hook_install. When they change, old versions are gone (they live in cvs...). I'd say the opposite : I've never seen a module that kept its old db schemes in its install files - but maybe I'm not looking close enough.
Besides, _update_1 happened to be available (dunno why), but what if it had not ? And where will we put the current scheme if there is another change ?
But, as I said, why not ?

< got me again.... I was saying :

Maybe if we enclose it in an appropriate conditional (something like if (content scheme version < 8)) we could manage something ?

About _update_1 for documentation : ah, yes, why not. To me, db schemes live in the _hook_install. When they change, the old scheme is gone, and lives in cvs. I'd tend to say the opposite : I've never seen a module that keeps a history of its db schemes in its install file - but maybe it's me not looking close enough.
Besides, it happens _update_1 was available (dunno why), but what if it had not been ? And where will we store the current scheme if we change to a new one in the future ?
But, as I said, why not...

Since we changed _update numbers to the 1000 series, my up-to-date 5.0 install wants me to run updates 1000 and 1001.
So we should also wrap _update 1001 inside a if (db_table_exists('node_type_content')).

I'm working a an update of this patch that adds in an error message and incorporates the SQL escaping discussed at http://drupal.org/node/100742. I hope to have this ready to post (and commit) soon.

I'm also going to add an UPGRADE.txt file to the module. I can't include a non-existant file in a patch, but here's what I propose for both the UPGRADE.txt field and to be added to the handbook as a new 'Upgrade' page. Feedback is welcome.

Updating the content module from 4.7 to 5.x

The 4.7 version of the content module created a table called node_type to track 
the custom node types that were created in CCK. In Drupal 5.x, that table is 
created and maintained as a core file. The most recent 4.7 versions include an 
update to rename the content module version of this table so that the upgrade to
5.x will be able to create it's own version of that table. 

If you are running the the most recent 4.7 content module version and have performed
all possible updates in 4.7, you should have no trouble updating to 5.0. However, if 
your version of 4.7 is not up to date, you may get error messages when you update 
to 5.x when the core installation tries to create that table. 

There are two ways to handle the update if you are not running the latest 4.7 version:

OPTION #1

Before updating your site to Drupal 5.x, download the latest version of the 
4.7 content module and upload it to your site, then go to update.php and run all 
required updates. Check that the updates worked correctly, then go ahead and upgrade 
to 5.0 as explained in the UPGRADE.txt file in the 5.0 installation files. If you do this, 
you should see no errors related to the node_type table when you do the update.


OPTION #2

Go ahead and update to 5.0 without first updating the 4.7 version. This will
save you the time of downloading and updating your 4.7 version, but if you do it
this way you may see errors when you do the 5.x update that say that the node_type
table already exists. You can ignore those errors because the content module update,
which runs after those errors are generated, will update your tables and rerun
the failed core update to create the table properly.

And here's my new patch. Hopefully I haven't missed anything. I added db_escape_string() to the update_sql to handle the escaping problem while still displaying the query results on the update screen. For the message, I set a session value if the core update needed to be rerun, then added drupal_set_message() at the bottom of the install file to display only if the session value was set.

Oh yeah, and also I dropped the creation script for the original node_type table completely. As yched noted, if you create the table as node_type_content, the later update will fail, so the right way to create it is as it was originally created, as node_type, but that won't work in a 5.0 installation since by that time the table has already been created. So I just dropped it out.

About the error message : I think it would be best if the user could see the message _before_ he runs the system updates.
If he freaks out seeing the error on system_1005 , he might not even get to the "updete module" part where we tell him "don' t worry, the error was normal and has been dealt with" - or he might get there but after trying strange things with his db in between.
I'm aware this might be trickier - I'll try to work on this later on today if you want.

About the UPDATE message ; good idea but IMO the text you propose steps into detailed technical considerations that will turn most readers away.
Given what the patch does, I think it can be much simpler - something like

If you upgrade from a 4.7 install that does not have the most recent 4.7 version of CCK, you might encounter SQL errors during the core system upgrading process.
If so, please proceed, these errors will get corrected during the CCK upgrade phase

And maybe leave the detailed explanation after that, for savvy / interested readers ?

Er, and thank you very much for your work on this not nice issue :-)

I agree it would be nice to set the message before the updates are run, but I'm not sure if it's possible. I don't think our file gets included before the core updates run (but I might be wrong about that). Sure, go ahead and see if you can figure it out.

I suppose you're right about the update.txt. We can lead with your line, then something like 'Details below:'. I always like to see the details. Sometimes I forget that others don't want to know that much :-)

Also, I'm working on updating the CCK handbook documentation, but I'm not going to add the update info until we get this patch committed, so I hope we can get it done pretty quickly.

User messages : well, you were right obviously, that's not so simple indeed.
Problem is, as you pointed out, that in some cases, the content.install won't even be loaded during system 4.7 -> 5.0 migration :
- if the cck files are not present in the to-be-upgraded site (as is recommended in the upgrade handbook : move your drupal folder to a backup, create a new one with 5.0 core files, run update.php)
- or if they are but present but not in the same place where they used to be in 4.7 (often modules/, whereas the recommended location for contrib in 5.0 is sites/all/modules/)

For those cases, the first run of update.php _will_ trigger (a lot of) worrying error messages, and there's nothing we can do about it. All we can do is display relaxing messages in a subsequent run of update.php, that will get run when the user updates contrib modules.

The attached patch is the best I could come up with to deal with the two scenarios :
- content updates are run in the same update as system updates : OK we can warn about the errors before they happen
- content updates are run afterwards : all we can say is "Er, you probably had errors before, never mind, we're going to correct this now".

I hate to say this, but I don't think this is satisfying... I'm beginning to think that we should patch system_update_1005 to not issue the errors in the first place. This is hardly an acceptable 5.0 migration experience, it will freak people out and ruin their first impression of the whole 5.0 (OK, it will only affect users with outdated CCK, but still...)

I'm ready to propose a core patch to this effect if you agree. I'd like to have your opinion first, because this would probably mean ditching most of the work we (er, you...) put on this issue. Sorry if I come with this that late, it's my progressive understanding of the issue and our elbowroom on it.

And the patch...

I am in agreement about trying to patch the system update, I'm just not very sure how likely it is that our patch will be accepted. The core maintainers don't like to put anything into core updates that relates to contributed modules and we're almost ready for a RC so they'll be less likely to add changes. However this is a case where you could say 'You broke it, you fix it' so maybe it would be accepted. I was thinking of just asking them to check if that table exists before running the update, and exiting, then our update would go ahead and finish things, but that won't handle a situation where the content module is not enabled at the time of the update (like if someone tried it out long enough to create a table, then quit using it). So maybe the proposal for the core update would be to check if the table exists before trying to create it. If so, rename it to node_type_content, then go ahead with the regular update. That would I think handle all situations -- an installed cck with a non-current update, a previously-installed cck that is no longer installed, etc etc.

At any rate, I don't think it changes what we do in this patch which should be OK either way.

Well we would need one change, wrap update_8 with if (!db_table_exists) so if the system starts renaming the tables our update doesn't do it again. Then wrap your error message with the same thing (if the table has been renamed, there won't be any errors).

This way we can commit this patch so things start working better until, hopefully, a core patch is committed, and thereafter it should still be OK.

Karen : Exactly - this is basically putting content_update_8 inside system_update_1005. I tried this, it works flawlessly. We'd just have to add an additional check in our own content_update_8 to check if the rename has been made by system before. content_update_1000 becomes useless, too.

I'm aware that getting this in core is "borderline" - I guess it's a question of whether they want a clean upgrade path or not. The argument is there's not a thing we in cck can do about it...

I have to leave right now, I'll submit a core patch tonight (or go ahead if you want to move faster :-) )

Oops, looks like we're posting concurrently.

In my tests, the core update would remove the need for warning messages cause there should be no errors at all - but I may have forgotten something ?

I submitted a core patch at http://drupal.org/node/101454.

I committed this patch to HEAD and 5.x versions.

I left in the warning messages because even if the core patch gets committed, we have Beta-1 and Beta-2 out there that are missing that patch.

I just committed a slight fix in content_update_1001 (node_content_type was deleted one time too many) - it seems this was just in 5, not in HEAD

And thanks for the core patch, BTW, it seems it will get in :-)

If you run into this problem, there is a simple solution:

First identify the symptoms: if mysql table 'node_type' exists and if 'node_type_content' also exists in the old 4.7 mysql database, try this:

Run the upgrade script for 5.x as normal.

Then note the error SQL queries that did not work. In my case it was:

CREATE TABLE {node_type} ( type varchar(32) NOT NULL, name varchar(255) NOT NULL, module varchar(255) NOT NULL, description mediumtext NOT NULL, help mediumtext NOT NULL, has_title tinyint unsigned NOT NULL, title_label varchar(255) NOT NULL default '', has_body tinyint unsigned NOT NULL, body_label varchar(255) NOT NULL default '', min_word_count smallint unsigned NOT NULL, custom tinyint NOT NULL DEFAULT '0', modified tinyint NOT NULL DEFAULT '0', locked tinyint NOT NULL DEFAULT '0', orig_type varchar(255) NOT NULL default '', PRIMARY KEY (type) ) /*!40100 DEFAULT CHARACTER SET utf8 */;

Copy the query, go into myPhpAdmin and:

1. drop the 'node_type' table (It may have old CCK node types in it. You can confirm this by viewing the names of the CCK types having the "-" instead of the "_" in the names.)

2. Go to the myPhpAdmin "SQL" tab and enter this SQL command (you have to get rid of the drupalized '{node_type}' and change to simply 'node_type'):

[code]
CREATE TABLE node_type ( type varchar(32) NOT NULL, name varchar(255) NOT NULL, module varchar(255) NOT NULL, description mediumtext NOT NULL, help mediumtext NOT NULL, has_title tinyint unsigned NOT NULL, title_label varchar(255) NOT NULL default '', has_body tinyint unsigned NOT NULL, body_label varchar(255) NOT NULL default '', min_word_count smallint unsigned NOT NULL, custom tinyint NOT NULL DEFAULT '0', modified tinyint NOT NULL DEFAULT '0', locked tinyint NOT NULL DEFAULT '0', orig_type varchar(255) NOT NULL default '', PRIMARY KEY (type) ) /*!40100 DEFAULT CHARACTER SET utf8 */;
[/code]

All errors disappear. Wonderful!

joeh

Hey

I had a similar error that caused drupal to not generate all of my content tables in content_update_1003(), this function was calling $types = content_types() which referred to _content_type_info() to obtain a list of content types - except for some reason my 4.x contained a broken list of cached data (even tho the site was functioning 100% fine) , which caused the content_update_1003() to not be aware of all the content tables that needed updating.

It turned out it is absolutely critical for this cache to be cleared in two places (1) after you have turned off all your modules and are about to run update 1000 of system.module, and (2) after you have run all the updates for system.module but before you have turned on any of your contrib modules (such as CCK)

My recommendation is that content_update_1003() should be flushing this cache prior to content_types() call, and/or content_types() should accept a bool flag to tell content_types() to flush the cache.

otherwise you can just do

delete from cache_content;

hope this helps, have bumped the status of this bug because i think its critical that the update for content.module does not rely on cached info.

Actually, cache_content wont exist in 4.7 so this needs to be run at the 5.x stage