System update 6027 fails

sigh.. 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.

webernet, is there some way I can get the database? I just did an update from 5 and update 6027 ran without problems.

Update #6027
ALTER TABLE {blocks} ADD `cache` TINYINT NOT NULL DEFAULT 1

Hm, 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.

Hm, 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.

Calling 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.

Looking at _block_rehas() and what system_update_6027() needs, we need something like (in system_update_6027):

  foreach (module_list() as $module) {
    if ($module_blocks = module_invoke($module, 'block', 'list')) {
      foreach ($module_blocks as $delta => $block) {
        db_query("UPDATE {blocks} SET cache = %d WHERE module = '%s' AND delta = %d", $block['cache'], $module, $delta);
      }
    }
  }

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 :)

Still untested, but in patch form.

Nice 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.

The 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?

Tested with Gábor's patch and got the same errors. This patch adds drupal_write_record() and the notices are gone.

Do 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.

Whoops, I tested patch from #10 and got the notices. Haven't checked #13. Not sure what changed.

[edit: doh, will test #13]

Gabor's patch is wrong, the default is 1 , set as database default so my patch only sets cache if there is something to set.

Indeed, my wrong. Let's get #15 tested then.

Same here, tested #15 without problems.

Since _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.