Improve API to allow implementing modules easier access to flag

Yes, my preference would be to not have this included with Flag itself directly. However if any API changes are necessary to support this flag type then I'd be open to including them. Right now you can already create your own link types through hook_flag_link_types() and hook_flag_link(), there's no need to include this code directly in flag.module as far as I can tell.

Nice! The only thing I see in here is that you're only adding in $attributes if a class is set:

+  // Add class attributes, if they exist in the link type.
+  if (!empty($link['attributes']['class'])) {
+    $attributes = array('class' => $link_types[$flag->link_type]['attributes']['class']);
+  }

Why wouldn't you just check for $link['attributes'] instead? Other properties (like "rel") might still be used here for various purposes. Also, it looks like $attributes is going to be undefined unless a class is specified, causing a PHP notice when $attributes is used later.

(For #871064: Making flaggings fieldable we will obviously want the option to show the flagging form in a dialog (this functionality could be in a separate module).)

Seems like the patch on #5 needs some work. The things that jumped out at me:
- tabs rather than spaces used for indenting
- 'js callback' => FALSE, is duplicated

Amitai, let's wait till #871064: Making flaggings fieldable advances a bit because I'm dealing there with a similar issue.

While cleaning the issue queue (well, trying to) I stumbled upon #587040: New Flag type, CTools Confirm. Amitai, should we mark that other issue a duplicate of this one?

(I downloaded the patches and will look at them later today.)

Amitai, here's some feedback:

There are two parts to your patch. One I consider bad, the other good.

Part 1: Making it possible to include 'href', 'attributes' and 'token' in a link-type definition.

I don't see the merit in this.

You say that now programmers won't "even need to implement [their] own hook_flag_link".

But how is the proposed feature much better?

Look for example at 'href' (which should more accurately be named 'base href'): it only works if it's at the beginning of the URL. So, for example, I won't be able to use this feature for #871064: Making flaggings fieldable because there I tuck the '/nojs' at the end of the URL, not at the beginning.

Indeed, when you arrive at the CSS/JS handling you notice this disadvantage of static properties and you resort to callbacks. But here I think you prove my point: Why not simply put the logic (the code) in hook_flag_link() ?

Part 2: Making the CSS/JS inclusion better.

That's good.

btw, why is drupal_add_js and drupal_add_css in the tpl file?

I think that's an inheritance from the D5 days where you couldn't override either the CSS or the JS so to make life easier for themers it was put in the tpl. But I believe we should remove it from there. Moreover, I see that 'dman' warns in api.drupal.org/drupal_add_css that calling drupal_add_css() from a tpl is "incompatible with css caching".

We could add two flags to the link-type definition: 'use standard css' and 'use standard js'. These will tell us whether to load the standard flag.css and flag.js for this link.

But instead of putting 'css' and 'js' slots in the link-type definition, why not return an #attached from hook_flag_link()? For D6 we can write a very minimal support for this structure (like the Vertical Tabs module does). On the other hand, one can ask a good question: Why bother? After all, modules can add their CSS/JS in hook_flag_link() or in a theme preprocess. (One answer to that could be: Our template_preprocess_flag() also includes a mechanism (the $first_time variable) that prevents inclusion multiple times so this is more efficient. On the other hand, modules could mimic this mechanism...)

==========

(Of course, the integration with the Dialog API module (or similar) is a an excellent idea. It's a must in 2010! Unfortunately, our "confirmation form", even when ajaxified, is not very satisfying.)

Maybe we should do it on D7 first and back port it D6?

I think first we need to do some cleanup on our existing code. The way our own CSS/JS is loaded isn't very nice. I opened a new issue:

#956800: Cleanup the CSS / JS inclusion

That #956800 issue is not supposed to address the issues you mention here (we'll revisit them afterwards), that's why I opened a new issue.

Marked #956800: Cleanup the CSS / JS inclusion a duplicate of this issue.

Amitai, perhaps there's some misunderstanding. I thought, and still think, that #956800: Cleanup the CSS / JS inclusion was a very good idea. The purpose of #956800 was cleanup only, not to introduce any new feature(s). If you support #956800 as-is, let me know.

[Comment deleted: I accidentally typed my comment here instead of the other issue. IGNORE THIS EMPTY COMMENT.]

#871064: Making flaggings fieldable is now in.

Are any of the people working on this feature still interested? Is it still relevant on 3.x?

Drupal 7 is EOL. No more new features will be added. Issue will be closed, but patches are still here