Posted by bleen18 on April 23, 2010 at 8:58pm
5 followers
| Project: | CAPTCHA |
| Version: | 7.x-1.x-dev |
| Component: | User interface |
| Category: | feature request |
| Priority: | minor |
| Assigned: | pavel.karoukin |
| Status: | closed (fixed) |
| Issue tags: | low-hanging fruit |
Issue Summary
After taking much more time than I'd care to admit to track down why page caching wasn't working properly on the site I am currently working on I came across this in the captcha module:
// Prevent caching of the page with CAPTCHA elements.
// This needs to be done even if the CAPTCHA will be ommitted later:
// other untrusted users should not get a cached page when
// the current untrusted user can skip the current CAPTCHA.
global $conf;
$conf['cache'] = FALSE;While I understand the need for this code and recognize that it is not a bug, I think it would be extremely helpful to put a drupal_set_message upon installing captcha (or maybe at the top of the captcha admin page) that says something along the lines of "Enabling captcha will cause Drupal's page caching not to work on pages that have a captcha challenge element."
Certainly would have saved me {cough} hours of troubleshooting today.
Comments
#1
There is already a rather big paragraph about this on the help page of the CAPTCHA module (see attachment)
Maybe putting the info message on the performance/caching settings page would be more useful than on the CAPTCHA admin page?
(edit: s/persistence/performance/)
#2
@soxofaan: I did see that paragraph, but only after I new what the problem was. I feel like the help section is there to answer questions like "what does this module setting mean" or "how do I work this thing" and not "if you install this, xyz may be effected." Because captcha is easy to use, I had no need to ever visit the help...
As for putting the message on the performance page, I think that is an excellent place to put it.
#3
(tagging)
#4
Just did the same trip as bleen18, spending 4 hours on figuring out why cache not working on some pages. Ended here. I think it's very important to put warning message about this somewhere, because CAPTCHA is very popular module, and i presume that many of users didn't know about this, thus their caching looses efficiency, especially if keep in mind that comment form placement in most cases is in the same page as node.
I've prepared code for this, but i don't know how to make patches, so it will be wonderfull, if someone can take this and make things right way.
Code applied to captcha.module - find
function captcha_form_alterand insert following code just before last "}":
// Set warning message on top of Perfomance Settings page about caching.if ($form_id == 'system_performance_settings') {
$form['captcha_caching']['#weight'] = -10;
$form['captcha_caching'] = array(
'#type' => 'fieldset',
'#title' => t('CAPTCHA Message'),
'#description' => t('<strong class="error">Warning! Pages that contains CAPTCHA can not be cached, even if caching mode set to Normal or Aggressive. Comment form on same page with node view and CAPTCHA leads to entire page be non-cacheable for anonymous users, for example.</strong>'),
'#weight' => -10,
);
}
So, end lines of function code should look like this:
// Get placement in form and insert in form.
$captcha_placement = _captcha_get_captcha_placement($form_id, $form);
_captcha_insert_captcha_element($form, $captcha_placement, $captcha_element);
}
}
// Set warning message on top of Perfomance Settings page about caching.
if ($form_id == 'system_performance_settings') {
$form['captcha_caching']['#weight'] = -10;
$form['captcha_caching'] = array(
'#type' => 'fieldset',
'#title' => t('CAPTCHA Message'),
'#description' => t('<strong class="error">Warning! Pages that contains CAPTCHA can not be cached, even if caching mode set to Normal or Aggressive. Comment form on same page with node view and CAPTCHA leads to entire page be non-cacheable for anonymous users, for example.</strong>'),
'#weight' => -10,
);
}
}
At the end it will look like this: http://img338.imageshack.us/img338/4918/captchao.png
#5
#6
since there is no patch .. this should be "active"
#7
Umm, sorry for this, for some reason I misunderstood meaning of "Need Work" status... Anyway, now we have patch :)
#8
#9
The last submitted patch, captcha_cachingwarning_patch.patch, failed testing.
#10
Trying to fix errors for automated testing...
#11
#12
+++ captcha_patched.module Fri Oct 29 23:57:59 2010@@ -390,6 +390,16 @@ function captcha_form_alter(&$form, $for
}
+ // Set warning message on top of Perfomance Settings page about caching and CAPTCHA. ¶
there is extra whitespace at the end of this line
-----
Also, this should not be a fieldset as there are no fields... so that is not semantically correct. Why not just add this message to the existing description below the "caching mode"
-----
"Warning! Pages that contains CAPTCHA can not be cached, even if caching mode is set to Normal or Aggressive."
Should be
Warning! Pages that contain a CAPTCHA challenge will not be cached, even if caching mode is set to Normal or Aggressive.
Powered by Dreditor.
#13
I agree with the things bleen18 mentioned.
And I also think the warning itself can be a bit shorter.
see patch and screenshot
#14
much gooder :) RTBC
#15
on second thought ... this patch would be even better if we do a check in captcha_install() to see if page caching is already on... if so, we should use drupal_set_message to let the site admin know that this may turn off page caching on some pages. Something like this:
<?phpif (variable_get('cache', 0 > 0) {
drupal_set_message(t('Note that pages containing a CAPTCHA challenge will not be cached even though you have page caching turned on. See admin/settings/performance for details.'));
}
?>
#16
Already committed patch from #13: http://drupal.org/cvs?commit=444454
About #15: good idea, but I don't have the time to do this. Any takers?
#17
here we go
#18
#17 had a stray quote mark
#19
that's fast :) thanks
Two remarks:
1) wouldn't it be cleaner to just do a separate drupal_set_message call (with "warning" level), instead of appending text to the existing "status" message?
2) "Note that pages containing a CAPTCHA challenge will not be cached even though ..." sounds like it is a bug, I would use something like "the CAPTCHA module disables caching of ...", which makes it sound more like a feature
#20
fair enough...
#21
That's even faster :)
Don't you think it should be a "warning" message instead of "status"?
Also, I found the following condition in common.inc:
<?phpvariable_get('cache', CACHE_DISABLED) != CACHE_DISABLED
?>
which is a bit clearer than using "0" imho.
#22
indeed ...
#23
Committed: http://drupal.org/cvs?commit=445164
thanks
to be ported to D7 version
#24
#25
Here we go
#26
As for CACHE_DISABLED -- seems not available in Drupal 7
#27
Hi Pavel,
great work :)
committed to CVS: http://drupal.org/cvs?commit=448662
#28
Automatically closed -- issue fixed for 2 weeks with no activity.
#29
I just discovered that the patch from #13 (http://drupal.org/node/780206#comment-3646784) also needs porting to D7
#30
fixed by http://drupalcode.org/project/captcha.git/commit/47fd59e
#31
ported patch from #13 to D7: http://drupalcode.org/project/captcha.git/commit/47fd59e
#32
Automatically closed -- issue fixed for 2 weeks with no activity.