Refactor advpoll_choices schema for unique ids, etc.

Hmm what version of MySQL are you using and what error do you get when you change vote_offset to be NOT NULL?

Ah, take out the preceding "default" when you change it to "not null".

We might as well fix this in the code, as I don't see a good reason to keep "default NULL".

Here's the old MySQL bug for this btw (fixed in more recent versions): http://bugs.mysql.com/bug.php?id=390

I think we can remove that "KEY vote_offset (vote_offset)" also, because if a column is already listed as a primary key it shouldn't need another separate key, no?

Sounds fair to me, even though not really my area.

Here's a patch. I tested upgrading and installing from scratch and I didn't get any errors.

Bah, just noticed an error in the pgsql code.

And like many areas, we should look to poll.module for comparison: http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/poll/poll.install?...

In this case poll.module has its vote_offset (chorder) as NOT NULL, and does not have a separate index. Also note that each choice has a unique id in core, which is something we should eventually do too (for reordering, etc., esp. with the sweet AHAH drag-and-drop in D6).

I realized I should just stop being lazy and implement unique ids for choices. A comprehensive patch is in the works.

Excellent. Testers are lined up.

Here's an initial patch with basic functionality - needs more testing though.

Thought: should we also rename vote_offset to "order"?

0. Old polls doesn't display the results correctly. I couldn't find the problem but I think it's a display issue, and not a problem with the upgrade path.

1. (nid, label, vote_offset, writein) VALUES (%d, '%s', %d, 0) - If writein is never anything else than 0, it should be safe to leave it out.
2. db_query('SELECT cid, vote_offset, label, writein - can be db_query('SELECT *, not sure what is prefered.
3. $form_values['nid'], check_plain(implode(', ', $form_values['promote']))); - usually check_plain is only used on output.
4. I prefer {order} instead of {vote_offset}
5. Just an observation: Core uses 'chid' instead of 'cid' (but also prefixes all column names with ch).

I will run the upgrade path on a bigger dataset when this is ready.

Btw, D6 just got drag-and-drop reordering: http://drupal.org/node/193076

0. Ah should be fixed; I needed to update votingapi_cache as well as votingapi_vote for the new choice ids.
1. Double true.
2. Eh I prefer to write em out.
3. I noticed that, but it's a separate issue.
4. I started doing order but I realized it might mess up the SQL since it's probably a reserved word. The D6 reordering patch also renamed that column and they chose weight, so that's what I did here.
5. Yeah that's just because poll.module is old and bad.

Quick code review, will do some more testing later.

1. Typo, should be cid int unsigned NOT NULL auto_increment
2. I core, when serial is used, this is often used: cid serial CHECK (cid >= 0)
3. update_sql('ALTER TABLE {advpoll_choices} ADD cid INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY FIRST') - Double-check use of upper/lower case?

1. & 3. - got it.
2. Yeah I thought about doing that but I couldn't find an example of getting that added in an update function. Since it's optional I would prefer to keep the installs and updates with the same db schema.

Run the upgrade on a larger dataset and I couldn't see any problems.

Don't forget to bump up the upgrade to 8 (7 just went in), apart from that this is ready. Exciting times in Drupal 6!

Woot - http://drupal.org/cvs?commit=90024

Right after the commit I realized I forgot about advpoll_convert.module. Fixed now: http://drupal.org/cvs?commit=90031

Ok, cool. The last SQL statement in that row is mixing %d and numbers (1 and 0), extremely minor, but we might as well fix that.

Which way of fixing it do you prefer? I'm ambivalent.

Me too, that's why I didn't suggest anything :) I search core some days ago and at least saw that they used 0 instead of %d once. But you decide :)

Bah fine :P 0 it is.

http://drupal.org/cvs?commit=90032