Have a 'flag.tpl.php'

(I should do some benchmarking: see if include()ing a file is much slower than having the markup in the source. If it isn't much slower, we could use a template outright.)

I have the benchmark results ready. I'll publish them soon. I benchmarked against our current code too.

Here are the results.

The bottom line: using an external template file will make our code about 4% slower than it is today.

DETAILS:

I benchmarked three versions of the theming function (theme_flag_link):

"current": our current code.
"insource": markup embedded in the source, as in the patch above.
"template": the markup is moved to a template file and then include()'ed, similar to what D6 does.

I benchmarked using this code, and here are the results:

version    time    time(relative to "current")
----------------------------------------------
current    2667    100%
insource   2388     89%
template   2937    110%

Most surprisingly, the "insource" is 11% faster than the current, pure PHP version. I have no explanation for this. I've heard PHP's string operators suck, but surely not to this extent.

The "template" version is ~23% slower than the non-template one ("insource"), but it's only 10% slower than our current code.

HOWEVER, benchmarking the template function directly doesn't accurately reflect how our overall code will be affected, because our code does more things besides outputting the link. So I benchmarked flag_create_link() as well, assuming it gives an indication for the overall effect:

version    time    time(relative to "current")
----------------------------------------------
current    14724   100%
insource   14291    97%
template   15378   104%

The differences among the three become smaller because the theming function has smaller part in the overall process.

The bottom line: using an external template file will make our code slower, but, negligibly slower: it's only 4% of *our* code, and a much, much, much, much smaller fraction of the overall *page* rendering time. And, of course, an external template has significant advantages.

I'll soon prepare a better patch: one that uses a template file. (There will be a D5 version too.)

I'll follow CCK and Views example and put it in a 'theme' sub-folder, together with a README.txt. The .css should eventually go there as well.

Here's the patch for D6 (I'll work on the D5 one only after I get the ok. No sense in doing double and quadruple work).

Some changes:

• We had two theming functions, 'flag' and 'flag_link'. I renamed 'flag' to 'flag_overall' and 'flag_link' to 'flag'. The reason: I feel that since 'flag' (the new one) is the main theme, and moreover appears as 'flag.tpl.php' in the filesystem, and tweaked by the themer as such, then it's more logical to simply name it 'flag'. It's one less confusion.
• The 'flag_overall' theme was getting an $is_flagged parameter. I changed it to $action, for two reasons of elegance, which I'll explain in length if asked to.

(The theme registry must be cleared after applying the patch.)

BTW, that patch makes using images for the links a piece of cake.

Say our flag name is 'favorites'. We drop a 'flag--favorites.tpl.php' in our theme folder, and edit it to have an {img src=...}. No need for CSS tricks.

This is a better title.

theme_flag_overall() seems clunky. Is there any way we can have a theme_flag_js() and call it from template_preprocess_flag() instead of calling theme_flag() from theme_flag_overall()?

Yes, that seems a good idea. I'll make it so.

but I think including a version for D5 is a working outside the normal Drupal conventions. I'd much rather see the D5 version use the traditional theme system.

I understand your concern, but the D5 version will use the traditional D5 system. I really want you to re-consider your stand on this. We won't need to re-educate themers.

The README.txt will include an optional phptemplate_flag() --to be copied into 'template.php'-- that instructs the theme engine to load the template file from the theme folder.[1]

This is a very conventional D5 theming.

You may reply:

"But the more common D5 theming is for the themer to copy a theme_flag() into phptemplate_flag() and change its PHP to her liking."

To this I will reply:

Themers aren't likely to use this "traditional" technique with our module. Our issue queue strongly suggests this; Modifying the PHP requires a themer to be PHP savvy. How many themers know of the 7th parameter to l()? Since most themers aren't such, they aren't going to use this technique; A thing which isn't used doesn't exist; Something which doesn't exists can't even be touted as "the traditional way".

And in fact templates are traditional in D5: they're used for nodes and blocks, and more. So it seems to me that we lose nothing by going the template way in D5.

I'm looking forward to hearing your thoughts on this.

