I am getting a Drupal/PHP error message when trying to update a node that has been already included in the spam_tracker table in the database. The call to db_query() inside spam_content_update() seem to be broken. I have attached a patch to fix it. Thanks.

CommentFileSizeAuthor
spam.module.patch595 bytesprogga
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

AlexisWilke’s picture

progra,

Good catch, note also that the hostname is a string! (And you should use -du with diff)

<     db_query("UPDATE {spam_tracker} SET score = %d, hostname = %d, timestamp = %d WHERE content_type = '%s' AND content_id = '%s'", $score, time(), $type, $id);
---
>     db_query("UPDATE {spam_tracker} SET score = %d, hostname = '%s', timestamp = %d WHERE content_type = '%s' AND content_id = %d", $score, ip_address(), time(), $type, $id);
Jeremy’s picture

Status: Active » Fixed

Thanks, patch committed:
http://drupal.org/cvs?commit=468970

(As an aside, I suspect that the 'hostname' hook is somewhat buggy, in part is it's only implemented for comments. I was originally going to use it instead of ip_address(), but ip_address() should always be valid when creating content.)

gnassar’s picture

Title: spam_content_update() is broken. » spam_content_update() is broken - inconsistent hostname derivation
Status: Fixed » Needs work

This is one of the ones I wanted to hold off on and discuss. It's more complex than it looks, at least if we want to make a clean fix instead of adding to the confusion.

Lines 302-306, in the same update function:

if (!isset($extra['host'])) {
// Content type modules can set this value, should REMOTE_ADDR not be
// the correct IP for their content type.
$extra['host'] = ip_address();
}

So the "right" way to do this is probably to move this above the UPDATE line (since it's not really dependent on the if block it's in), and then use $extra['host'] as the variable.

...except not necessarily. It's one option. Another is to fix the hostname hook -- I'm not sure it's actually buggy, as much as it is simply not implemented. The _comment content type is the only one that has that hook. _node and _user don't, which could potentially cause problems where the hook is in use in spam.module: specifically, when calling spam_mark_as_(not_)spam for a content item that isn't in spam_tracker yet.

We can also get rid of all the granular hooks for every little content item, and instead ask folks to implement one "spam_details" hook which would return an array of all this stuff. Kind of like $extra[] is now, but not depending on it being passed. (Which we'd need for the _mark_as_spam calls, again.) That would make later Actions integration easier, I'd imagine, when we can pull that info instead of having to have it pushed to us.

Whichever way we pick, we should use it consistently across the module. Which brings up a related point: this entire problem can be refactored completely out of the "insert" and "update" cases, since they already occur in the mark_as_ functions.

I'm leaning toward fixing the hostname hook as a short-term fix, and hook_spam_details as the long-term API modification. Thoughts?

AlexisWilke’s picture

I'm not yet aware of all the things that the module does so I cannot say much about the hook_spam_details() idea. However, if using the $extra['host'] would be better, then we should at least do that. This being said, this very bug was about the UPDATE using %d instead of '%s' and '%s' instead of %d which simply did not do anything good at all.

Thank you.
Alexis

gnassar’s picture

Yes, that's correct. But the bug is caused by an underlying structural problem, which is what I was addressing. This was part of the refactoring that I mentioned I had started doing on the "support" ticket -- this bug is representative of a bigger problem that needs a more thorough solution.

Using $extra['host'] would require that to be passed everywhere else in the code that we'd be calling these functions. Using the hooks to pull that data instead of requiring functions to push it is probably both easier and a better long-run strategy.

AlexisWilke’s picture

What I do like with the module_invoke() is that any module can add functionality to your module. However, that call is very slow. Each time it goes through the entire array of ALL the PHP functions! That's quite terrible.

So if it is useful/necessary in the API, using a module_invoke() is a great idea. Otherwise, having the $extra as a parameter would be better (as in a lot faster at execution time.)

Thank you.
Alexis

gnassar’s picture

Well, that's the trick about an API, I guess. :) If you want a module to be extensible, you have to allow for hooking. And that's pretty much module_invoke().

Is there another way to do this that would be less heavy, but wouldn't rely on folks to provide arrays of seemingly-arbitrary data to get things to work? I'm not sure we have any other options that are as robust.

AlexisWilke’s picture

That's the Drupal way, although an object oriented approach would have been faster in general... where you extend a class with your own implementation of a function. But the truth is that in most cases objects need to register themselves to get called. The best in PHP is certainly what the views do: create a file with a specialized name. That file is automatically loaded as required.

Anyway... the main idea is certainly to determine whether the extendability is necessary at that location. If you foresee other modules offering capabilities via that call (with say one or two examples on how it would be used) then we may want to go with the module_invoke() call.

Thank you.
Alexis

gnassar’s picture

Like I mentioned, Actions integration would be one of those.

If an action fired on an event, obviously you'd need to be able to pull the hostname from the content (instead of using ip_address(), which would just give you the server's IP or the IP of the person firing the event, which may not have anything to do with updating the content). But nothing will be "pushing" the hostname to the action through a function -- we'll need the ability to pull the hostname instead. We abstract away all content types into content modules, of course, so to pull the hostname for a particular content type, we'd need a call to the content module to request the hostname.

AlexisWilke’s picture

That makes sense... so I guess we'd need that module_invoke() call. 8-)

Note that in regard to actions, we should look into Rules instead because Actions are gone in D7. Plus, they are not very useful for users (only developers and still, they don't really offer such great functionality.)

Thank you.
Alexis

gnassar’s picture

A quick note -- I just checked, and to what extent I can determine, Actions are still in D7 (in core, just like they were in D6). Rules are more a replacement for triggers (and to some degree the workflow module). Rules require Actions, in that they perform whatever Action is defined when they are triggered.

AlexisWilke’s picture

Version: 6.x-1.0 » 6.x-1.x-dev
Status: Needs work » Active

gnassar,

If you assumption is correct (i.e. message in #3) then the insert is also a problem:

function spam_content_insert($content, $type, $extra = array()) {
  ...
    if (!$sid) {
      db_query("INSERT INTO {spam_tracker} (content_type, content_id, score, hostname, timestamp) VALUES('%s', '%s', %d, '%s', %d)", $type, $id, $score, ip_address(), time());
      $sid = db_result(db_query("SELECT sid FROM {spam_tracker} WHERE content_type = '%s' AND content_id = '%s'", $type, $id));
    }
  ...
}

This being said, the bug that was reported in 1.0 is now fixed in 1.2. Just wanted to mention that.

Changing from needs work to active since we do not currently have a patch for the problem at hand.

Thank you.
Alexis Wilke