I just noticed today that the list of Spam comments is completely broken in 6.x-1.x-dev...

I would expect a regular list, but instead the data is a big jumble!

I will post a PNG with what it should look like and how it looks now.

I'm talking about this list:

admin/content/comment/list/spam

That happens on all my sites so I'm pretty sure there's something fishy.

Thank you.
Alexis Wilke

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

AlexisWilke’s picture

There are the screenshots of a normal list and the broken one...

I know it's not the theme since I tested with several websites all have a different theme (plus if all lists work except that one...)

Thank you.
Alexis

killes@www.drop.org’s picture

Status: Active » Fixed

that list is looking fine on my test install.

Can you clear your caches?

AlexisWilke’s picture

Status: Active » Fixed

It's broken on several sites, if the cache was the culprit, it would be strange...

I'll investigate on my end to see what could be wrong. Maybe I have some other module that creates the problem.

Did you do a get latest from git? (git pull) just in case...

Thank you.
Alexis Wilke

killes@www.drop.org’s picture

Status: Fixed » Active

git pull
Already up-to-date.

Didn't mean to set it fixed.

AlexisWilke’s picture

Status: Fixed » Active

Killes,

Okay. I understand what's happening, although I don't yet have a fix.

The module_implements() function caches the list of modules that support a given hook. If you call it "too soon" then the comment_spamapi() is not yet defined. When you rebuild the themes, somehow, the comment content is not yet there and thus the spamapi() is missing. The result is that the comment spamapi() doens't get called and the corresponding theme is "lost".

I know you changed the direct module_load() and moved them inside the hook_init() instead. This is a nice cleanup but I had not realized it could have such a side effect.

When I change the spam_invoke_api() by adding spam_load_inc_files() before doing any invoke, it works. My table is fixed.

function spam_invoke_api() {
  spam_load_inc_files();
  $args = func_get_args();
  array_unshift($args, 'spamapi');
  return call_user_func_array('module_invoke_all_spam', $args);
}

The loading of hooks must be done around the time Drupal Core loads all the modules. Otherwise, it's too late. So we probably need to keep the load outside as it was before.

The hook_boot() is way to early (i.e. way before the modules are being loaded and most everything is not even defined.) So what I'm saying is that the call to spam_load_inc_files() is required immediately whenever the module is being loaded. Either that, or ALL the hooks in the content/* sub-folders need to be moved in the spam.module... or other modules need to be created (i.e. spam_node.module, spam_comment.module, etc.) which is similar to having the load as is in the main spam.module.

At this point, the module may be quite broken (certain things won't work because of this and it will be really hard to determine what.)

However, it is clear that adding a very clear documentation on why the includes are done immediately is necessary (if we choose this method.)

Thank you.
Alexis Wilke

AlexisWilke’s picture

Just a note... Change was talked about in here: #1194730: Mark node as spam = page not found error

AlexisWilke’s picture

Assigned: Unassigned » AlexisWilke
Status: Active » Needs review
FileSize
2.36 KB

There is a patch with the fix and a long comment about why it is done that way. Also there was another call to the function somewhere else which I removed (not necessary.)

I'm not too sure how to replicate the breakage of the theme, although for me it was very easy to simply run Mini -> Rebuild theme registry.

At that point the comment table theme is not making it because it's after another hook_smthg() call and that hoot does not exist (yet) since it was not loaded.

Loading at the same time as the .module is the best solution we have (i.e. all those hooks should be defined before they may be called.)

Thank you.
Alexis Wilke

killes@www.drop.org’s picture

Status: Needs review » Reviewed & tested by the community

fine with me (for the record: I didn't make that change initially, just tried to fix it)

AlexisWilke’s picture

Status: Reviewed & tested by the community » Fixed

Okay! Done. 8-)

It looked like you checked it in. But that being said, you may have used a patch from someone else... I also was thinking that using the hook_init() would work. Unfortunately, not if you load other hooks required from the time the .module is loaded. And I did not notice the problem for a while meaning that in many cases it's probably just fine with the hook_init().

Thank you.
Alexis

See: http://drupalcode.org/project/spam.git/commit/356b74a

Status: Fixed » Closed (fixed)

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