On the comment overview list at /admin/content/comment the admin user can select multiple comments and set the "update options" select box to the teach filters option.

However this never does anything. I debugged it and found its because the form handling code in comment_spamapi_form_alter is looking for $form['#post'] which does not appear to be set at that stage. Using $form_state['post'] works though.

Patch attached.

CommentFileSizeAuthor
spam_comment.inc_.patch1.31 KBmr.j
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jeremy’s picture

Jeremy CreditAttribution: Jeremy commented
Status: Needs review » Fixed

Thanks, rewrote patch and applied it here:
http://drupal.org/cvs?commit=468978

gnassar’s picture

gnassar CreditAttribution: gnassar commented
Status: Fixed » Needs review

Sorry to be the thorn in the side, but can we check the debugging on this? It seems like if that explanation were correct, the "teach filters" option wouldn't be the only one that wouldn't work -- in fact, none of them would work at all. And yet the "mark as (not) spam" ones work fine.

gnassar’s picture

gnassar CreditAttribution: gnassar commented
Category: bug » task

Actually, this is probably more related to #531214: Missing argument 3 for drupal_process_form() - forms code not fully ported to API 2.0 meta-ticket than I'd like.

I think it may be that we didn't give the Forms API code the thorough look-through that we needed to going from 5.x to 6.x. We also had a session-variable bug that I think we recently closed that could be related to this. Can't find it with a brief search; will link later if I do.

In general, instead of passing the current form values around with session vars and using $form, we should be using $form_state for all those instances, if I understand the changes correctly. A top-to-bottom scouring of this now will probably make the move to 7.x much, much easier.

What do you guys think?

gnassar’s picture

gnassar CreditAttribution: gnassar commented
Category: task » bug

Sorry -- accidental switch. This is a bug.

AlexisWilke’s picture

AlexisWilke CreditAttribution: AlexisWilke commented

I thought #531214: Missing argument 3 for drupal_process_form() - forms code not fully ported to API 2.0 meta-ticket was fixed... 8-)

I ran in the problem at first, I'm wondering if one of the other bugs we fixed already did fix the problem. I will give it a try again but I'm pretty sure that the last time I clicked on the link, it worked (for me.)

We would need mr.j to run the 6.x-1.x-dev to make sure.

Thank you.
Alexis

gnassar’s picture

gnassar CreditAttribution: gnassar commented

Not really, no. The problem statement was that that error appeared and the feedback form approval failed. We got the error to go away, but the feedback form approval still fails. See #888328: Never posts comments with feedback.

We got rid of the error by adding a $form_state parameter, but I'm not sure we ever populated that variable correctly. And I'm also not sure that it's correctly used in other scenarios -- like the one in this ticket, for example. That's why I think a thorough review would probably be a good thing.

gnassar’s picture

gnassar CreditAttribution: gnassar commented

You know what -- the Forms API problem is bigger than I thought. Moving discussion of the review back to #531214: Missing argument 3 for drupal_process_form() - forms code not fully ported to API 2.0 meta-ticket and retitling. This issue (932758) can remain specific to this error.

mr.j’s picture

mr.j CreditAttribution: mr.j commented

It has been a while but from what I can remember of this one I just looked at how another module was handling the submission of that form and they used $form_state. So using a debugger I confirmed that it would work so I switched the code around.

Jeremy’s picture

Jeremy CreditAttribution: Jeremy commented
Status: Needs review » Postponed (maintainer needs more info)

Has anyone tested this? I tested before and after my committed changes, and confirmed that it was working as expected... thus, I'm confused what the issue is here?

gnassar’s picture

gnassar CreditAttribution: gnassar commented

From comment #2:

It seems like if that explanation were correct, the "teach filters" option wouldn't be the only one that wouldn't work -- in fact, none of them would work at all. And yet the "mark as (not) spam" ones work fine.

That's all I was concerned with -- the $form['#post'] explanation would seem to affect *all* options, not just "teach filters" -- so why did this apparently fix the problem? Did we misidentify the cause, and if so, what was it that we changed to fix it? Or did we simply circumvent a problem instead, which could come back to get us elsewhere?

An explanation to the fix would suffice for me. :)