You added the field in nodequeue_update_5203(). Here is the patch to add it to nodequeue_install() as well.
--
Olav

Comments

dww’s picture

Status: Needs review » Needs work

You forgot the pgsql case in hook_install(). Otherwise, good catch.

dww’s picture

Status: Needs work » Needs review
StatusFileSize
new1.58 KB

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?

merlinofchaos’s picture

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.

dww’s picture

Assigned: Unassigned » dww
Status: Needs review » Needs work
dww’s picture

Status: Needs work » Needs review
StatusFileSize
new1.87 KB

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.

hunmonk’s picture

Status: Needs review » Reviewed & tested by the community

tested on pgsql. works as advertised.

note that there are several other bugs in the install/uninstall process:

  1. extra comma in table creation statement for {nodequeue_nodes}
  2. the nodequeue_queue_qid_seq needs to be dropped upon uninstall for pg.
dww’s picture

@hunmonk #6:
(1) http://drupal.org/node/182378
(2) http://drupal.org/node/182458

Thanks for the review and testing on this one...

merlinofchaos’s picture

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)

merlinofchaos’s picture

Status: Reviewed & tested by the community » Fixed
dww’s picture

@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.

Anonymous’s picture

Status: Fixed » Closed (fixed)