I've added a hook_captcha_placement_alter() that can be used to customize placement of captcha elements. This may be useful in situations where the form is complex and the captcha module's default placement needs to be overridden.

I've also added documentation showing how to add a CAPTCHA to a webform using the forms API. This would address the issue raised in Document how to add a CAPTCHA programmatically, but since my patch is against Drupal 7, I'm creating a separate ticket here for it.

Files: 
CommentFileSizeAuthor
#8 1377380_hook_captcha_placement_map-01.patch4.48 KBsoxofaan
PASSED: [[SimpleTest]]: [MySQL] 718 pass(es).
[ View ]
#6 Screen shot 2011-12-23 at 17.30.00.png179 KBsoxofaan
#1 add_captcha_programatically-1377380-1.patch1.8 KBSheldon Rampton
PASSED: [[SimpleTest]]: [MySQL] 720 pass(es).
[ View ]

Comments

StatusFileSize
new1.8 KB
PASSED: [[SimpleTest]]: [MySQL] 720 pass(es).
[ View ]

Here's the patch.

Status:Active» Needs review

Status:Needs review» Needs work

Hi,

Cool! Thanks for the work. Also nice you thought to update the API documentation :)

One remark: you placed the hook_alter trigger after caching of the placement map (variable_set('captcha_placement_map_cache', $placement_map);). Any reason for this? Wouldn't it be better for performance to cache the altered placement map?

Note that I already provided place to put the hook:
around line 245 in captcha.inc:

<?php
if ($placement_map === NULL) {
     
// If second level cache missed: start from a fresh placement map.
     
$placement_map = array();
     
// This is the place to (pre)fill the placement map with some hard coded default entries.
      // However, probably all Drupal core forms are correctly handled with the best effort
      // guess based on the 'actions' element (see below). So not much to here at the moment.
      // TODO: provide a hook here so other modules can inject placements here?
      // TODO: also make the placement 'overridable' from the admin UI?
   
}
?>

Hm, well for starters the way you're caching the placement map is by setting a Drupal variable, which means it is a cache that never expires, and there is no settings page or other hook that enables it to be altered. This means that if the captcha module sets a value for a particular form ID, thereafter it will always skip over the place in your code where you are suggesting inserting a drupal_alter function. Effectively, therefore, it will still be impossible for the hook to modify the placement once the captcha module has defined it.

Also, the hook I wrote modifies placement for a single form ID, whereas placing the hook in the location you suggested would require modifying the entire placement map. I think in practice people who want to have this kind of hook will likely only want to modify placement on one or two forms on their website, so modifying the $placement array rather than the $placement_map array seems to me like an easier hook to understand and implement.

Anyway, that's my thinking.

Hi Sheldon, thanks for your reply.

About the caching: there is a button on the CAPTCHA settings page to flush the placement cache, precisely for this (see attachment).
The caching is there because CAPTCHA placement is not something that changes very often once a website is set up. It would be a waste to do all the form array traversing/processing every time for every CAPTCHA protected form, when the result is probably the same as last time.
Conceptually speaking, I think that the result of placement altering by other modules should be included in this caching.
You might have a point that cache invalidation is currently not optimal (only a button on the settings page). Alternatives: invalidate it when new modules are enabled/disabled, always invalidate it when there is CAPTCHA configuration activity (e.g. the CAPTCHA settings form is submitted), ...

Also, the hook I wrote modifies placement for a single form ID, whereas placing the hook in the location you suggested would require modifying the entire placement map. I think in practice people who want to have this kind of hook will likely only want to modify placement on one or two forms on their website, so modifying the $placement array rather than the $placement_map array seems to me like an easier hook to understand and implement.

I don't think there would be a lot of difference.
You propose:

<?php
function hook_captcha_placement_alter(&$placement, $form_id) {
  if (
$form_id == 'appform_form') {
   
$placement['path'][1] = 'fieldset3';
   
$placement['key'] = 'submit';
   
$placement['weight'] = NULL;
  }
  else if (
$form_id == 'another_form') {
   
// ...
 
}
}
?>

While I was thinking along the lines of

<?php
function hook_captcha_placement_map_alter(&$placement_map) {
 
$placement_map['my_fancy_form'] = array(
   
'path' => array('fieldset3'),
   
'key' => 'submit',
   
'weight' => NULL,
  );
 
$placement_map['another_form'] = array(
   
// ..
 
);
}
?>

Which looks even easier to me (no need for if's, just direct assignments).

StatusFileSize
new179 KB

(and here is the attachment)

Hm, that's interesting. I didn't understand how your caching worked. I guess I didn't read the settings page very closely. Also, when I see the word "caching" I think of Drupal's cache tables and expect to be able to clear caches using "flush all caches," "flush theme cache," etc. I wonder if it would be better to use a phrase such as "reset to defaults" to avoid confusion. In any case, I see the logic to your suggested changes. Do you want to reroll the patch, or should I?

Status:Needs work» Needs review
StatusFileSize
new4.48 KB
PASSED: [[SimpleTest]]: [MySQL] 718 pass(es).
[ View ]

Hi, I worked a bit on this and added a hook_captcha_placement_map() hook.

I also added clearing of the placement cache when a new module is enabled.

Status:Needs review» Fixed

ok, decided to commit this one
will be in next release

Status:Fixed» Closed (fixed)

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

Version:7.x-1.x-dev» 7.x-1.0

Note. I had to disable then enable captcha after adding this hook. Normal drupal clear all cache (drush cc all) did not work.