common.inc is nice and simple, with not much to convert.

OK, not entirely true. There's some logic in drupal_error_handler() that is dependent on the old database system for determining whether an error occurred "inside" the DB layer that I don't think applies anymore, but there are other patches already in the queue to overhaul that function anyway. I also didn't touch the horror that is drupal_write_record(), as I am not convinced that we even need to keep that function. We'll find out after refactoring the rest of Drupal to see if db_insert() and db_merge() can reasonably replace it (I think they can). If not, we can convert it then but for now it's not worth the time. That makes this a low-kitten-count patch. :-)

CommentFileSizeAuthor
#5 common_inc.patch3.58 KBCrell
common_inc.patch1.7 KBCrell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Code looks good. Need to test.

OT: Why does the return in flood_is_allowed do return ($number < $threshold ? TRUE : FALSE); instead of return ($number < $threshold);?

Crell’s picture

Um. I have no idea. The original code did, and I didn't want to harm any kittens in this patch. :-)

Dave Reid’s picture

Separate patch for the TRUE : FALSE in #319666: Remove unnecessary boolean ternary conditionals. I found a few more of them throughout core.

Senpai’s picture

Status: Needs review » Needs work

Fail. Visiting user/3/contact while logged in as uid3 results in a PDO exception.

PDOException: SELECT COUNT(*) FROM {flood} WHERE event = :event AND hostname = :hostname AND timestamp > :timestamp - Array ( [:name] => contact [:hostname] => 127.0.0.1 [:timestamp] => 1224899714 ) SQLSTATE[HY093]: Invalid parameter number: parameter was not defined in flood_is_allowed() (line 1003 of /www/drupalhead/includes/common.inc).

Incidentally, there were 0 lines in {flood} when I started the test.

Crell’s picture

Status: Needs work » Needs review
FileSize
3.58 KB

Yar. Fixed. I also removed the redundant ternary while I was at it.

Dries’s picture

I committed this to CVS HEAD. Thanks Crell and Dave.

Dave, can you create an issue for the fact that the SimpleTests did not catch the first regression? Sounds like we're not testing the flood system.

Dries’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

Crell’s picture

Issue tags: +DBTNG Conversion

Tagging.