Support from Acquia helps fund testing for Drupal Acquia logo

Comments

soxofaan’s picture

new version of patch against captcha.module version 1.42.2.24

soxofaan’s picture

Component: Code » User interface
FileSize
3.64 KB

minor update of patch (tweaked UI strings)

soxofaan’s picture

screenshot of the result of the patch

RobLoach’s picture

Should the counter go inside the 'if' statement because you don't want it to count the wrong responses?

    // set form error
     form_set_error('captcha_response', t('The answer you entered for the captcha challenge was not correct.'));
    // log to watchdog if needed
    if (variable_get('captcha_log_wrong_responses', FALSE)) {
      // update wrong response counter
      variable_set('captcha_wrong_response_counter', variable_get('captcha_wrong_response_counter', 0) + 1);
      watchdog('CAPTCHA',
        t('%form_id post blocked by CAPTCHA module: user answered "%response", but the solution was "%solution".',
          array('%form_id' => $form_id, '%response' => $captcha_response, '%solution'=> $_SESSION['captcha'][$form_id][$captcha_token])),
        WATCHDOG_NOTICE);
    }
   }
soxofaan’s picture

No, you didn't get me right. There are two separate (but related) things in this patch:

  • always counting the wrong responses. It would be overkill to make this optional with some checkbox, counting is not very resource intensive, it's just about 1 variable.
  • optionally logging the wrong responses to the watchdog log. Because wrong response logging could pollute the log this should be optional.

hope this makes it more clear.

RobLoach’s picture

I think I'd rather have Captcha integrate with the Actions module. Then you could do ridiculous things like: log it in watchdog, have it send an email, report it to drupal_set_message, and send an SMS message to the user telling them they're stupid :-P .

soxofaan’s picture

I think I'd rather have Captcha integrate with the Actions module. Then you could do ridiculous things like: log it in watchdog, have it send an email, report it to drupal_set_message, and send an SMS message to the user telling them they're stupid :-P .

Do you mean you want Actions functionality instead of the logging from my patch, or just additional Actions functionality?
Since Actions is not a core module, it seems a bad idea to add this dependency.
And IIRC, with Drupal 6, there will be built-in functionality to route the log items the way you want, so I would wait for that do enable the crazy stuff you mentioned.

soxofaan’s picture

Update of this patch (for captcha.module v1.42.2.28)

soxofaan’s picture

update of patch (against captcha.module version 1.42.2.30)

soxofaan’s picture

This is a small harmless subset of the previous patches, which only adds a wrong response counter. (see patch)

soxofaan’s picture

update of patch from #10

RobLoach’s picture

FileSize
1.52 KB

If you're going to be writing to the database at all during an anonymous session, I'd rather have it write useful data rather than just an incremental value. The count of how many forms is has blocked isn't really helpful. What forms were blocked? What time were these forms blocked? This information you can't get from just the count. When you put it in the log, however, you can get all that information, including the number of forms that were blocked.

Putting log information like the count of forms that were blocked into the CAPTCHA settings doesn't really make sense to me, as usually log information goes into the log entries section of the Drupal administration. I don't know, maybe it's just me...

Overall, I like what you did with the watchdog, just counting the number of blocked forms in the settings page seems useless when the watchdog can do that sort of thing for us.

RobLoach’s picture

... And if you want to get a count of forms that are blocked in the settings page, you could use a SELECT COUNT on the watchdog table for the entries of type 'captcha'.

soxofaan’s picture

How I see it:

As a CAPTCHA user I would like to know how many submissions were already blocked, just to see how efficient it is to enable CAPTCHA on my site and to see how many spam cleanup I saved. E.g. if I have a CAPTCHA on the comments and I get 1 human submission and 100 blocked spam bot submissions per week, I know it was a good choice to enable CAPTCHA. Without a counter I have no idea what the CAPTCHA module does and if it realy works on real spam bots. Just a count (without a clue about when, how and where) would already be very helpful (for me).
Concerning writing to the database for the counter (on anonymous requests): I don't think this is much overhead, since it is only done after validation (not on rendering) and there is already some writing to the database, e.g. the serialization of $_SESSION, which is done on every render.

I would see the logging to watchdog just as a debug/optimization tool for CAPTCHA. In a real life situation where you have, say, 100 spam submissions a day, it would pollute the log very badly I think. It also spends more resources than the counting thing. I would only use logging for fine tuning the settings of my CAPTCHA and disable it after I see in some way that humans can pass and spam bots are blocked.
If logging is disabled, it can't be used for counting.

in short, to quote myself in #5:

  • always counting the wrong responses. It would be overkill to make this optional with some checkbox, counting is not very resource intensive, it's just about 1 variable.
  • optionally logging the wrong responses to the watchdog log. Because wrong response logging could pollute the log this should be optional.
soxofaan’s picture

new version of patch that only adds the simple counting
(against captcha.module v 1.42.2.34)

soxofaan’s picture

After talking to Rob, I recombined the counting an logging again.
with this patch:

  • always count wrong responses, counter is shown in status report (not on settings page like before)
  • optional: log data about wrong responses to watchdog log

As a sidenote: the counting can't be done by counting the watchdog entries for CAPTCHA because the watchdog log typically gets trimmed. That's why a dedicated database entry in the variables table is used.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.23 KB

Renamed the setting to "Log incorrect responses" with a description of "Have log entries written when an incorrect response is written to a challenge." It looks much prettier:

[X] Log incorrect responses
Have log entries written when an incorrect response is written to a challenge.

Looks good to me. It makes much more sense to have the log in the status report.

soxofaan’s picture

A checkbox with a description is indeed prettier.

But here are some extra iterations (maybe a bit pedantic):
I'm not a native English speaker, but isn't "wrong response" the same as, but easier than "incorrect response"?
I agree that "watchdog log" could be "unobvious" to unexperienced Drupal users and just "log" or "log entries" is better.
The sentence "Have log entries written when an incorrect response is written to a challenge." seems a bit involved to me (e.g. three verbs).

Anyway, this new patch goes for this version:

[X] Log wrong responses.
Log information about wrong responses to your Drupal log.

with "Drupal log" being a link to the watchdog log

RobLoach’s picture

Status: Reviewed & tested by the community » Fixed
Anonymous’s picture

Status: Fixed » Closed (fixed)

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