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.

Files: 
CommentFileSizeAuthor
#25 captcha.install.message.patch702 bytespavel.karoukin
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#22 captcha-message.patch785 bytesbleen18
PASSED: [[SimpleTest]]: [MySQL] 566 pass(es).
[ View ]
#20 captcha-message.patch757 bytesbleen18
PASSED: [[SimpleTest]]: [MySQL] 566 pass(es).
[ View ]
#18 captcha-medssage.patch903 bytesbleen18
PASSED: [[SimpleTest]]: [MySQL] 566 pass(es).
[ View ]
#17 captcha-message.patch904 bytesbleen18
PASSED: [[SimpleTest]]: [MySQL] 4 pass(es).
[ View ]
#13 780206_caching_warning.patch763 bytessoxofaan
PASSED: [[SimpleTest]]: [MySQL] 566 pass(es).
[ View ]
#13 performance-captcha-and-caching.jpg88.92 KBsoxofaan
#10 captcha_cachingwarning.patch815 bytesTreidge
PASSED: [[SimpleTest]]: [MySQL] 566 pass(es).
[ View ]
#7 captcha_cachingwarning_patch.patch834 bytesTreidge
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in captcha_cachingwarning_patch.patch.
[ View ]
#1 Screen shot 2010-04-24 at 00.12.39.png190.25 KBsoxofaan

Comments

StatusFileSize
new190.25 KB

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/)

@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.

Issue tags:+low-hanging fruit

(tagging)

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_alter

and 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

Status:Active» Needs work

Status:Needs work» Active

since there is no patch .. this should be "active"

StatusFileSize
new834 bytes
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in captcha_cachingwarning_patch.patch.
[ View ]

Umm, sorry for this, for some reason I misunderstood meaning of "Need Work" status... Anyway, now we have patch :)

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, captcha_cachingwarning_patch.patch, failed testing.

StatusFileSize
new815 bytes
PASSED: [[SimpleTest]]: [MySQL] 566 pass(es).
[ View ]

Trying to fix errors for automated testing...

Status:Needs work» Needs review

Status:Needs review» Needs work

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new88.92 KB
new763 bytes
PASSED: [[SimpleTest]]: [MySQL] 566 pass(es).
[ View ]

I agree with the things bleen18 mentioned.

And I also think the warning itself can be a bit shorter.

see patch and screenshot

Status:Needs review» Reviewed & tested by the community

much gooder :) RTBC

Status:Reviewed & tested by the community» Needs work

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:

<?php
if (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.'));
}
?>

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?

Status:Needs work» Needs review
StatusFileSize
new904 bytes
PASSED: [[SimpleTest]]: [MySQL] 4 pass(es).
[ View ]

here we go

StatusFileSize
new903 bytes
PASSED: [[SimpleTest]]: [MySQL] 566 pass(es).
[ View ]

#17 had a stray quote mark

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new757 bytes
PASSED: [[SimpleTest]]: [MySQL] 566 pass(es).
[ View ]

fair enough...

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:

<?php
variable_get
('cache', CACHE_DISABLED) != CACHE_DISABLED
?>

which is a bit clearer than using "0" imho.

StatusFileSize
new785 bytes
PASSED: [[SimpleTest]]: [MySQL] 566 pass(es).
[ View ]

indeed ...

Version:6.x-2.x-dev» 7.x-1.x-dev
Component:Miscellaneous» User interface
Status:Needs review» Patch (to be ported)

Committed: http://drupal.org/cvs?commit=445164
thanks

to be ported to D7 version

Assigned:Unassigned» pavel.karoukin

Status:Patch (to be ported)» Needs review
StatusFileSize
new702 bytes
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

Here we go

As for CACHE_DISABLED -- seems not available in Drupal 7

Status:Needs review» Fixed

Hi Pavel,

great work :)
committed to CVS: http://drupal.org/cvs?commit=448662

Status:Fixed» Closed (fixed)

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

Status:Closed (fixed)» Patch (to be ported)

I just discovered that the patch from #13 (http://drupal.org/node/780206#comment-3646784) also needs porting to D7

Status:Patch (to be ported)» Fixed

Status:Fixed» Closed (fixed)
Issue tags:-low-hanging fruit

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