Posted by kylethaynes on June 5, 2011 at 8:16am
2 followers
| Project: | CAPTCHA |
| Version: | 7.x-1.x-dev |
| Component: | User interface |
| Category: | feature request |
| Priority: | normal |
| Assigned: | kylethaynes |
| Status: | active |
| Issue tags: | low-hanging fruit |
Issue Summary
I've tested this on both dev and alpha3 (after dev was uninstalled and deleted). I've tried setting image-captcha to the default captcha and also setting it directly on all forms. When accessing /user to login, the captcha that appears is always the math captcha regardless of settings. I've also tried flushing the captcha form location cache and there were no changes.
Image captcha appears correctly in the examples section and image captcha tab, but will not appear on the /user login form.
Comments
#1
I guess your site is in maintenance mode.
From image_captcha,module:
<?php// In maintenance mode, the image CAPTCHA does not work because the request
// for the image itself won't succeed (only ?q=user is permitted for
// unauthenticated users). We fall back to the Math CAPTCHA in that case.
global $user;
if (variable_get('maintenance_mode', 0) && $user->uid == 0) {
return captcha_captcha('generate', 'Math');
}
?>
#2
It is in maintenance mode. Sorry about that. Thank you for helping me out.
#3
Closing.
#4
No problem.
Maybe it's not a bad idea to put a message on the image captcha admin page about this, when in maintenance mode.
Something like
If someone knows a better wording, be welcome to improve it.
Also marking it as low-hanging fruit for candidate-co-maintainers.
#5
for reference, some duplicate/related issues:
#133548: Image not available when site is offline for maintenance
#407074: Image CAPTCHA in offline mode: math challenge displayed instead (feature, not a bug)
#983254: Captcha image not visible on user login page in site maintenance (offline) mode
#6
I wrote some code to implement the warning feature. I included a patch as well, but I'm not sure it'll work correctly since this is my first time using git to submit a patch. Please tell me if I didn't create the patch correctly.
Add to image_captcha.module.
/**
* Implements hook_form_FORM_ID_alter() for the captcha_admin_settings form.
*
* This function displays a warning message if the Image CAPTCHA is being used while
* maintenance mode is enabled.
*/
function image_captcha_form_captcha_admin_settings_alter(&$form, &$form_state, $form_id) {
$check_for_old_message = FALSE;
$warning_message = t('Your site is currently in maintenance mode and an Image CAPTCHA is being used with one or more forms! While your site is in maintenance mode, an Image CAPTCHA cannot be displayed to anonymous users. Any Image CAPTCHAs will be substituted with Math CAPTCHAs for anonymous users.');
if (variable_get('maintenance_mode', 0)) {
$target_types = array();
switch (variable_get('captcha_default_challenge', 'captcha/Math')) {
case 'image_captcha/Image':
$target_types[] = 'default';
default:
$target_types[] = 'Image';
}
$result = db_select('captcha_points', 'cp')
->fields('cp', array('captcha_type'))
->condition('captcha_type', $target_types)
->range(0, 1)
->execute()
->fetchAll(PDO::FETCH_ASSOC);
if (!empty($result)) {
drupal_set_message($warning_message, 'warning', FALSE);
}
else {
$check_for_old_message = TRUE;
}
}
else {
$check_for_old_message = TRUE;
}
// The warning message would show erroneously after clicking the submit button if an Image CAPTCHA was
// activated on the previous page and all the Image CAPTCHAs had just been deactivated by a submitted form.
// After refreshing the page again, the warning message would disappear. The solution is to clear the old message
// out of the drupal message array.
if ($check_for_old_message) {
$messages_wrapper = drupal_get_messages('warning', TRUE);
if (!empty($messages_wrapper)) {
$messages = $messages_wrapper['warning'];
$old_message = array_search($warning_message, $messages);
if (!($old_message === FALSE)) {
unset($messages[$old_message]);
}
// The remaining warning messages should go back into the drupal message array.
if (!empty($messages)) {
foreach ($messages as $message) {
drupal_set_message($message, 'warning', FALSE);
}
}
}
}
}
#7
Tests I did on this code:
1. single Image CAPTCHA on/off.
2. multiple Image CAPTCHA on/off.
3. default CAPTCHA = Image for single and multiple.
4. default CAPTCHA = Math for single and multiple.
5. combined default CAPTCHA = Image + manually set Image CAPTCHA.
5. maintenance mode with and without Image CAPTCHA activated.
The code behaved correctly under these conditions using Drupal 7.2 with Captcha 7.x-1.x-dev as of June 7, 2011.
#8
hi kylethaynes,
Thanks for the patch, it applied without problems.
But I had something much simpler in mind (after all, it will be visible very rarely).
I would just always show the warning in maintenance mode, regardless whether there are image CAPTCHA's enabled. This avoids a lot of complexity and makes the feature more predictable to the end user.
Also, in your patch you add the warning to the general CAPTCHA admin page, which requires a form_alter hook. If we just add it to the Image CAPTCHA admin page, we don't need a form_alter hook, which further limits the required code.
See attached patch for what I had in mind
what do you think?
#9
While making my original code, I tried the solution that you are proposing. I liked the more complex solution because I thought it was more appropriate to have a warning that reacted specifically to the unexpected behavior when it occurred where the options that would turn the the unexpected behavior on and off were located. My code does a quick and optimized database search for any activated options that might require an Image->Math CAPTCHA switch. Since I'm using hook_form_FORM_ID_alter, the code is optimized to run only with the captcha_admin_settings form. I also corrected the bug where an outdated and incorrect warning_message (stuck in the drupal message array for some reason) would display on the next form after the CAPTCHA settings had been changed to a use case which did not activate the unexpected behavior. The code tracks whether or not it needs to run a check on the warning messages in the drupal message array. The code has also been tested by me in the relevant test cases as mentioned in the post above.
I've already written code that handles the more complex behavior in an optimized fashion and tested it. I've commented the code fairly well and used self-explanatory names. I'm not the maintainer, so I obviously don't have any right to decide whether the code is used or not. Personally, I like this solution better and that's why I went to all the extra trouble.
If you feel it would be better, you can use the code I wrote on the image_captcha page. If you feel it would manage the complexity better, it could be split off into a helper function.
My personal opinion is that it is more appropriate to have it on the general settings page, since the general CAPTCHA admin page will be where people will change the options that will activate the unexpected behavior, and it is where people will check first to see if they accidentally set the CAPTCHA to Math after seeing the unexpected behavior that converts Image CAPTCHAs to Math CAPTCHAs). I intentionally put in the conditional behavior to minimize the warning message display to only when it was appropriate and necessary.
#10
By the way, this is driving me crazy... I know that you're supposed to use the issue number in the first part of the patch name, but I'm wasn't able to find it (1179062 I'm assuming by your patch name). I wanted to do everything using the appropriate conventions, but I'm still not able to find the location of that mysterious issue number.
Hmm, actually, now that I look at it... If you're interested in using my code, I think I might actually put in some more comments. The top half of the code looks rather empty to me.
#11
I went ahead and created two example helper functions of possible replacement warning behavior. One detects replacement behavior, while the other does a simple check for maintenance mode. Tell me which behavior your prefer (Option 1 or Option 2) and whether you want the message on the general CAPTCHA admin page or the Image CAPTCHA page, and I'll put together a patch for you and test it.
Option 1:
Code that detects replacement behavior and shows warning message.
/**
* Helper function to detect Image -> Math CAPTCHA replacement behavior for anonymous users
* and show a warning message.
*
* This function displays a warning message if and only if the Image CAPTCHA is being used while
* maintenance mode is enabled. This warns the user that their Image CAPTCHAs will be replaced with Math
* CAPTCHAs specifically when that behavior occurs.
*/
function _image_captcha_detect_replacement_and_show_warning() {
// Sometimes the old warning message gets stuck in the queue, so the message will display when
// the replacement behavior isn't occurring. This variable tracks whether it is necessary to check
// for a lingering message.
$check_for_old_message = FALSE;
$warning_message = t('Your site is currently in maintenance mode and an Image CAPTCHA is being used with one or more forms! While your site is in maintenance mode, an Image CAPTCHA cannot be displayed to anonymous users. Any Image CAPTCHAs will be substituted with Math CAPTCHAs for anonymous users.');
// Maintenance mode must be activated for the behavior to occur.
if (variable_get('maintenance_mode', 0)) {
$target_types = array();
// Only check for the default CAPTCHA setting in the database if the default setting is the Image CAPTCHA.
switch (variable_get('captcha_default_challenge', 'captcha/Math')) {
case 'image_captcha/Image':
$target_types[] = 'default';
default:
$target_types[] = 'Image';
}
// Find all the instances of relevant CAPTCHA use in the database, but only return one
// instance, since that is the minimum neccessary to prove that the message should be shown.
// This optimizes the size of the database call to only the necessary data that we need.
$result = db_select('captcha_points', 'cp')
->fields('cp', array('captcha_type'))
->condition('captcha_type', $target_types)
->range(0, 1)
->execute()
->fetchAll(PDO::FETCH_ASSOC);
// Display the warning message under the condition that an Image CAPTCHA is being used while maintenance
// mode is activated.
if (!empty($result)) {
drupal_set_message($warning_message, 'warning', FALSE);
}
else {
// If an Image CAPTCHA isn't in the database then we should check for a lingering warning message.
$check_for_old_message = TRUE;
}
}
else {
// If maintenance mode isn't activated then we should check for a lingering warning message.
$check_for_old_message = TRUE;
}
// The warning message would show erroneously after clicking the submit button if an Image CAPTCHA was
// activated on the previous page and all the Image CAPTCHAs had just been deactivated by a submitted form.
// After refreshing the page again, the warning message would disappear. The solution is to clear the old message
// out of the drupal message array.
if ($check_for_old_message) {
// Clear and return only the warning messages.
$messages_wrapper = drupal_get_messages('warning', TRUE);
// Search for and remove the incorrect warning message if it exists.
if (!empty($messages_wrapper)) {
$messages = $messages_wrapper['warning'];
$old_message = array_search($warning_message, $messages);
if (!($old_message === FALSE)) {
unset($messages[$old_message]);
}
// The remaining warning messages should go back into the drupal message array.
if (!empty($messages)) {
foreach ($messages as $message) {
drupal_set_message($message, 'warning', FALSE);
}
}
}
}
}
Option 2:
Code to detect maintenance mode and show warning.
/**
* Helper function to detect maintenance mode and warn the admin that their Image CAPTCHAs will be replaced
* with Math CAPTCHAs for anonymous users.
*
* This function displays a warning message if maintenance mode is activated.
*/
function _image_captcha_show_replacement_warning() {
$warning_message = t('Your site is currently in maintenance mode! While your site is in maintenance mode, an Image CAPTCHA cannot be displayed to anonymous users. Any Image CAPTCHAs will be substituted with Math CAPTCHAs for anonymous users.');
if (variable_get('maintenance_mode', 0)) {
drupal_set_message($warning_message, 'warning', FALSE);
}
}
I also noticed on the git page that you recently submitted to branches 7.x-1.x and master. Which branch should development patches be built with?
#12
Hi kylethaynes,
Some remarks, in random order:
First, it is not really required to use the issue number, at most recommended. Second, you'll find the issue number nicely disguised in the url of this page. Probably one of those things that come naturally when you know it, but are black magic otherwise ;)
<?php$target_types = array();
// Only check for the default CAPTCHA setting in the database if the default setting is the Image CAPTCHA.
switch (variable_get('captcha_default_challenge', 'captcha/Math')) {
case 'image_captcha/Image':
$target_types[] = 'default';
default:
$target_types[] = 'Image';
}
?>
IMHO this is a good example of the evilness of switch statements :) I had to stare pretty long at this chunk before I got it. I think it's better to something like this:
<?php$target_types = array('image_captcha/Image');
if (variable_get('captcha_default_challenge', 'captcha/Math') === 'image_captcha/Image') {
$target_types[] = 'default';
}
?>
count($form_state['input']) == 0).Working against the 7.x-1.x branch is ok.
I hope this answers most of your questions.
#13
Thank you for helping me out.
What do you think about having a warning message pop up once per maintenance mode activation, and the message put on the image captcha or the general captcha admin page (which one do you like better?). That way it wouldn't be too annoying for the user. The code is pretty simple and takes up around 10 lines (if you don't count comments, spacing and word wrapping).
/**
* Helper function to detect Image -> Math CAPTCHA replacement behavior for anonymous users
* and show a warning message.
*/
function _image_captcha_show_replacement_warning() {
$warning_message = t('While your site is in maintenance mode, an Image CAPTCHA cannot be displayed to anonymous users. Any Image CAPTCHAs will be substituted with Math CAPTCHAs for anonymous users.');
if (variable_get('maintenance_mode', 0))) {
if(!(variable_get('captcha_warning_shown', FALSE)) {
drupal_set_message($warning_message, 'warning', FALSE);
variable_set('captcha_warning_shown', TRUE);
}
}
else {
variable_set('captcha_warning_shown', FALSE);
}
}
(This code is just for demonstration. I haven't formatted or tested it or anything.)