This small patch adds a token at the end of the links which is checked in the flag menu callback. I added two tiny wrapper functions which could be removed to save space but IMO they are a worthy abstraction. This is how nodequeue and og have implemented the same feature.

I didn't see any other place where we emit flag links (e.g. tokens or emails). If there are some, let me know and I will add the needed code.

I also renamed template_preprocess_flag() to flag_preprocess_flag() since thats the recommended way for modules to name their preprocess functions. They don't 'own' the 'template' namespace. Be sure to flush cache when testing.

Comments

quicksketch’s picture

Status: Needs review » Needs work

Thanks for whipping this up Moshe. It's got a few problems we'll need to fix yet. We'll need to make it so that this works for AJAX-toggled links also. Right now this'll throw a JS error because we return an HTML page instead of a JSON string. Granted, this shouldn't ever really happen on a legit site, it's best to be prepared and return a value that the JS can understand.

Also, according to the handbook page on preprocessors, the module that defines the template file should use the namespace "template_", for preprocess functions, similar to how they claim the namespace "theme_" for theme functions.

moshe weitzman’s picture

Status: Needs work » Needs review
StatusFileSize
new2 KB

Changed the process function name back. I did not know that.

The flag_page() already returns a drupal_access_denied() page in another code branch. That page issues a 403 http header which is a pretty good indication to the caling code that their request was forbidden. I would not get caught up in this returning invalid JS. There is no standard way to return JS for a 403 action.

quicksketch’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new1.83 KB

Looks good to me. Our JS also looks like it accounts for handling 403 errors already, so we're all good there. Here's a Drupal 5 port (just a change in the l() parameters).

mooffie’s picture

Right now this'll throw a JS error because we return an HTML page instead of a JSON string.

Looks good to me. Our JS also looks like it accounts for handling 403 errors already

I haven't yet tested the patch, but I think your original concern is still valid, isn't it?

Incidentally, the logic in flag_page() has always been hard to follow (for me), and parhaps it's time to revamp it a bit (e.g., rename $result to $success, and add an 'else' to the 'if').

quicksketch’s picture

Status: Reviewed & tested by the community » Fixed

I fixed the remaining question about JS links not erroring correctly and slightly restructured flag_page(). I think it's substantially more clear than before. You can test the error behavior by editing the link href in Firebug. Now bad tokens sent by JavaScript are returned with a JSON error message.

D5: http://cvs.drupal.org/viewcvs/drupal/contributions/modules/flag/flag.mod...
D6: http://cvs.drupal.org/viewcvs/drupal/contributions/modules/flag/flag.mod...

Thanks Moshe for taking initiative with this!

Anonymous’s picture

Status: Fixed » Closed (fixed)

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