Posted by olav on September 24, 2007 at 4:02pm
4 followers
| Project: | Nodequeue |
| Version: | 5.x-2.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | dww |
| Status: | closed (fixed) |
Issue Summary
You added the field in nodequeue_update_5203(). Here is the patch to add it to nodequeue_install() as well.
--
Olav
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| nodequeue_1.patch | 540 bytes | Ignored: Check issue status. | None | None |
Comments
#1
You forgot the pgsql case in hook_install(). Otherwise, good catch.
#2
As far as I know, this will fail on pgsql:
db_add_column($ret, 'nodequeue_queue', 'reverse', 'int(1)');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?
#3
I 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.
#4
#5
Tested 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.
#6
tested on pgsql. works as advertised.
note that there are several other bugs in the install/uninstall process:
#7
@hunmonk #6:
(1) http://drupal.org/node/182378
(2) http://drupal.org/node/182458
Thanks for the review and testing on this one...
#8
One 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)
#9
#10
@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.
#11