Allow admin to select Ajax or non-ajax behaviour per Bookmark type

I can think of a couple reasons why users might not want javascript used on certain bookmarks. I'll consider adding this feature for sure. If anyone wants to take a stab at a patch, I'd be glad to add this feature.

Moving to Flag, where I have additional interest in this feature.

I'm actually thinking 3 display modes:

Link Display:
( • ) JavaScript toggle link
(   ) Normal link
(   ) Link with confirmation form

I'll take this on.

I wonder if the admin would like to show non-ajax links in one place, but ajax links in another. So perhaps this setting should be overridable in the themeing layer, but I don't yet see how this should be done. (For Views' 'Ops' field this should be easy: there's an options form).

This may need to be in the module layer for the proper security. Basically I'm thinking that visiting flag/flag/name/node/10 would actually need to present you with a form to confirm the flagging. JS would need to be disabled for this kind of confirmation. This would be the most secure kind of flagging, since it'd be safe from XSRF attacks (unlike our current AJAX linking approach).

Here's an initial pass at this. This adds display options for the link as described in #2. It saves the display options in the "options" database column. Things that are missing/I'm not sure of:

- We should probably provide "Flag confirmation message" and "Unflag confirmation message" as two additional fields. The current hard-coded message can have awkward phrasing at times.

- Is there a better way to reuse our form between the Flag configuration form and the Views configuration form? I'm fine with the way it is now, but it seems like there should be a cleaner way.

As demonstrated in the code, themers can potentially change the behavior of links using the following:

    $flag = flag_get_flag('bookmark');
    $flag->link_type = 'confirm';
    return $flag->theme($is_flagged ? 'unflag' : 'flag', $content_id);

Looks good.

I haven't tested this yet.

(I wonder if we should add the "link_type" as a class on the link, or on the wrapper. Then, for example, some Javascript magic could open the confirmation box in a "thickbox" or whatever. BTW, I know you've been working on a "confirmation box" feature for Drupal's core.)

Do we want to commit this before beta4? I'd prefer not to, but you decide.

$flag->link_type = 'confirm';
return $flag->theme($is_flagged ? 'unflag' : 'flag', $content_id);

Maybe we should make theme() accept an $options array. This is a general problem I was aware of. I was thinking about a user-settable $flag->identifier, a la Panels, to determine further template "suggestions", but having $flag->identifier, or your $flag->link_type, preserved on the object and accidentally passed to the next renderings isn't very good. Or we could have a $flag->clone(), but...

'%flag_title' => $flag->get_title($content_id));

(BTW, that the 'title' can accept tokens is an undocumented feature. It's probably useless, but it's there.)

+ function get_content_title($comment) {
+ return $comment->title;

(I think comments have 'subject', not 'title'.)

I'm in no hurry to add this functionality, but it's something that I think would be useful for "offensive" type flags. Adding a class to the link (or the wrapper) is a good idea, as we might want multiple options for JavaScript behaviors, such as a popup like you suggest or some other kind of confirmation.

But nothing prevents somebody from typing and using the non-confirmation URL, is that right?

So I think flag_page() should refuse to work if the flag is configured to use confirmation.

Similarly, Views' options_form() shouldn't show 'JavaScript toggle' and 'Normal link' if the flag is configured to use confirmation.

I think we should investigate ajax confirmation solutions right now to see what they entail. I'm not familiar with the jQuery pyrotechnics realm so I don't have suggestions at the moment.

So I think flag_page() should refuse to work if the flag is configured to use confirmation.

Absolutely. #315881: More robust flag/unflag links patch implemented this indirectly, where links no longer work without the special tokens. Though I think we could add in extra security here also.

Similarly, Views' options_form() shouldn't show 'JavaScript toggle' and 'Normal link' if the flag is configured to use confirmation.

These options could still be helpful from an administration stand-point. While your user-facing views would probably use the confirmation form, while administrators might not want to be hassled in admin-facing views.

This patch rerolls against the current Dev and adds the following changes:

- The default behavior of the Flag Ops field for Views is not to "inherit" from the link settings.
- Adds two fields for "flag confirmation message" and "unflag confirmation message". These fields are required if using the "Confirmation form" link display option.

Removing a bit of cruft from an earlier patch that's no longer needed.

I only have one remaining concern about this patch generally: should we provide a hook to let modules provide link types?

With this patch as it is now, modules could create their own link types by implementing hook_form_alter() and modifying the flag form. However, they'd also need to remember to form_alter() the views configuration for the Flag Ops. Finally, they'd implement hook_flag_preprocess() and set the $link_href value to whatever is necessary for their module.

My general inclination is that we should provide hook_flag_link_types(), but modules will still need to use the preprocess function to alter the link before it is themed. Thoughts?

We want flag_history/flag_form to be elegant. Doing form_alter/hook_preprocess seems like a hack to me, and it might not be fail-safe. I think your idea, of having a hook_flag_link_types() ia a good one. We could have this structure return callbacks (or a PHP class) for constructing the URL. (I already felt that construting the URL should be moved out to a separate function/method.)

some Javascript magic could open the confirmation box in a "thickbox" or whatever

(Panels 2 has a javascipt popup window. We should check it out, maybe we could "steal" its code.)

We could have this structure return callbacks (or a PHP class) for constructing the URL.

Fantastic idea. This makes it so we don't need to create 2 different hooks, and the user won't have to muck around in the preprocess functions. I'll give this another round this week.

Here's a revised patch. After fiddling with the implementation, I didn't find providing a "url callback" each link type was that helpful. Instead this version introduces 2 hooks: hook_flag_link_types() and hook_flag_link().

function flag_flag_link(&$flag, $action, $content_id) {
  $token = flag_get_token($content_id);
  return array(
    'href' => "flag/". ($flag->link_type == 'confirm' ? 'confirm/' : '') ."$action/$flag->name/$content_id",
    'query' => drupal_get_destination() .'&token='. $token,
  );
}

function flag_flag_link_types() {
  return array(
    'toggle' => t('JavaScript toggle'),
    'normal' => t('Normal link'),
    'confirm' => t('Confirmation form'),
  );
}

As you can see, the implementation is pretty simple for a 3rd party module. Now I'm feeling really good about this patch. Anyone care to give it a review?

Looks good... almost ;)