[1] If this phptemplate_flag() isn't used, our original theme_flag() kicks in and directly include()s our official flag.tpl.php.

the theme_flag_overall() seems clunky. Is there any way we can have a theme_flag_js() and call it from template_preprocess_flag() instead of calling theme_flag() from theme_flag_overall()?

Here's an improved patch for D6 that does what you asked for. I named the helper function _flag_add_js(), and it's called from template_preprocess_flag().

Notes:

1. 'diff' does a messy job of showing what's changed. If you really want to enjoy the beauty of my code, you'll have to apply the patch.

2. Instead of calling theme() directly, I'm now calling a theme() method on the flag object. I'm following here Views' pattern because otherwise I'll have to provide quite a lot of parameters directly to Drupal's theme().

3. I'm not attaching 'flag.tpl.php' and 'README.txt' again because I haven't changed something important in them.

I'll soon upload the D5 patch.

And here's the patch for the Drupal 5 version.

(You'll notice the new theme() method, in flag.inc. This will be used for both D5 and D6 because I don't want to maintain two files.)

I'm attaching 'flag.tpl.php' for completeness sake: it's the same as that of the D6 version.

And the 'theme/README.txt' for D5 is of course differenet than that for D6. It contains the function to put in 'template.php'. I'd better move the body of that function to the module itself. First I wanted that you get an impression of it.

I'm marking both patches 'needs review'.

I like the new _flag_add_js(), but it should be called from a theme function rather than in a preprocess function (meaning it should probably be a public function also). While it totally pains me to put a function call in a .tpl.php file, it's necessary because users overriding the template file should be able to remove the JS file or add their own version. Once added to the page through drupal_add_js(), it's really hard to get back out again. The block-admin-display-form.tpl.php is an example of a tpl.php file doing this.

I suggest to have the following in flag.tpl.php:

  if ($setup) {
    drupal_add_css(drupal_get_path('module', 'flag') .'/theme/flag.css');
    drupal_add_js(drupal_get_path('module', 'flag') .'/theme/flag.js');
  }
  flag_add_extra_js($flag, $action, $content_id, $after_flagging);

1. _flag_add_js() had three duties: adding CSS link, adding JS link, adding html to the huge JS structure. The above code splits that into three so that each step can be altered.
2. A bonus: one can comment out the 'flag_add_extra_js(...)' to not have the huge JS structure (what happens then, that's to be dealt with in a different issue).
3. The $setup variable is for efficiency only: it's TRUE only if this is the first inclusion of the file.
4. I suggest we move flag.css and flag.js into the theme folder. I think the JS file belongs to the theming. What's your opinion?

I wouldn't mind cutting out the documentation for theme_flag() out of the Drupal 6 version, maybe we could just include that all in a description flag_preprocess_flag()?

OK. I'll follow forum.module style: it just lists the input variables and directs the user to read the full documentation in the .tpl.php.

So, is this patch, for both D5 and D6, approved? :-)

(No point in me posting the patch again: the altering to flag.tpl.php, in #14, and the renaming of _flag_add_js, is pretty much all.)

Keep in mind that anything we put in a .tpl.php file has to be the way we want to keep it. As users are likely to override it.

Yes, I know. Note the README.txt of the D5 version, which contains a 'phptemplate_flag()' to be copied into template.php. The point you mention is the reason I want to move the body of that function to an API utility function (in the D5 branch only, of course). I see no reason not to do that. I haven't done that yet because I didn't want you to encounter many "new" things in this issue. I suggest we leave that to a subsequent refinement issue.

Committed.

Yeeeeeeeeha! :-D

Keep in mind that anything we put in a .tpl.php file has to be the way we want to keep it. As users are likely to override it.

Yes, I know. Note the README.txt of the D5 version, which contains a 'phptemplate_flag()' to be copied into template.php. The point you mention is the reason I want to move the body of that function to an API utility function [...]

I've opened a new issue for that:
#285669: Better phptemplate_flag() for D5

I added a reminder to the homepage, "... you should clear your Drupal cache." It's needed because of the theme registry.