When attempting to upgrade d6->d7 the following series of events occurs

- Drupal bootstraps to DRUPAL_BOOTSTRAP_VARIABLES
- This calls _drupal_bootstrap_variable(), calling variable_initialize($conf)
- This attempts to acquire a lock on 'variable_init'
- This fails because the semaphore table does not exist
- The exception handler then calls lock_may_be_available()
- This ALSO fails, because the semaphore table does not exist
- This results in an uncaught exception, which ends page execution
- Then on shutdown, lock_release_all() is called, since it is registered as a shutdown function in _lock_id() which is called in lock_acquire()
- Guess what happens then?

Marking as critical since it is part of the upgrade path, and tagging. Not sure of the best solution, this whole area is pretty new to me.

CommentFileSizeAuthor
#23 update-followup-864464-23.patch3.78 KBDavid_Rothstein
#21 864464_old_d6_upgrade_please_commit_me_webchick_pleeeeeeeeease.patch4.28 KBgdd
#18 864464_old_d6_upgrade_please_commit_me_webchick_pleeeeeeeeease.patch5.45 KBgdd
#17 864464_old_d6_upgrade_boom_sad_panda.patch4.28 KBBeanjammin
#8 864464_old_d6_upgrade_boom_sad_panda.patch4.09 KBgdd
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented

Crap I just realized that I was upgrading a 6.12 site (my personal site) which is why this whole thing is falling apart. That said, it might be worthwhile to add a check or warning somewhere, to check a minimum 6.x version from which you can upgrade. I don't know where this belongs though.

Gonna upgrade my personal site and test this again.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Priority: Critical » Major

Downgrading.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
Title: locking system breaks d6->d7 upgrade path » Upgrade path broken if not run from at least Drupal 6.16

I was just looking into this and I'm not sure how to proceed. I tried adding a check in update_prepare_d7_bootstrap() and exiting before the drupal_bootstrap(DRUPAL_VARIABLES) if you weren't running a new enough version, but unfortunately this always falls through to a drupal_bootstrap(DRUPAL_BOOTSTRAP_SESSION) which then causes the exact same problem.

Another option is to check for the semaphore table in update_prepare_d7_bootstrap(), and if it doesn't exist just create it. We do this for some other tables that are required before bootstrap can take place.

I realize this is only for people not following direction (since the directions specifically state to upgrade to the newest D6 before attempting a D6->7 upgrade) but it would be nice if it was handled gracefully.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

If I'm not mistaken, http://api.drupal.org/api/function/update_prepare_d7_bootstrap/7 gives us a chance to fix these before we get to DRUPAL_BOOTSRAP_VARIABLES. That's where we're adding the registry and cache_bootstrap tables already.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented

If everyone feels that the best thing to do is to test for the table and just make it if it doesn't exist, the yes that is definitely the place to do so. This is certainly easiest, I just don't know how people feel about testing for and possibly creating a table that really by all rights should exist by anyone upgrading anyways.

What you can't do is requirement checking here, because after update_prepare_d7_bootstrap() exits, update.php runs a full bootstrap and everything falls apart before the requirements errors get displayed.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented

So in thinking about this I actually think we need to add the semaphore in table update_prepare_d7_bootstrap() and also add a requirements check for which D6 version the person is running. The current experience is terrible and it seems to me like this is a situation people will unwittingly encounter often.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
Status: Active » Needs review
FileSize
864464_old_d6_upgrade_boom_sad_panda.patch4.09 KB

So attached is a shot at a patch to fix this. It does the following.

- We can't actually check the D6 version that was running from the D7 code base (so far as I can tell) so the next best thing I found is checking the schema version of system.module. I added a constant REQUIRED_D6_SCHEMA_VERSION for the latest one. This will have to be chased if D6 gets another rev before launch. Not a great solution, other options more than welcome.

- Adds the semaphore table if it didn't already exist.

- Adds a requirements check for your Drupal 6 version against the previously defined constant, and creates a REQUIREMENTS_ERROR if you don't match it.

- Fixes a bug in update_fix_d7_requirements() that was supposed to drop and recreate the 'value' index on the semaphore table, but instead dropped the 'expires' index and attempted to re-add the 'value' one.

Now, will I get the patch attached and submitted before my battery runs out?

Status: Needs review » Needs work
Issue tags: -D7 upgrade path

The last submitted patch, 864464_old_d6_upgrade_boom_sad_panda.patch, failed testing.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
Status: Needs work » Needs review
Issue tags: +D7 upgrade path