Small error where you set '#default_value' => $this->link_type, should be '#default_value' => $flag->link_type,.

Attached revised patch.

I tested by installing a new d6 and enabling flag. I then modified and duplicated the default 'bookmark' flag resulting in the following flags, each one having a different link type:
http://img.skitch.com/20081114-1m33mp2qb26mrg1ssdpbn3rkmh.png

Upon clicking each it it performed with the desired effect.

How about d5? ;)

Here's a minor update to D6, removing a call to flag_get_token() that wasn't used. This D5 port should be fully working too, might need another look though.

Very nice. Looks RTBC to me.

Some minor comments (but none hinders commiting the patch as-is):

- The previous 'flag.tpl.php' was self-documenting with regard to the CSS classes. A designer could look at it and figure out the classes. Now that the classes are generated inside the module, we lose this property. I'm not saying we should change anything here (maybe we should, I don't know), just voicing my thoughts.

- Doesn't (!isset($link['html']) || $link['html'] != TRUE) equal empty($link['html'])? I know the former is considered "faster", but it's also less readable. (I'm only mentioning this because I saw a similar pattern in your "default flags" patch, and had to stop to "decipher" it ;-)

- $link['title'] should be check_plain()ed, I think.

- The form in 'flag_handler_field_ops.inc' may trigger a PHP E_ALL warning, I think.

- (Minor "coding style" issues you can disregard.) "They" now tell us to put a space on each side of a string concatenating dot, and I obey. You disobey, so we have two styles in the same function ;) But that's really a minor issue I even feel silly mentioning. Also, for the sake of uniformity, maybe we sould use only 'echo' in the 'tpl' (and it's faster, some say).

- The previous 'flag.tpl.php' was self-documenting with regard to the CSS classes. A designer could look at it and figure out the classes. Now that the classes are generated inside the module, we lose this property. I'm not saying we should change anything here (maybe we should, I don't know), just voicing my thoughts.

Yeah I'm not sure there's much way around this. We need the special classes so we can identify which links are AJAX and which ones are not. Generally just having a full set of classes is helpful. Hopeful having them in a single variable won't put off too many themers.

- Doesn't (!isset($link['html']) || $link['html'] != TRUE) equal empty($link['html'])? I know the former is considered "faster", but it's also less readable. (I'm only mentioning this because I saw a similar pattern in your "default flags" patch, and had to stop to "decipher" it ;-)

Heh, ultimately I ended up with something awfully similar to empty(). I'll switch this out, the code previously had been using opposite logic, and when I switched it I didn't realize it was almost identical to empty().

- $link['title'] should be check_plain()ed, I think.

Yep, I agree.

- The form in 'flag_handler_field_ops.inc' may trigger a PHP E_ALL warning, I think.

I'll turn back on E_ALL and give this a look.

- (Minor "coding style" issues you can disregard.) "They" now tell us to put a space on each side of a string concatenating dot, and I obey. You disobey, so we have two styles in the same function ;) But that's really a minor issue I even feel silly mentioning. Also, for the sake of uniformity, maybe we sould use only 'echo' in the 'tpl' (and it's faster, some say).

The spacing issue is funny. Since the space on either side of the dot is not "standard" until Drupal 7. The Drupal 5/6 standard is a single space on one side. Honestly though, maybe it's time for me to leave the past and switch over my code too. As for print/echo, core uses print so I think sticking with that would be preferable.

I tidied up these remaining issues (other than spacing) and committed. Yay for progress!

Sweet, thanks for this one guys. It'll eliminate the need I had when I created flag_form.module :D YAY!