update.php does not currently work for D6 for a variety of reasons. This issue is for fixing it. I may not have time to drive this issue to completion but I have other patches that depend on it being fixed (such as http://drupal.org/node/156741, fixing system_update_6019()) so I am motivated to try. If there is another general update.php for D6 issue already, please point me to it.

http://drupal.org/node/152383 (fixing an INSERT INTO {batch} statement) is involved here.

For the record, here are some notes that hunmonk wrote up:

the menu index changes in 6019: can't remember why, but these were causing errors. i think it's simply best to remove them, because the menu table is no longer being used, anyways.

the files update in 6022: the current queries bomb in postgres. i did some research and it appears that we can't use that kind of construct there. what's there now was a first crack at another approach, untested. i think we can probably find a more efficient way to get it done.

also, very important, although i didn't address it in these patches: system_update_1022() and system_update_6004() do the same thing. the mechanism that tests to see if 1022 was already ran before running 6004 is broken (basically, if you installed D5 after 1022 landed, the D6 upgrade still thinks 6004 needs to be run). there will need to be a restructuring of how that's handled, i think, the approach being used has caused problems before.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bjaspan’s picture

Status: Active » Needs work

The attached patch fixes a number of problems with update.php. So far I have only tested it with pgsql, and there are still some known problems. There are both too many fixed problems and too many known problems, many closely related, to create separate issues for them; they all deal with D5 to D6 update.

Known issues after this patch:

- schema.module reports that the sequences table is missing (on pgsql). This table never existed in pgsql b/c we never needed it, but it is in the system schema. We should either remove it from the schema and drop the table on mysql or leave it in the schema and create it on pgsql. I can see good arguments for both. The argument for keeping/adding it, namely that contrib modules may depend on it, would be stronger if db_next_id() was still available.

- schema.module reports that menu_custom table is missing. What is up with this table?

- schema.module reports that the menu table is extra (in db, not in schema). Is this table obsolete? If so, we should drop it.

- schema.module reports that the watchdog table is EXTRA!! (in db, not in schema). This is because the table is now owned by the dblog module which is not automatically enabled when upgrading from D5 to D6. I'd say we should enable this module automatically, otherwise everyone's logging will unexpectedly stop on upgrade. One gotcha: We can't "install" the module because its table already exists! Minor trickiness required.

- The system_update_1022() and system_update_6004() conflict hunmonk mentioned still exists.

Bugs fixed in this patch:

- update.php: E_ALL compliance for db_add_column.

- update.php: Added update_fix_d6_requirements(), always run, which adds cache*.serialized, system.info, and system.owner columns. Without these, update.php throws many errors and in fact cannot operate correctly.

- database.*.inc: Updated db_change_field() documentation to mention more strongly that keys and indices must be dropped and recreated manually (see below).

- database.pgsql.inc: E_ALL compliance.

- form.inc: batch.token has no default value. Don't depend on broken MySQL behavior.

The rest of the fixes are all in system.install. In several cases I rewrote pre-schema API system_update_N() functions to use the API because they were broken and it was easier to fix them that way.

- 6001: term_node.vid is specified as unsigned.

- 6002: Re-create primary key after using db_change_field.

- 6008: Rewrite based on what update_fix_d6_requirements() handles in update.php.

- 6009: Correctly handle adding watchdog.variables as a not-null, no default column.

- 6012: Now a no-op due to update_fix_d6_requirements().

- 6019: Use db_change_field() instead of the unsafe db_update_field(). Remember to drop and re-create keys and indices for changed fields. Also remove some incorrect changes to menu and user tables.

- 6022: Fix the SQL syntax to work on pgsql.

- 6023: db_change_field() fixes.

- 6024: db_change_field() fixes.

bjaspan’s picture

Oops, here's the patch.

bjaspan’s picture

New patch attached. All known issues listed in my previous comment are fixed. This patch results in an error-free run of update.php and a clean schema comparison report once the update is complete (except for menu, see below). Additional since the last patch, all system updates:

- 6004: Don't add {users}_created index, it already exists. See comment.

- 6005: Resolve historical conflict with url_alias_dst_idx and url_alias_dst_key. See comment.

- 6026: Set schema version in system table for update.module, otherwise errors occur when the modules form is next submitted.

- 6027: New update. Enable the dblog module so that watchdog continues to work.

- 6028: New update. Create the sequences table for pgsql. Never used, but necessary for consistency with mysql.

The only remaining schema error on pgsql is for the menu module. The menu upgrade patch is under development at http://drupal.org/node/147657.

This patch is still CNW because I haven't tested it with mysql yet.

jakeg’s picture

I think I have the same issue. Using latest HEAD (#73224), when going to install.php and clicking the 'Update' submit button, I get a blank white page at the URL "/update.php?op=start&id=6". Then, navigating away I see that the following errors came up:

    * notice: Undefined index: id in D:\Websites\localhost\drupal-HEAD\includes\form.inc on line 2039.
    * user warning: Field 'token' doesn't have a default value query: INSERT INTO batch (timestamp) VALUES (1184258849) in D:\Websites\localhost\drupal-HEAD\includes\database.mysql.inc on line 157.

I haven't tried your patch yet. Will do tomorrow probably when I have myself setup for patching.

I'm using MySQL 5.0.27 and PHP 5.2.1 on Apache 2.0.59 here.

jakeg’s picture

Your patch doesn't work for me and maybe isn't relevant anyway. I made a quick one line fix to get rid of my white screen on update.php - http://drupal.org/node/158918

bjaspan’s picture

New patch attached. This one works on pgsql and mysql. update.php runs from d5 to d6 without query errors and produces a clean schema report on both databases (see caveats below). I am going to need to write a treatise on proper usage of default values and handling of not null/no default columns. Haven't done that yet.

Changes since previous patch:

- New function: _db_type_placeholder. Return the appropriate %-placeholder for a schema placeholder, e.g. '%s' for varchar, %d for int. Needed for db_add_field (see below).

- db_add_field now supports creating NOT NULL columns with no DEFAULT value by allowing the caller to specify the initial value for the new column in all rows while still not setting a column default. Without this, we cannot ever add a new column that is NOT NULL and has no default, such as the new variables column in watchdog.

- Fixed a bug in mysql's db_add_index(), it did not handle index keys with prefix specifications.

- system update 6010: Create the watchdog variables column to match the schema (not null, no default) in a way that works for mysql and pgsql and should work for any database.

Caveats/known issues:

- menu tables are still left for the previously referenced issue

- The locale tables are not properly handled for either database (I forgot to create the tables in d5 so the update did not touch them).

- On mysql, this patch requires that the incorrect patch in http://drupal.org/node/147947 has been reverse-applied. Otherwise, update.php still works fine but schema.module reports 10 mismatched tables because of text:big to text:medium mismatches. I am waiting for that patch to be rolled back out of core.

bjaspan’s picture

Status: Needs work » Needs review
FileSize
29.5 KB

New patch attached. Changes since last patch:

- Fixed the new _db_type_placeholder() function to handle numeric and datetime fields.
- Removed the updates in system_update_6019() to the locale tables.

The locale updates in 6019 (which I originally wrote) were wrong. The locale module has other problems during update, too, and I'm not the right person to either. So it seemed best to just remove those updates from 6019; they can be added to a correct update in locale.install, we do not need to hold up this patch for them. This one patch does not have to fix EVERY update problem with EVERY module.

This patch is now ready for review. If you want to verify schema correctness with schema.module, remember the reverse-apply the patch in http://drupal.org/node/147947 until that issue gets un-committed.

has been reverse-applied. Otherwise, update.php still works fine but schema.module reports 10 mismatched tables because of text:big to text:medium mismatches. I am waiting for that patch to be rolled back out of core.

drewish’s picture

Some comments apply to both database.mysql-common.inc and database.pgsql.inc.

This doesn't follow the coding standards for control structures:

+  if (!empty($default)) { $ret[] = update_sql("ALTER TABLE {". $table ."} ALTER $column SET $default"); }
+  if (!empty($not_null)) {
+    if (!empty($default)) { $ret[] = update_sql("UPDATE {". $table ."} SET $column = $default_val"); }
     $ret[] = update_sql("ALTER TABLE {". $table ."} ALTER $column SET NOT NULL");
   }

I feel like the comment is a bit confusing:

+    case 'numeric':
+      // We cannot use %f because it casts to float which does not
+      // have the scale and precision that a numeric column value
+      // might.  We return %s, not '%s', because numeric values
+      // should not be enclosed in quotes (though they can be, at
+      // least on mysql and pgsql).  Numerics should only have
+      // [0-9.+-] and presumably no db's "escape string" function will
+      // mess with those characters.
+      return '%s';

small thing, but it might be better to say We return '%s', not \'%s\'', .

This comment needs to be a proper sentence:

+    // all this b/c update_sql does not support %-placeholders

Notably is spelled wrong, PostgreSQL is improperly capitalized, and some of the wording is a bit hard to follow:

+ * IMPORTANT NOTE: On some database systems (noteably PostgresQL),
+ * changing a field definition involves adding a new field and
+ * dropping an old one. This means that any indices, primary keys and
+ * sequences (from serial-type fields) that use the field to be
+ * changed get dropped.  For database portability, you MUST drop them
+ * explicitly before calling db_change_field() and then re-create them
+ * afterwards.  Use db_{add,drop}_{primary_key,unique_key,index} for
+ * this purpose.

everything else *looks* good, but i haven't tested it.

pwolanin’s picture

I noice your comment:

+    // all this b/c update_sql does not support %-placeholders

is there any reason why it can't? I just discovered this the hard way and it seems like a bug. In fact update_sql passes a weird, apparently unused, "true" as a parameter to db_query()...

here's what I think it could be changed to:

function update_sql($sql) {
  $args = func_get_args();
  array_shift($args); 
  $result = db_query($sql, $args);
  return array('success' => $result !== FALSE, 'query' => check_plain($sql));
}
drewish’s picture

+1 on update_sql() taking parameters. i was bitching about that on irc just the other day.

cburschka’s picture

Seconded. I remember submitting an issue about it a long while ago, actually: http://drupal.org/node/119760

cburschka’s picture

(Addendum: And that was a duplicate of one submitted even longer ago. http://drupal.org/node/36324)

hass’s picture

Here are the errors i've got on a update from D51 DB to D6 latest (mysql):

    * notice: Undefined index: QUERY_STRING in C:\Inetpub\wwwroot\drupal6\includes\bootstrap.inc on line 668.
    * user warning: Unknown column 'textgroup' in 'where clause' query: SELECT lid, location FROM locales_source WHERE source = 'Unspecified error' AND textgroup = 'default' in C:\Inetpub\wwwroot\drupal6\includes\database.mysqli.inc on line 154.
    * user warning: Unknown column 'textgroup' in 'field list' query: INSERT INTO locales_source (location, source, textgroup) VALUES ('misc/drupal.js', 'Unspecified error', 'default') in C:\Inetpub\wwwroot\drupal6\includes\database.mysqli.inc on line 154.
    * notice: Undefined index: QUERY_STRING in C:\Inetpub\wwwroot\drupal6\includes\bootstrap.inc on line 668.
    * notice: Undefined index: QUERY_STRING in C:\Inetpub\wwwroot\drupal6\includes\bootstrap.inc on line 668.
    * user warning: Unknown column 't.language' in 'where clause' query: SELECT s.lid, s.source, t.plid, t.plural, t.translation FROM locales_source s LEFT JOIN locales_target t ON s.lid = t.lid WHERE t.language = 'en' AND s.location LIKE '%.js%' AND s.textgroup = 'default' ORDER BY t.plural DESC in C:\Inetpub\wwwroot\drupal6\includes\database.mysqli.inc on line 154.
PHP Warning: Unknown column 'serialized' in 'field list' query: SELECT data, created, headers, expire, serialized FROM cache WHERE cid = 'variables' in C:\Inetpub\wwwroot\drupal6\includes\database.mysqli.inc on line 154 

PHP Warning: Unknown column 'serialized' in 'field list' query: UPDATE cache SET data = 'a:189:{s:13:\"theme_default\";s:4:\"garland\";s:13:\"filter_html_1\";i:1;s:18:\"node_options_forum\";a:1:{i:0;s:6:\"status\";}s:19:\"file_directory_temp\";s:9:\"../phptmp\";s:9:\"site_name\";s:49:\"my Site name\";s:9:\"site_mail\";s:20:\"example@example.com\";s:11:\"site_slogan\";s:0:\"\";s:12:\"site_mission\";s:0:\"\";s:9:\"anonymous\";s:9:\"Anonymous\";s:14:\"site_frontpage\";s:4:\"node\";s:9:\"clean_url\";s:1:\"1\";s:8:\"site_403\";s:0:\"\";s:8:\"site_404\";s:0:\"\";s:11:\"error_level\";s:1:\"0\";s:14:\"cache_lifetime\";s:4:\"3600\";s:5:\"cache\";s:1:\"0\";s:19:\" in C:\Inetpub\wwwroot\drupal6\includes\database.mysqli.inc on line 154 

PHP Warning: Unknown column 'serialized' in 'field list' query: SELECT data, created, headers, expire, serialized FROM cache_page WHERE cid = 'http://localhost/drupal6/update.php?' in C:\Inetpub\wwwroot\drupal6\includes\database.mysqli.inc on line 154 

PHP Warning: Unknown column 'language' in 'where clause' query: SELECT src FROM url_alias WHERE dst = 'node' AND language IN('en', '') ORDER BY language DESC in C:\Inetpub\wwwroot\drupal6\includes\database.mysqli.inc on line 154
hass’s picture

user warning: Unknown column 't.language' in 'where clause' query: SELECT s.lid, s.source, t.plid, t.plural, t.translation FROM locales_source s LEFT JOIN locales_target t ON s.lid = t.lid WHERE t.language = 'en' AND s.location LIKE '%.js%' AND s.textgroup = 'default' ORDER BY t.plural DESC in C:\Inetpub\wwwroot\drupal6\includes\database.mysqli.inc on line 154.
bjaspan’s picture

@#8: New patch attached. I fixed the comments and code-style errors (the code-style errors were already in core, my patch just fixes a bug on those lines).

@#9: I'd be happy for update_sql to take %-placeholders, but that is a separate issue. Note that your proposed patch would cause update.php to display (e.g.) 'Query: UPDATE foo SET bar = %d' without showing the value for %d. This is probably not what we want.

@#13: Two things:

1. For now, this patch does not handle updating the locale module and update.php will break if locale.module is enabled (see #7).

2. Are you are using IIS? If so, has update.php ever worked for you without errors? It looks to me like ini_set('display_errors', FALSE); is failing to turn off error display during the part of update.php that is known to produce errors.

Everyone: Note that http://drupal.org/node/147947 has now been backed out so those schema comparison errors are gone. It turns out a similar problem was introduced by http://drupal.org/node/147761 causing one schema mismatch on dblog's watchdog.message field. I submitted a new patch on that issue to fix it.

hass’s picture

No - an update have never worked from D51 to D6, yet. Locale module will be enabled, if i select to install a different language then EN, isn't it? I know the installer is working with German PO files for new installations. This was broken, but has been fixed in the last 6 days... i can only say a dev from 08.07 isn't working and yesterday version with your patch works :-). Not verified if your patch fixes this...

Yep, i'm using IIS on XP.

bjaspan’s picture

New patch attached. The only change is removing my temporary form.inc {batch} fix in lieu of the recently committed fix. This patch still produces an error-free, clean-schema upgrade from D5 to HEAD if the locale module is disabled, excepting the menu and menu_custom tables which are the subject of a different issue.

Dries’s picture

There seems to be some cruft in the code because of the disconnect between update_sql() and db_query(). Can't we nuke update_sql() in favor of db_query()? I think that would make for a better solution. That should allow us to clean up db_add_field() and friends a bit.

+ * It never existed before on pgsql and will never be used, but since
+ * it still exists in mysql it needs to be in the schema so it needs
+ * to exist in pgsql to avoid inconsistent schemas.

I don't understand the motivation of still creating the sequences table. It would be helpful if you could motivate this a bit better in the code comments.

pwolanin’s picture

@Dris - the argument that chx made was that some contrib modules may have data in {sequences} or continue to use it in 6.x. Thus, since the goal is to have a consistent schema across all DBs, if we are not going to delete it from MySQL, then we must create it for PostgreSQL.

pwolanin’s picture

a quick functional test of this patch with MySQL 4.1/PHP 4.4 looks very good except I find it a bit problematic that i nthe first screen the link in #3 "Put your site into maintenance mode." may be invalid - for example, during a D5->D6 update. Clicking this link generates a bunch of errors. I don't have any good suggestions for how to handle this, however.

bjaspan’s picture

@Dries:

There seems to be some cruft in the code because of the disconnect between update_sql() and db_query(). Can't we nuke update_sql() in favor of db_query()? I think that would make for a better solution. That should allow us to clean up db_add_field() and friends a bit.

The purpose of this patch is to make D5 upgradable to D6. Currently all update functions use update_sql() for all queries they can so the queries can be displayed by update.php. Schema API functions like db_add_field() will almost always be called from update functions. Therefore, I wrote it to use/be compatible with update_sql() and it does cause a little cruftiness because update_sql() does not support %-placeholders.

Certainly, we could use db_query() instead of update_sql() and/or we could improve update_sql() to support %-placeholders. I avoided doing those things keep this (already large) patch as small as possible so it can get committed. There are several other issues blocking on it.

I don't understand the motivation of still creating the sequences table.

I expanded the comment extensively.

/*
 * Create the sequences table on PostgreSQL
 *
 * In D5 and earlier, we used db_next_id() to generate unique ids for
 * any object (e.g. nodes, terms, etc).  On MySQL, we used the
 * sequences table to store the most recent such value so that
 * db_next_id() would know what value to return the next time it was
 * called.  On PostgreSQL, db_next_id() was implemented in terms of
 * database sequence objects, so we never needed the sequences table
 * and it was never created.  Thus, the table exists on MySQL systems
 * but not PostgreSQL systems.  Now that we have a single unified
 * schema data structure for all database engines, this is no longer
 * acceptable.  The table must either exist on all installations or
 * none of them.
 *
 * In D6, we now use auto-incrementing columns instead of
 * db_next_id().  We have removed db_next_id() from the Drupal API and
 * no longer use or need the sequences table for core.  However, there
 * are many contrib modules that used the function and the table
 * may/does contain important data.  So, even though core created the
 * table but no longer needs it, we are keeping it around.
 *
 * Since we are keeping the table around for MySQL systems it must
 * exist in the schema structure.  Since it exists in the schema
 * structure it needs to exist on PostgreSQL (and any other database
 * engine) even though it will never be used on non-MySQL systems.
 */

I've attached a new patch. The only change is the updated comment, nothing functional.

hass’s picture

I don't understand why this sequences table stay in the DB if db_next_id() is removed from API. The function is gone and therefor nobody can use the table anymore with an API function. Should the modules write their own API to access sequences table?

pwolanin’s picture

@hass - yes I think that's the idea. Some modules may depend on the sequences table for other types of counters, and thus, at the least, dropping the tabel would mena they lose their data.

bjaspan’s picture

We should probably declare the sequences table as "deprecated" to be removed in D7.

(Actually, there is reason to believe that switching to auto-incrementing columns and removing db_next_id() was a mistake in which case the function may return some day! :-) But that is for a separate issue.)

JirkaRybka’s picture

FileSize
407 bytes

Locale data are still broken - I tried a regular update 5.1 to latest 6-dev, on a test-install with real site's data. The site uses Czech locale. After update, locale doesn't work.

I found two problems, and managed to create patches, but PLEASE someone review it, and probably correct the style. I seem unable to get the style right here in updates, still learning. But however, with these two patches the locale works well after upgrade. The problems are:

--- The new 'textgroup' column in 'locales_source' is not populated with correct data on existing rows. Default is empty, but the correct value should be the word 'default', otherwise these rows are ignored. I added a query to put this word in.

--- In 'locales_target' the column 'locale' should change to 'language', but this doesn't happen - there's no such code in the updates. Later on, another query fails due to this (Failed: ALTER TABLE {locales_target} ADD INDEX language (language)), and of course Locales doesn't work then.

Attaching the patch for locale.install.

JirkaRybka’s picture

FileSize
528 bytes

Another part is located in system.install - I'm not sure about handling the locale tables at two different places, but I leave that as is. Attaching patch file for this part.

JirkaRybka’s picture

More comments - this is just a kind of user's feedback.
My experience with current update.php on real site's data includes some more problems:

--- Authentication: After swapping codebase to D6 but before finishing update.php, the system seems to be unable of handling existing sessions and/or access control, so on this update, being logged in as user 1 doesn't work: Still update.php always shows "access denied", requiring the user to go editing the check-bypassing flag inside code. After finishing update.php, despite all the links provided, Drupal takes me as anonymous user, showing "Offline for maintenance", so I must go to the login page manually first. I'm afraid this would be impossible to fix, due to nature of the update process (correct me if I'm wrong), but still is confusing for the user, and deserves a mention in UPGRADE.txt at the very least. Also a link to the login page from update.php results would be nice.

--- Lots of pointless error messages: During the whole upgrade, it shows a lot of red error messages above (I guess these are coming via drupal_set_message() ), warning of missing table-columns which were not created by updating process yet, but will be. These messages are confusing, somewhere even broken in layout (overflowing to the right - I always say tableless design is no good for top-level page regions), there's several screens full of red messages above the tiny progress-bar. This might, again, confuse the user and make him think that all goes wrong. Wouldn't it be better to hide the messages during update?

---Logging is off by default: Update.php offers the link to admin/logs/watchdog, but that page even doesn't exist until I go and enable the "Database logging" module, and then there's nothing in there from the upgrade. This module, now separate, keeps previously existing functionality, so I think it should be enabled by default?

Given my very limited knowledge of D6 and upgrading process, I don't dare to write patches for this. My apologies here.

JirkaRybka’s picture

FileSize
428 bytes

Thinking about it, I realized that my patch from #25 might be more safe if changed a bit. Attaching slightly modified one. Still the other from #26 is also needed. Sorry for so much messages from me.

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
26.94 KB

I have used a drupalorg_testing profile install and updated it. Worked. I removed the sequences update which created the sequences table. That contrib uses it is no excuse to keep/create it. I enrolled a default textgroup. The other patch, in #26 can be a followup. Aside from removing that function I just rerolled and tested.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

Jirka: what update are you trying with??? The patch included does have code to enable dblog module for example. This would be great to clean up, so we know whether you have a problem with the after-patch version actually. (If not, we seem like on the way to RTBC this).

webchick’s picture

Small re-roll; update_6028 now needs to be 6029.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Other than the renaming issue which is fixed by the above patch, a fatal error in book.install which is covered by http://drupal.org/node/170004, and the menu update path which is covered by http://drupal.org/node/147657, I can confirm that this patch handled the 5.x => 6.x update without errors. There might still be some further bugs with more seldomly-used core modules, but I think as it is this patch makes it much easier to get more people testing the upgrade path to find those kinds of issues.

Marking RTBC.

JirkaRybka’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
28.43 KB

Re: #30 - Sorry sorry sorry... Seems I was testing unpatched 6.x-dev, my mistake. Wrong way through my local directories. BTW I'm testing with real data from production site, which is running 5.1, Czech locale.

Patched (#31) version is much better, but still there are problems with locale: Column "locale" not renamed to "language" in "locales_target" provides extensive warnings all through the update, plus locales not working at all on the site. Also the new column "textgroup" on "locales_source" is added late, gives quite a few warnings during update.php.

I did a few changes (yes, while reading the files to find my initial mistake with patching, I learned a bit about update.php), so now attaching new patch, which is further modified from #31. The changes are: locale_update_6002() moved to update_fix_d6_requirements() to get rid of warnings, added update for the "language" column problem - partially into update_fix_d6_requirements(), the rest took place instead of removed locale_update_6002() - to keep numbering of other updates intact and so minimize patch size.

Warnings from update.php before my change: Start 5x, progress a lot, results page 37x.
After change: Start clean (no warnings), progress 2x, results page 2x.

Remaining warnings:
Update progress:

    * user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') ORDER BY fit DESC LIMIT 0, 1' at line 1 query: SELECT * FROM drup_menu_router WHERE path IN () ORDER BY fit DESC LIMIT 0, 1 in /home/jirux/test-webspace/nat5end/includes/database.mysql.inc on line 157.
    * user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') ORDER BY fit DESC LIMIT 0, 1' at line 1 query: SELECT * FROM drup_menu_router WHERE path IN () ORDER BY fit DESC LIMIT 0, 1 in /home/jirux/test-webspace/nat5end/includes/database.mysql.inc on line 157.

Results page:

    * user warning: Table 'nat5end.drup_menu_custom' doesn't exist query: SELECT * FROM drup_menu_custom WHERE menu_name NOT IN ('navigation','primary-links','secondary-links') ORDER BY title in /home/jirux/test-webspace/nat5end/includes/database.mysql.inc on line 157.
    * user warning: Table 'nat5end.drup_menu_custom' doesn't exist query: SELECT * FROM drup_menu_custom WHERE menu_name NOT IN ('navigation','primary-links','secondary-links') ORDER BY title in /home/jirux/test-webspace/nat5end/includes/database.mysql.inc on line 157.
JirkaRybka’s picture

FileSize
2.69 KB

Just for better readability of my changes - attaching a diff against previous (#31 patched) version, showing what's new in my modified patch above.

webchick’s picture

@JirkaRybka : Nice one! My test site didn't have locale module so I totally missed that one.

The menu_* errors are all taken care of by another patch: http://drupal.org/node/147657 .. it still needs some work, though.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Just ran through this again with the new patch, and continues to work fine, sans menu stuff, but that's covered in that other issue.

Tentatively marking RTBC again. :)

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Except that the patch contained an $access_check = FALSE, which I needed to fix, my code review came up with no problems.

Dries' note on the cruft was said quite long ago, and we are way into the release cycle to not modify all update function calls back to every release we support updates from. I also agree with people that if we don't have an API for the sequences, contrib authors will realize that they have to migrate away from that table.

hass’s picture

hass’s picture

JirkaRybka’s picture

$access_check=FALSE came from previous patches - it's indeed quite easy to overlook. Also, in my opinion, update.php is a bit unusal place for such user-editable setting.

My new issue for this: http://drupal.org/node/170638

Anonymous’s picture

Status: Fixed » Closed (fixed)