There's one omission in our API: there's no official way to get the 'fcid' value associated with the flagging. This value is quite important for modules that want to associate further data with flaggings.

It's true that hook_flag() is passed this value, but this doesn't help when we want to know the fcid outside this workflow.

The attached patch solves this by extending is_flagged() to optionally return the {flag_content} record (the 'fcid' value is the DB key of this record). It's cheap because these records are cached.

The patch also documents the return value of _flag(), _unflag(), _is_flagged().

_is_flagged() was modified to return the 'fcid' instead of 'fid' (because returning 'fid' is useless: we already know its value). This non-cached version of is_flagged() is used in the flag() method.

CommentFileSizeAuthor
#7 is_flagged.diff2.24 KBmooffie
return_record.diff3.89 KBmooffie

Comments

mooffie’s picture

mooffie commented

Nate, let me know if you're ok with this patch. I need this feature for #871064: Making flaggings fieldable.

quicksketch’s picture

quicksketch
he/him
commented

This sounds like a good idea. Is there any reason why we need to return the entire record rather than just the FCID? I'm not a big fan of changing the return value based on the extra $return_record parameter and would prefer to always just return the FCID all the time (or FALSE if it's not found).

mooffie’s picture

mooffie commented

Is there any reason why we need to return the entire record rather than just the FCID?

A good question. I don't mind returning just the FCID.

The patch returns the entire record because I thought perhaps the programmer/themer would want to peek at the other fields there (e.g., at the timestamp ("flagged 47 hours ago", or at other columns a contrib module might have added to that table). I didn't see returning the whole record as breaking the prallelism with the methods that return just the FCID, because these all methods can still be said to return "either 'false' or somethign that evaluates to 'true'".

In a couple of hours I'll return and commit this patch:

I'll use your suggestion and modify is_flagged() to unconditionally return just the FCID (or FALSE). But if you change your mind let me know and I'll return the record.

mooffie’s picture

mooffie commented

"flagged 47 hours ago"

An slightly off-topic comment:

When #871064: Making flaggings fieldable is in flace there will be a 'flagging' entity. One of its properties will be the timestamp column (other properties will be the attached fields, of course). There will be (there already is, actually) a [flagging:date] token to access this timestamp property and allow admins to to type "Bookmarked ([flagging:date:ago] ago)" into the "Unflag text".

But, of course, doing this won't be possible on D6. Would returning the {flag_content} record from is_flag() make it considerably easier for themers to achieve this? I don't know. Perhaps calling the underlying flag_get_user_flags() isn't too hard after all.

So, to repeat:

I'll modify the patch to return just the FCID. But if you change your mind left me know.

quicksketch’s picture

quicksketch
he/him
commented

Cool, I don't want to got too far down overloading is_flagged() into something else. Using is_flagged() to get the timestamp of the flagged time just feels wrong.

mooffie’s picture

mooffie commented

(I committed the modification to _is_flagged(), and the documentation. But I haven't touched is_flagged(). I'll soon explain why.)

mooffie’s picture

mooffie commented
StatusFileSize
new is_flagged.diff2.24 KB

But I haven't touched is_flagged(). I'll soon explain why.

Nate, are you sure we won't break anything by returning a number instead of TRUE?

I did "grep -R is_flagged ." and the fact that I lingered on some lines caused me to hesitate in modifying is_flagged(). Now, I know in PHP a non-zero number is truthful, but what if somebody writes the result of is_flagged() to some database or serializes it to some XML schema expecting it to end up as a boolean? Perhaps I'm just being paranoid. What do you think?

If you told me "yada yada yada, just make is_flagged() return the FCID and everything will be fine!" I'd do it.

I'm attaching a proposal: There's a get_flagging_record() method now, and is_flagged() is implemented by calling it. If somebody wishes to find out the 'fcid' or the 'timestamp' he can call get_flagging_record() directly.

#871064: Making flaggings fieldable will introduce a get_flagging() method to get the flagging entity (don't worry about code bloat: it's the only public method the 'fieldable' feature will introduce). So this proposed get_flagging_record() could complement the API.

quicksketch’s picture

quicksketch
he/him
commented

Makes sense to me. It's pretty cool that adding such methods are so trivial with our existing APIs. The only caveats here are the changes to the PHPdoc. Function descriptions should be a single line sentence, followed by additional paragraphs if necessary to describe the functions. We shouldn't change the description of the function to be the return value and we should keep the @return where it was. Code changes look good to me.

mooffie’s picture

mooffie commented
Status: Needs review » Fixed

Committed. (After fixing the phpdoc.)
http://drupal.org/cvs?commit=409382
http://drupal.org/cvs?commit=409380

Status: Fixed » Closed (fixed)

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

geerlingguy’s picture

geerlingguy commented

How does one get the fcid using this fix? I'm trying to get the flag by calling get_flagging_record($content_id, $uid), but that's not working (I'm getting Call to undefined function get_flagging_record()...

If I try calling return flag_flag::get_flagging_record(15, 1);, I get the error Using $this when not in object context... I'm not too well-versed in class-based programming in PHP, and I'm getting confused here.

I'm trying to call this within a form_submit handler function.

[Edit: Okay, I've now tried getting the data like so:

$flag = new flag_flag();
$flag->get_flagging_record(15, 1);
dpm($flag);

But this returns an empty object... how can I get a record?