There are a few issues in the 6000 upgrade script.

I get the error:
user warning: Duplicate entry 'garland-gallery-block-1' for key 2 query: UPDATE blocks SET delta = 'block-1' WHERE module = 'gallery' AND delta = 'grid-0' in XXX/sites/all/modules/gallery/gallery.install on line 128.
It's strange because I tried to debug it and $delta is definitely 2 at this point so it should be setting delta = 'block-2', but it does not.

By not merging $ret and the return from gallery_update_variables into one $ret this failure goes unnoticed in the update portion of the return text.

The new variables (like gallery_block_1_imageblock_num_cols, gallery_block_num etc) are not serialized before being inserted into the database and so appear blank when you look at them on the settings page.

CommentFileSizeAuthor
#9 gallery_install_fix2.patch5.64 KBkiz_0987
#4 gallery_install_fix.patch2.9 KBkiz_0987
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

profix898’s picture

profix898 CreditAttribution: profix898 commented

Drupal 5 to Drupal 6 updates are not really supported yet. I wrote the code to migrate block settings a few weeks ago on the train, but didnt have time to test and debug (priority was to make the module work first and then complete the upgrade path). What means it is totally possible (and I even expect it) that the upgrade path is broken atm. You are welcome to get your hands dirty on the _update code ;)

kiz_0987’s picture

kiz_0987 CreditAttribution: kiz_0987 commented
Assigned: Unassigned » kiz_0987

Will do.

profix898’s picture

profix898 CreditAttribution: profix898 commented

Great. Thx. I hope to fix the D6 menu issues later this evening.

Once both issues are fixed there is one major problem left: Javascript. Several aspects of the new YUI widgets in G2.3 dont work and I still cant tell exactly why! Do you know any JS guru out there? (references: http://drupal.org/node/224019 and http://drupal.org/node/188035)

kiz_0987’s picture

kiz_0987 CreditAttribution: kiz_0987 commented
FileSize
gallery_install_fix.patch2.9 KB

Here's a patch to fix the 3 issues mentioned. It turns out that update_sql does not support % substitution. It looks like earlier Drupal 6 did, but then the feature was dropped before release (security?). Now the return is less useful -- always returns 'success' => TRUE even if it wasn't. This is common to some of the core modules (eg system.install). Not great, but it works.

kiz_0987’s picture

kiz_0987 CreditAttribution: kiz_0987 commented
Status: Active » Needs review
profix898’s picture

profix898 CreditAttribution: profix898 commented
Status: Needs review » Needs work

I took a quick rush through your changes:
1. update_sql is simply a wrapper for db_query (see http://api.drupal.org/api/function/update_sql/6), so there shouldnt be any different behavior for it
2. There was a patch to improve compatibility with Oracle/MSSQL/... committed not too long before the RCs. The result is that now every string/literal must be moved out of the actual query and replaced with a %-placeholder instead. (see http://drupal.org/node/114774#db-query-standard or sql coding conventions at http://drupal.org/node/2497). We should probably check all our queries to comply the new standard.
3. $ret2 = gallery_update_variables($migrate, $obsolete); $ret = array_merge($ret, $ret2); can be concatenated using += notation, like $ret += gallery_update_variables($migrate, $obsolete);, not? We want to keep both arrays completely and not merge them if keys are identical in both of them ...

kiz_0987’s picture

kiz_0987 CreditAttribution: kiz_0987 commented

1. update_sql is simply a wrapper for db_query (see http://api.drupal.org/api/function/update_sql/6), so there shouldnt be any different behavior for it

Yes, you'd think so and I did :-( , but the docs state that

%-substitution parameters are not supported

and indeed they do not work. I suspect the 'true' added to the db_query call is designed to override any args that you try to add to the call.

I'll reroll with your third comment (and I noticed that the new image grid var for gallery_block_X_imageblock_block_block is not migrated so I'll also look at that).

kiz_0987’s picture

kiz_0987 CreditAttribution: kiz_0987 commented

I started to work through changing the db queries to follow the % placeholder approach you mention. This makes almost all update_sql calls totally useless as it will not do % substitution. I then checked to see how core modules are doing this and you will see a bunch of cases like
$ret[] = update_sql("UPDATE {profile_fields} SET category = 'Account settings' WHERE LOWER(category) = 'account'"); or even worse
$ret[] = update_sql("UPDATE {permission} SET perm = '$renamed_permission' WHERE rid = $role->rid"); in system.install.

How would you like to proceed?

kiz_0987’s picture

kiz_0987 CreditAttribution: kiz_0987 commented
FileSize
gallery_install_fix2.patch5.64 KB

Here's an update which has most of the db_queries/update_sql changed. And it fixes the migration of the grid block.

profix898’s picture

profix898 CreditAttribution: profix898 commented

Do you plan to complete this patch or do you want me to take over?

kiz_0987’s picture

kiz_0987 CreditAttribution: kiz_0987 commented
Status: Needs work » Fixed

Committed bugfix. Also changed db_query calls in gallery.install to use new %-placeholder requirements.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

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

rerooting’s picture

rerooting CreditAttribution: rerooting commented
Status: Closed (fixed) » Active

I hate to reopen this, but just had this happen on a D5 to D6 upgrade. Ran update.php and got this error in the log:

Duplicate entry 'gallery_block_num' for key 1 query: INSERT INTO variable (name, value) VALUES ('gallery_block_num', 'i:2;') in /home/shelleylubben/public_html/sites/all/modules/gallery/gallery.install on line 161.