DBTNG poll.module

This issue is strict forward and simple, +1. BTW, it is duplicated with http://drupal.org/node/199101.
Tested with MySQL and PostgreSQL, pass.

Rerolled against latest head. Applies & works correctly with both MySQL and PostgreSQL. Simpletests run successfully for poll module. +1

Patch reroll via CVS HEAD, test with MySQL and PostgreSQL.

Please rewrite this one to use proper placeholders.

@Damien: Sorry that I can't find any placeholders used within this issue. May you give a kindly review again?

There are no placeholders, that's precisely the issue.

This issue is about the syntax cleanup of != with <> based on ANSI standard, but not legacy placeholder update... Am I correct?

Ok, not it's both.

So in this way? Update both db_query(), fetchField() and db_update().

Tested with MySQL, cron run correctly where outdated post inactive correctly, too.

Maybe #299176: Replace db_rewrite_sql() with hook_query_alter(). ??

Sorry that I am so fool and not really understand the needs of this issue: the original goal is about != => <> conversion, but now seems talking about TNG DB standard. I need some clear instruction about the requirement of change, and may someone give me a hand?

Patch no longer applies to HEAD.

Here's a patch to bring the entire poll module in line with DBTNG and includes the ANSI != fix as well. Passed all poll tests 100%.

Subscribing, need to review this...

Probably the recent bot bug...

That exception is for dblog.test. All the poll tests passed and this should be good.

Ok so I need to figure out how to do the following without errors:

  db_update('poll_choices')
    ->fields(array())
    ->expression('chvotes', 'chvotes + 1')
    ->condition('chid', $node->vote)
    ->execute();

  db_update('poll_choices')
    ->fields(array())
    ->expression('chvotes', 'chvotes - 1')
    ->condition('chid', $node->vote)
    ->execute();

  $condition = db_and()->condition('nid', $node->nid);
  if ($user->uid) {
    $condition->condition('uid', $user->uid);
  }
  else {
    $condition->condition('hostname', ip_address());
  }
  db_delete('poll_votes')
    ->condition($condition)
    ->execute();

Forgot to set as code needs work.

Uh. fields() with an empty array? Does not compute. What are you trying to do there?

It is replacing: db_query("UPDATE {poll_choices} SET chvotes = chvotes + 1 WHERE chid = %d", $node->vote)

Just leave off fields() entirely then. If that doesn't work, that's a bug in DBTNG.

db_update('poll_choices')
  ->expression('chvotes', 'chvotes + 1')
  ->condition('chid', $node->vote)
  ->execute();

Without the fields() I get:
* Warning: Invalid argument supplied for foreach() in UpdateQuery->execute() (line 916 of /home/davereid/Projects/drupal-head/includes/database/query.inc).
* Warning: Invalid argument supplied for foreach() in UpdateQuery->__toString() (line 939 of /home/davereid/Projects/drupal-head/includes/database/query.inc).

For some reason the db_and() isn't working correctly: when that code is called I get:
Fatal error: Call to undefined function db_and() in /home/davereid/Projects/drupal-head/modules/poll/poll.module on line 871

I got everything working. To get the update query working, just had to set the UpdateQuery class variable to a default array: "protected $fields = array();"

Postponed until #343620: db_update() fails on expression-only SQL gets in to fix the UpdateQuery in DBTNG.

Now that #343620: db_update() fails on expression-only SQL is in, I figured my way around the multiple-node-load changes, and I was got bit by the table name changes, revised DBTNG patch ready for review. Passes all tests.

I'm simply puzzled as to why changes to poll.module are causing exceptions in dblog.module.

Found the issue... there was an error in the poll_cron, since dblog invokes cron on all modules...

This all looks good - although why explicitly use fetchObject() instead of fetch()?

I thought I read somewhere that it is preferred to use fetchObject() and over fetch() since it is easier for anyone else to understand what type of data the results will be. I cannot find anything right now to back me up, so it very well might have been my own theory,

It's probably more self-documenting that way. We should decide on a standard and stick to it either way.

We've informally decided on fetchObject() and fetchAssoc() for cases when you need to specify it directly. I need to update the docs to reflect that better. fetch() should be used when you need some other fetch mode than those two easy ones.

Adding tags.

Re-assigning for work at Drupalcon DC Code-Sprint.

Cool. :) Most of your work is already done in #32, but just needs a quick update.

The biggest pain in this seems to be related to the PagerDefault queries. I've replaced all of the pager queries and db_rewrite_sql queries, and everything passes the provided simpletests. Time to get someone else's eyes on this thing.

Whoops, changing status.

Re-rolling from the root directory.

The bot likes it, I read the code and I like that too. So... go :)

Please check trailing whitespace in the patch.

+    $select->fields('n', array('nid'))      <--- whitespace
...
+      ->addTag('node_access');
+      <--- whitespace
...
-    return $block;
+    }    <--- whitespace
...
etc

p.runtime != :runtime should be changed to p.runtime <> :runtime for ANSI compatibility.

Re-rolled with those fixes. Nice catch on the whitespace.

Cleaning up an issue with poll_cron.

Actually for poll_cron, instead of

  $queried_polls = db_query('SELECT p.nid FROM {poll} p INNER JOIN {node} n ON p.nid = n.nid WHERE (n.created + p.runtime) < :request_time AND p.active = :active AND p.runtime <> :runtime', array(':request_time' => REQUEST_TIME, ':active' => 1, ':runtime' => 0));
  foreach ($queried_polls as $poll) {
    db_update('poll')
      ->fields(array('active' => 0))
      ->condition('nid', $poll->nid)
      ->execute();
  }

Let's do:

$nids = db_query('SELECT p.nid FROM {poll} p INNER JOIN {node} n ON p.nid = n.nid WHERE (n.created + p.runtime) < :request_time AND p.active = :active AND p.runtime <> :runtime', array(':request_time' => REQUEST_TIME, ':active' => 1, ':runtime' => 0))->fetchCol();
db_update('poll')
  ->fields(array('active' => 0))
  ->condition('nid', $nids, 'IN')
  ->execute();

Theoretically, I could see that leading to a rather cumbersome update clause. Practically, I don't really see that happening, though, so I went ahead and changed it. The only addition I made other than that was to protect against returning no results.

Committed to CVS HEAD. Thanks.

One advantage of the new syntax is that we don't care how cumbersome a long list of ints in an IN clause is, because it's handled for us automatically. :-) The change from #49 reduces the task to only 2 queries ever, which is great.

Thanks everyone!

Good to know. Thanks for the info, and all the help at the sprint.
REALLY liking the new DB layer. :-)