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.

CommentFileSizeAuthor
#25 captcha.install.message.patch702 bytespavel.karoukin
#22 captcha-message.patch785 bytesbleen
#20 captcha-message.patch757 bytesbleen
#18 captcha-medssage.patch903 bytesbleen
#17 captcha-message.patch904 bytesbleen
#13 780206_caching_warning.patch763 bytessoxofaan
#13 performance-captcha-and-caching.jpg88.92 KBsoxofaan
#10 captcha_cachingwarning.patch815 bytesTreidge
#7 captcha_cachingwarning_patch.patch834 bytesTreidge
#1 Screen shot 2010-04-24 at 00.12.39.png190.25 KBsoxofaan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

soxofaan’s picture

soxofaan CreditAttribution: soxofaan commented
FileSize
Screen shot 2010-04-24 at 00.12.39.png190.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/)

bleen’s picture

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.

soxofaan’s picture

soxofaan CreditAttribution: soxofaan commented
Issue tags: +low-hanging fruit

(tagging)

Treidge’s picture

Treidge CreditAttribution: Treidge commented

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

Treidge’s picture

Treidge CreditAttribution: Treidge commented
Status: Active » Needs work
bleen’s picture

bleen CreditAttribution: bleen commented
Status: Needs work » Active

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

Treidge’s picture

Treidge CreditAttribution: Treidge commented
FileSize
captcha_cachingwarning_patch.patch834 bytes

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

Treidge’s picture

Treidge CreditAttribution: Treidge commented
Status: Active » Needs review

Status: Needs review » Needs work

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

Treidge’s picture

Treidge CreditAttribution: Treidge commented
FileSize
captcha_cachingwarning.patch815 bytes

Trying to fix errors for automated testing...

Treidge’s picture

Treidge CreditAttribution: Treidge commented
Status: Needs work » Needs review
bleen’s picture

bleen CreditAttribution: bleen commented
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.

soxofaan’s picture

soxofaan CreditAttribution: soxofaan commented
Status: Needs work » Needs review
FileSize
performance-captcha-and-caching.jpg88.92 KB
780206_caching_warning.patch763 bytes

I agree with the things bleen18 mentioned.

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

see patch and screenshot

bleen’s picture

bleen CreditAttribution: bleen commented
Status: Needs review » Reviewed & tested by the community

much gooder :) RTBC

bleen’s picture

bleen CreditAttribution: bleen commented
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:

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.'));
}
soxofaan’s picture

soxofaan CreditAttribution: soxofaan commented

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?

bleen’s picture

bleen CreditAttribution: bleen commented
Status: Needs work » Needs review
FileSize
captcha-message.patch904 bytes

here we go

bleen’s picture

bleen CreditAttribution: bleen commented
FileSize
captcha-medssage.patch903 bytes

#17 had a stray quote mark

soxofaan’s picture

soxofaan CreditAttribution: soxofaan commented
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

bleen’s picture

bleen CreditAttribution: bleen commented
Status: Needs work » Needs review
FileSize
captcha-message.patch757 bytes

fair enough...

soxofaan’s picture

soxofaan CreditAttribution: soxofaan commented

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:
variable_get('cache', CACHE_DISABLED) != CACHE_DISABLED
which is a bit clearer than using "0" imho.

bleen’s picture

bleen CreditAttribution: bleen commented
FileSize
captcha-message.patch785 bytes

indeed ...

soxofaan’s picture

soxofaan CreditAttribution: soxofaan commented
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

pavel.karoukin’s picture

pavel.karoukin CreditAttribution: pavel.karoukin commented
Assigned: Unassigned » pavel.karoukin
pavel.karoukin’s picture

pavel.karoukin CreditAttribution: pavel.karoukin commented
Status: Patch (to be ported) » Needs review
FileSize
captcha.install.message.patch702 bytes

Here we go

pavel.karoukin’s picture

pavel.karoukin CreditAttribution: pavel.karoukin commented

As for CACHE_DISABLED -- seems not available in Drupal 7

soxofaan’s picture

soxofaan CreditAttribution: soxofaan commented
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.

soxofaan’s picture

soxofaan CreditAttribution: soxofaan commented
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

soxofaan’s picture

soxofaan CreditAttribution: soxofaan commented
Status: Patch (to be ported) » Fixed

fixed by http://drupalcode.org/project/captcha.git/commit/47fd59e

soxofaan’s picture

soxofaan CreditAttribution: soxofaan commented

ported patch from #13 to D7: http://drupalcode.org/project/captcha.git/commit/47fd59e

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

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

Issue tags: +low-hanging fruit

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.