Patch for small bug in the captcha_admin() function.
The bug does not do anything wrong, it just spills CPU: drupal_get_form('captcha_admin_settings', $form_id) is executed twice.

CommentFileSizeAuthor
#2 captcha_admin_small_bug_2.patch1016 bytessoxofaan
#1 captcha.module_5.patch701 bytesrobloach
captcha_admin_small_bug.patch808 bytessoxofaan

Comments

robloach’s picture

robloach
he/him
commented
StatusFileSize
new captcha.module_5.patch701 bytes

Would removing it make more sense since that same exact line exists right after the switch?

      case 'enable':
        db_query("DELETE FROM {captcha_points} WHERE form_id = '%s'", $form_id);
        db_query("INSERT INTO {captcha_points} (form_id, module, type) VALUES ('%s', NULL, NULL)", $form_id);
        // No drupal_goto() call because we have to go to the captcha adminstration
        // form and not a different destination if that would be present in the
        // URI. So we call this form explicitly. The destination will be preserved
        // so after completing the form, the user will still be redirected.
        //drupal_get_form('captcha_admin_settings', $form_id); <--- REMOVE
        break;
    }
  }
  // no $form_id or legal action given: generate general captcha settings form
  return drupal_get_form('captcha_admin_settings', $form_id);
}

I attached a patch too.

soxofaan’s picture

soxofaan commented
StatusFileSize
new captcha_admin_small_bug_2.patch1016 bytes

Would removing it make more sense since that same exact line exists right after the switch?

I wouldn't do that for clarity of the code in the switch construct. The statements are indeed the same, but the context is a bit different (look for example at the comments).

Actually, it would be even better like this:

function captcha_admin($form_id='', $op='') {
  // if $form_id and action $op given: do the action
  if ($form_id) {
    switch ($op) {
      ...
      case 'enable':
        db_query("DELETE FROM {captcha_points} WHERE form_id = '%s'", $form_id);
        db_query("INSERT INTO {captcha_points} (form_id, module, type) VALUES ('%s', NULL, NULL)", $form_id);
        // No drupal_goto() call because we have to go to the captcha adminstration
        // form and not a different destination if that would be present in the
        // URI. So we call this form explicitly. The destination will be preserved
        // so after completing the form, the user will still be redirected.
        return drupal_get_form('captcha_admin_settings', $form_id);
        break;
    }
  }
  // no $form_id or legal action given: generate general captcha settings form
  return drupal_get_form('captcha_admin_settings');
}

(now the statements are different)

see attached patch

soxofaan’s picture

soxofaan commented

This issue will also be solved when the patch from http://drupal.org/node/169853 will be committed

robloach’s picture

robloach
he/him
commented
Status: Reviewed & tested by the community » Closed (duplicate)

Moved over there... Thanks!