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.
First reported in http://drupal.org/node/141727
After further testing, it appears that the followup patch in that issue didn't fix this:
Updating a drupal 5 database fails at system update 6027. (An HTTP error 200 occurred. /update.php?id=1&op=do)
Comment | File | Size | Author |
---|---|---|---|
#15 | patch_197500_13.patch | 1.34 KB | chx |
#14 | 6027-fix_b.patch | 1.6 KB | dvessel |
#13 | 6027-fix.patch | 1.5 KB | Gábor Hojtsy |
#10 | 6027-fix.patch | 1.47 KB | Gábor Hojtsy |
#3 | fix-custom-themes.patch | 768 bytes | Gábor Hojtsy |
Comments
Comment #1
dvessel CreditAttribution: dvessel commentedsigh.. I need a database that hits system_update_6027() for the update to really get into it. I have a vague idea what's causing it but can't test.
That update function is specific to themes and how it's configured.
Comment #2
dvessel CreditAttribution: dvessel commentedwebernet, is there some way I can get the database? I just did an update from 5 and update 6027 ran without problems.
Comment #3
Gábor HojtsyHm, I also run my usual update tests (I have a Drupal 4.6 site with the removed updates copied back to system.install and one menu_rebuild() call removed) and it still runs fine with the latest and greatest Drupal HEAD.
However, looking at system_update_6027 again (I did provide this insight in another issue, so this one might be a duplicate, hm). So anyway, looking into that issue, $custom_theme is modified and not restored, while $theme is modified but restored. So the next update might run with a different theme, which might cause the error. This depends on the set of themes in your blocks table, so it is not evident to reproduce with any data set.
With remembering the old custom theme and restoring at the end, it should work for all kinds of databases as far as I see.
Simple patch attached for your testing.
Comment #4
webernet CreditAttribution: webernet commentedAfter adding some drupal_set_message() calls, I found that it's failing when _block_rehash() is called for my custom theme (that was already ported to 6.x), whether it's enabled/default or not.
Comment #5
webernet CreditAttribution: webernet commentedCross posted with Gabor - Patch looks valid, but it's not the root issue.
I've traced the bug back to init_theme()...
Comment #6
Gábor HojtsyHm, looking at _block_rehash() the theme_init() call looked suspicious, but it should return if $theme is already set. Unfortunately system_update_6027 does set $theme to NULL, so it triggers lots of nasty stuff in theme_init(). Some notes around this code through:
- the update code takes great effort to set $theme and $custom_theme but none of these is used in _block_rehash...
- $theme_key is used however, which is not dealt with in the update code, so the successive _block_rehash() calls are doing the same thing multiple times (?)
- because _block_rehash() uses drupal_save_record(), it is only by pure luck that hell does not break loose... if the database schema would be different in the blocks table at the time _block_rehash() is called, then what it is in system_schema() we get glaring big error messages here...
All-in all it would be great to know why do we need those _block_rehash() calls at all, and eliminate it if at all possible. Fiddling with $theme and $custom_theme should also be removed as it seems as that is completely pointless.
Comment #7
webernet CreditAttribution: webernet commentedReminder - This issue only began appearing after the maintenance theme patch was committed.
Comment #8
Gábor HojtsyCalling any function which uses the Drupal 6 final schema while being in an uncertain schema state (which is most probably not the final Drupal 6 schema) is very bad. I even wrote a note to the devel mailing list about this. So instead of micro-managing the problem down in some deeper function call, we should eliminate the _block_rehash() call as it is.
Comment #9
Gábor HojtsyLooking at _block_rehas() and what system_update_6027() needs, we need something like (in system_update_6027):
We only need to set the cache fields properly, instead of all the theme mangling and bogus API calls. The above code is totally untested, written in a firefox textarea :)
Comment #10
Gábor HojtsyStill untested, but in patch form.
Comment #11
webernet CreditAttribution: webernet commentedThe update did complete, but I'm getting: notice: Undefined index: cache in /modules/system/system.install on line 2470.
Comment #12
dvessel CreditAttribution: dvessel commentedNice looking patch. :D
Isn't the block caching new in 6? So, I'm guessing the column isn't there since your updating from 5.
_block_rehash() took care of this with drupal_write_record() by providing missing values with defaults it seems.
Comment #13
Gábor HojtsyThe notice is from blocks which do not define the cache key. That's easy to fix. :) This exact update adds the cache column.
It would be good to think about whether we would miss anything from _block_rehash(), which we are not duplicating here?
Comment #14
dvessel CreditAttribution: dvessel commentedTested with Gábor's patch and got the same errors. This patch adds drupal_write_record() and the notices are gone.
Comment #15
chx CreditAttribution: chx commentedComment #16
Gábor HojtsyDo not use schema version dependent functions such as drupal_write_record() in update functions! Read the note above and the note on the devel list.
Let's talk about the patch in #13.
Comment #17
dvessel CreditAttribution: dvessel commentedWhoops, I tested patch from #10 and got the notices. Haven't checked #13. Not sure what changed.
[edit: doh, will test #13]
Comment #18
chx CreditAttribution: chx commentedGabor's patch is wrong, the default is 1 , set as database default so my patch only sets cache if there is something to set.
Comment #19
Gábor HojtsyIndeed, my wrong. Let's get #15 tested then.
Comment #20
webernet CreditAttribution: webernet commentedTested #15 -- Update completes with no errors.
Comment #21
dvessel CreditAttribution: dvessel commentedSame here, tested #15 without problems.
Comment #22
Gábor HojtsySince _block_rehash() will run soon on a live site anyway, and we only need to touch the cache field here, I committed this one, thanks.
Comment #23
(not verified) CreditAttribution: commentedAutomatically closed -- issue fixed for two weeks with no activity.