You added the field in nodequeue_update_5203(). Here is the patch to add it to nodequeue_install() as well.
--
Olav
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | nodequeue_update_5203_patch_2.txt | 1.87 KB | dww |
| #2 | nodequeue_update_5203_patch.txt | 1.58 KB | dww |
| nodequeue_1.patch | 540 bytes | olav |
Comments
Comment #1
dwwYou forgot the pgsql case in hook_install(). Otherwise, good catch.
Comment #2
dwwAs far as I know, this will fail on pgsql:
pgsql doesn't understand "int". Furthermore, these "(N)" declarations don't do what you expect for all cases except varchar. You really should stop using them.
Attached patch does the following:
- uses tinyint for this field on mysql
- uses smallint for this field on pgsql
- fixes update 5203 to get this right on both engines
I haven't actually tested any of this, yet. ;) Also, not sure how you want to handle this in terms of sites that already tried to run 5203? Should we clean all of this up in 5204 and just make 5203 silently do nothing for pgsql?
Comment #3
merlinofchaos commentedI would agree that 5203 should silently fail for pgsql and then fix everything in 5204.
Using int(x) is just habit, really; I'm perfectly ok with fixing them all to just int, honestly.
Comment #4
dwwComment #5
dwwTested this one on MySQL and I didn't break anything. Sadly, my PgSQL test site has fallen out of disrepair, so if we can find someone who can test this more easily, that'd be great.
p.s. Do you really want this column to allow (and default to) NULL? Just curious. This would be a good time to fix that if that'd not what you had in mind.
Comment #6
hunmonk commentedtested on pgsql. works as advertised.
note that there are several other bugs in the install/uninstall process:
Comment #7
dww@hunmonk #6:
(1) http://drupal.org/node/182378
(2) http://drupal.org/node/182458
Thanks for the review and testing on this one...
Comment #8
merlinofchaos commentedOne problem in 5204 was that nodequeue_nodes was being referenced rather than nodequeue_queue; I fixed that and applied (along with the missing comma patch)
Comment #9
merlinofchaos commentedComment #10
dww@merlinofchaos: ugh, sorry about that table-mismatch... i noticed that and fixed it locally, but apparently forgot to update the patch in here. :( thanks for catching it.
Comment #11
(not verified) commented