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.
Comment | File | Size | Author |
---|---|---|---|
#25 | captcha.install.message.patch | 702 bytes | pavel.karoukin |
#22 | captcha-message.patch | 785 bytes | bleen |
#20 | captcha-message.patch | 757 bytes | bleen |
#18 | captcha-medssage.patch | 903 bytes | bleen |
#17 | captcha-message.patch | 904 bytes | bleen |
Comments
Comment #1
soxofaan CreditAttribution: soxofaan commentedThere 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/)
Comment #2
bleen CreditAttribution: bleen commented@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.
Comment #3
soxofaan CreditAttribution: soxofaan commented(tagging)
Comment #4
Treidge CreditAttribution: Treidge commentedJust 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_alter
and insert following code just before last "}":
So, end lines of function code should look like this:
At the end it will look like this: http://img338.imageshack.us/img338/4918/captchao.png
Comment #5
Treidge CreditAttribution: Treidge commentedComment #6
bleen CreditAttribution: bleen commentedsince there is no patch .. this should be "active"
Comment #7
Treidge CreditAttribution: Treidge commentedUmm, sorry for this, for some reason I misunderstood meaning of "Need Work" status... Anyway, now we have patch :)
Comment #8
Treidge CreditAttribution: Treidge commentedComment #10
Treidge CreditAttribution: Treidge commentedTrying to fix errors for automated testing...
Comment #11
Treidge CreditAttribution: Treidge commentedComment #12
bleen CreditAttribution: bleen commentedthere 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.
Comment #13
soxofaan CreditAttribution: soxofaan commentedI agree with the things bleen18 mentioned.
And I also think the warning itself can be a bit shorter.
see patch and screenshot
Comment #14
bleen CreditAttribution: bleen commentedmuch gooder :) RTBC
Comment #15
bleen CreditAttribution: bleen commentedon 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:
Comment #16
soxofaan CreditAttribution: soxofaan commentedAlready 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?
Comment #17
bleen CreditAttribution: bleen commentedhere we go
Comment #18
bleen CreditAttribution: bleen commented#17 had a stray quote mark
Comment #19
soxofaan CreditAttribution: soxofaan commentedthat'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
Comment #20
bleen CreditAttribution: bleen commentedfair enough...
Comment #21
soxofaan CreditAttribution: soxofaan commentedThat'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:
variable_get('cache', CACHE_DISABLED) != CACHE_DISABLED
which is a bit clearer than using "0" imho.
Comment #22
bleen CreditAttribution: bleen commentedindeed ...
Comment #23
soxofaan CreditAttribution: soxofaan commentedCommitted: http://drupal.org/cvs?commit=445164
thanks
to be ported to D7 version
Comment #24
pavel.karoukin CreditAttribution: pavel.karoukin commentedComment #25
pavel.karoukin CreditAttribution: pavel.karoukin commentedHere we go
Comment #26
pavel.karoukin CreditAttribution: pavel.karoukin commentedAs for CACHE_DISABLED -- seems not available in Drupal 7
Comment #27
soxofaan CreditAttribution: soxofaan commentedHi Pavel,
great work :)
committed to CVS: http://drupal.org/cvs?commit=448662
Comment #29
soxofaan CreditAttribution: soxofaan commentedI just discovered that the patch from #13 (http://drupal.org/node/780206#comment-3646784) also needs porting to D7
Comment #30
soxofaan CreditAttribution: soxofaan commentedfixed by http://drupalcode.org/project/captcha.git/commit/47fd59e
Comment #31
soxofaan CreditAttribution: soxofaan commentedported patch from #13 to D7: http://drupalcode.org/project/captcha.git/commit/47fd59e