#8: 864464_old_d6_upgrade_boom_sad_panda.patch queued for re-testing.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

We simply need to require that sites are on latest version of D6. Thats the only one that gets tested. So creating a semaphore table is out of scope IMO.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented

If we don't create the semaphore table we can't bootstrap. If we can't bootstrap we can't actually tell people that they have not met the requirements. This is the problem.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community

OK, makes sense.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Reviewed & tested by the community » Needs work 
+++ includes/update.inc	27 Jul 2010 00:57:56 -0000
@@ -148,6 +163,40 @@ function update_prepare_d7_bootstrap() {
+  if (!db_table_exists('semaphore')) {

If we already require the latest D6, why do we try to create {semaphore} here? If this is required for some odd reason, then it badly needs an inline code comment.

Powered by Dreditor.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented

As stated in comment #12, the problem is that without semaphore we can't even report that there is a problem because we can't bootstrap and you just get a WSOD. If simply giving a WSOD to anyone who tries to upgrade from 6.15 or earlier is acceptable then this issue can be marked wont fix but I think that is kind of crappy personally.

I'm happy to add comments to make that bit clearer in the code.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Hm, I can't really help to provide direction on that question, other than that I thought we would not support upgrading from earlier minor releases anyway...? In that case, I'd simply throw an exception or error message that tells the admin to update to the latest D6 release first. I guess that would be far easier than trying to re-implement an upgrade path from earlier releases, and additionally, it can quickly be tweaked for more recent D6 releases.

However, if we really want to do this, then we'd basically have to put your reasoning in #15 into an inline comment right above that db_table_exists() condition. Not supporting that and throwing an error sounds reasonable to me though.

[Basically, instead of doing this, we could have left the D6 updates in core .install files in place... but they have been removed for some reason that is not clear to me.]

Beanjammin’s picture

Beanjammin CreditAttribution: Beanjammin commented
Status: Needs work » Needs review
FileSize
864464_old_d6_upgrade_boom_sad_panda.patch4.28 KB

Added comments as per #14/15.

BTW, the patch worked for me when attempting to upgrade from 6.13 to 7.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
FileSize
864464_old_d6_upgrade_please_commit_me_webchick_pleeeeeeeeease.patch5.45 KB

This is great! Thank you! I have attached a very minor coding style change because the comments were more than 80 columns long but otherwise this is ready to go.

Beanjammin’s picture

Beanjammin CreditAttribution: Beanjammin commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work 
+/**
+ * Older versions of Drupal 6 do not include the semaphore table, which is 
+ * required to bootstrap, so we add it now so that we can bootstrap and provide
+ * a reasonable error message.
+ */

These should be inline comments, not PHPdoc style.

 files[] = tests/upgrade/upgrade.test
+<<<<<<< simpletest.info
+files[] = tests/upgrade/upgrade.taxonomy.test
+=======
 files[] = tests/upgrade/upgrade.poll.test
+>>>>>>> 1.19

We don't want your steeeenking conflicts. :)

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
Status: Needs work » Needs review
FileSize
864464_old_d6_upgrade_please_commit_me_webchick_pleeeeeeeeease.patch4.28 KB

Rerolled!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Fixed

Ok, great. Committed to HEAD! Great work!

However, just before I committed this I noticed the indentation of those comments is off by 1. :P So I committed a follow-up that fixed that. Hope the testbot survives. :P

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Status: Fixed » Needs review
FileSize
update-followup-864464-23.patch3.78 KB

Nice fix, but I think we'd prefer not to say "You are running a current version of Drupal 6" to users of Drupal SEVEN every time they visit update.php :)

We should also define the new constant in update.inc rather than update.php so it is reusable by other scripts.

Attached patch solves those issues.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
CreditAttribution: gdd commented
Status: Needs review » Reviewed & tested by the community

Yes to both, looks good.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Oops. :P Nice catch.

Committed to HEAD!

EvanDonovan’s picture

EvanDonovan CreditAttribution: EvanDonovan commented

Since the latest version of Drupal 6 will be required for an upgrade, can a note to that effect be added to UPGRADE.txt? Patch waiting a final review here: #929188: UPGRADE.txt - Instructions to update to latest release of current version before upgrading to next major version.

Status: Fixed » Closed (fixed)
Issue tags: -D7 upgrade path

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +D7 upgrade path

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.