Hi Jerome,

I just started on the code review and discovered a little problem in the statistics. Nothing major since it is most certainly always called with the right operation. However, a bad operation should not be accepted and when so detected the function should return.

See patch for more info.

Thank you.
Alexis

CommentFileSizeAuthor
#4 spam-6.x-statistics_operation2.patch6.38 KBAlexisWilke
#2 spam-6.x-statistics_simplification.patch6.89 KBAlexisWilke
spam-6.x-statistics_operation.patch639 bytesAlexisWilke

Comments

gnassar’s picture

gnassar commented
Priority: Critical » Minor
Status: Active » Needs work

Can't hurt, I guess, even though we don't ever even call update_statistics with the $op parameter defined. But I think you'd still want spam_log to record the attempted update (as in the line after the return). So perhaps dupe that line before we break out of the function.

Also, if we're being picky, the function should probably be named _spam_update_statistics -- prefixed with the underscore, as it shouldn't be being called externally.

AlexisWilke’s picture

AlexisWilke commented
StatusFileSize
new spam-6.x-statistics_simplification.patch6.89 KB

In that case I propose that we simplify the function because that's totally useless to support totally unused stuff (unless you know for sure that's going to be used in the near future?)

I copy the new version here as it is a bit harder to read in the new proposed patch.

/**
 * Increment internal counters.
 */
function _spam_update_statistics($name) {
  spam_log(LOG_DEBUG, '_spam_update_statistics', t('@name = @name + 1', array('@name' => $name)));
  db_query("UPDATE {spam_statistics} SET count = count + 1, timestamp = %d WHERE name = '%s'", time(), $name);
  if (!db_affected_rows()) {
    db_query("INSERT INTO {spam_statistics} (name, count, timestamp) VALUES ('%s', 1, %d)", $name, time());
  }
}

Thank you.
Alexis

gnassar’s picture

gnassar commented
Category: bug » task

Sorry, but I believe the burden of proof goes in the other direction. Unless you think that the functionality will *not* be used, a substantive patch to remove said functionality isn't appropriate to the ticket.

So we can discuss whether that functionality would ever be used, or we can move the line and apply the patch.

Frankly, considering this is an error-check on a limited number of internal calls all of which we know ahead of time, this is a very trivial point to be spending a lot of time on, so I wouldn't be particularly keen on doing a thorough dissection of whether we ever need a minus case on statistics. This should be a one-minute fix, not a ten-minute discussion.

AlexisWilke’s picture

AlexisWilke commented
Status: Needs work » Fixed
StatusFileSize
new spam-6.x-statistics_operation2.patch6.38 KB

Okay, I moved the log call, added the underscore and the return statement and it is checked in.

Thank you for the review.
Alexis

P.S. It looks like you like to have the commit references... 8-)
http://drupal.org/cvs?commit=469604

gnassar’s picture

gnassar commented

It's not about having commit references -- it's about issues not going from "needs work" to "fixed." A patch should be submitted to the ticket and RTBC'd, even if that's just one person taking a look at it as a sanity check.

Like on this one. :) It's not a big deal, but it would be nice if it logged at the right time. The standard is, log immediately *after* the operation. Again, this piece is really minor, so it's not worth going back and fixing compared to time with the rest, but the log line should occur after the event. Just good standard practice.

So (this being pseudocode off the top of my head):

function spam_update_statistics() {
    if ($op != '+' && $op != '-') {
        watchdog ('op ignored');
    } else {
        db_query('UPDATE...')
        if (!db_affected_rows()) {
            all the other stuff
        }
    }
    spam_log(LOG_DEBUG, 'spam_update_statistics', etc.)
}

Sorta like that.

Again, the big deal here: ticket -> patch -> third-party OK -> commit. Can't skip those middle steps.

Status: Fixed » Closed (fixed)

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