Is anybody working on this? I can give it a try if nobody does.

"Flaggings" are the white elephant in the room. They are the records in {flag_content}.

Why make them fieldable? To implements things like:

  • A "reason" text field for a "Spam" flag.
  • A "folder" taxonomy field for a "Bookmarks" flag.

This would obsolete ideas like "private flags" and modules like "flag abuse".

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quicksketch’s picture

Sounds like a good idea to me. I'm not currently working on any 7.x only features (or really on 7.x much at all since the APIs keep changing on me). Help fixing the existing Drupal 7-only bugs would also be appreciated.

Also: great to hear from you mooffie!

mooffie’s picture

Assigned: Unassigned » mooffie

So I'll start working on this :-)

(I haven't "come back" yet. My memory of Drupal is a bit rusty, but I thought the "fieldable flaggings" thing is too exciting to ignore...)

amitaibu’s picture

@quicksketch,

I think we talked about on IRC, but can you remind me why are you against flag using field API?
Also, if flag itself was a fieldable entity, you would be able to flag flaggings "for free".

p.s. 3AM here, but baby thought it was play time :/

quicksketch’s picture

I think we talked about on IRC, but can you remind me why are you against flag using field API?

To clarify things, I'm against Flags *being a field*, I'm not against Flags being an entity, and therefor being fieldable.

amitaibu’s picture

> I'm against Flags *being a field*

Yeah, I mean that ^^ :) Remind me why are you against?

sirkitree’s picture

subscribing

quicksketch’s picture

Fields are intended to be a piece of information about an object. It doesn't line up with the Flag data model, which is basically establishing relationships between two different object (a user and something else). Fields don't let every individual user store a different value, it's always just setting a property directly on the object.

Additionally we need to store custom information (like flag counts aggregation) in a database table anyway, meaning that the benefit of using fields and their swappable storage backend would be minimal. Also, we can't re-architect all of Flag as a field and continue to adequately support both Drupal 6 and Drupal 7. If (and that's a big if) we ever convert Flag to be a field, it will be in the Drupal 7-only version when support for Drupal 6 comes to an end.

mooffie’s picture

(I had this feature implemented two weeks ago already. But I want the patch to be as clean as possible (that is, bereft of hacks), and that's why I've been submitting trivial patches to fix/enhance various issues in the module itself. I think it won't take much longer.)

mooffie’s picture

Version: 7.x-2.0-beta3 » 7.x-2.x-dev
Status: Active » Needs review
FileSize
14.52 KB

Okey dokey.

I'm publishing my work so far.

On the one hand, it's fully functional.

On the other hand, it's incomplete: I wanted to keep this first patch small (I left @todo comments).

 

How does it look like?

I've prepared a handbook page demonstrating this new feature. It contains some screenshots. This page should be an easy-reading for non-programmers too.

While reading that page you might get the impression that there's a lot of new functionality, and you'd be correct. However, most of this functionality is implemented by Drupal itself (its Field subsystem), not by the Flag module. There's no code bloat.

 

The implementation

The implementation of this feature ("fieldable flaggings") is strewn over three components:

  1. A core patch (by "core" I mean the Flag module itself). It isn't big.
  2. The Flag form module. It implements a "Form" link-type (which can be thought of as "a confirmation form on steroids").
  3. The Flag dialog module. Optional. It's a sub-module of Flag form. It makes the forms appear in a modal dialog box.

 

Downloading

  1. The core patch you can download below (there should be a link at the end of this post). You must apply this patch.
  2. The Flag form module (and its Flag dialog sub-module) you can download here (github). Additionally, The Flag dialog module is dependent on the Dialog API module (Its homepage says you need CTools, but you don't: The D7 version doesn't need CTools).

 

Reviewing the core patch

The core patch has three logical parts to it:

# What Where
1. Some basic Field API hooks flag.entity.inc
2. Connecting the $flag->flag() method, and later the rest of Flag, to the Field API flag.inc
3. Tokens flag.tokens.inc (and a bit in flag.inc)

The focus should be on part #2 (because that's the part that has the potential to change wildly).

 

Before applying this patch:
Make sure the Flag module works for you. Drupal 7 is a moving target.
You'll be fine if (at the time of this writing) you'll be using Flag's 'dev' and D7's alpha7 or later.

mooffie’s picture

A few words about the Flag Note module:

I was absent for several months and wasn't quite aware of this module. It was only a few hours ago that I looked at it. The proposed "fieldable flaggings" thing obviously has one feature of Flag Notes but it misses another: the ability to keep a history of the flaggings.

So, how can we implement this "history" feature? I see several options (I'm writing this hastily; research is needed):

  1. I think it's relatively easy to write another, separate module (Flag History) that mirrors the "flagging" entities onto "flagging-history" entities. Whenever a flagging entity is written, a flagging-history entity is written as well. Pros: probably easy to implement.
  2. Use Drupal's revision system? Pros: sounds very elegant. Cons: I'm not yet sure how easily the revision system can be applied to flaggings.
  3. Or we can implement this history feature in Flag itself. The proposed flaggings concept will have to be ditched in favor of a more general concept. Cons: It might complicate Flag's code.
  4. (Or we can keep deleted flaggings in the {flag_content} table: and add a 'status' column to tell us if it's deleted or not. This will require updating all the sql queries in Flag.)
amitaibu’s picture

@mooffie,

1) The docs look really interesting, however I think that if it's possible - show bigger images, otherwise, one has to click to see the image, and loses context
2) WOW! :)

mooffie’s picture

(Amitai, thanks. I too would like bigger thumbnails, but the image host I'm using (imageshack) doesn't let me configure this size.)

amitaibu’s picture

> but the image host I'm using (imageshack) doesn't let me configure this size.)

You can attach the images to this issue, instead of imgeshack.

aaron’s picture

exciting and subscribe! (coming over from #954918: Flag Media).

fago’s picture

Fields are intended to be a piece of information about an object. It doesn't line up with the Flag data model, which is basically establishing relationships between two different object (a user and something else). Fields don't let every individual user store a different value, it's always just setting a property directly on the object.

Still, you could store the reference on the flagging (regardless whether this is an entity) using a field. That's the way field_collection (http://drupal.org/project/field_collection) works. That way you'd automatically support each fieldable entity + I'd be consistent to configure for users I guess. Also it helps you with altering entity forms a lot ...

anyway, for going the fieldable flagging route, consider building upon the entity CRUD API. :)

quicksketch’s picture

anyway, for going the fieldable flagging route, consider building upon the entity CRUD API. :)

That is the plan, if we're adding fields to something then the Field API is an obvious choice... for things that are fields.

mooffie’s picture

#1026092: Flag with user reference? was marked a duplicate of this issue.

Itangalo’s picture

subscribing

snupy’s picture

subscribing

JohnnyX’s picture

Subscribe

Sounds and looks good. I this feature added to the dev or beta release of flags at the project page?

I would use flags and rules as notification solution but flag is missing the rules integration. Could I use fieldable flaggings to work with rules and trigger notification emails that way?

JJones’s picture

Is this in the D7 beta version? Interested in trying this out.

q0rban’s picture

subscribe

Shawn DeArmond’s picture

subscribe

Shadlington’s picture

Sub'd

q0rban’s picture

FileSize
11.18 KB

Here's a (untested) re-roll against the current version of Flag. Instructions still in #9.

q0rban’s picture

FileSize
14.54 KB

Whoops, missed includes/flag.entity.inc.

mooffie’s picture

Assigned: mooffie » Unassigned

I'm unassigning myself because I won't be working on this.

mooffie’s picture

Some odds and ends:

(...which I didn't want to discuss previously because I wanted to keep this thread focused.)

  • There's a certain feature that this patch makes it easy to implement:

    Users have been asking if it's possible for them to flag on behalf of another user (see 1, 2). We used to reply "No, it's not possible, because we don't have a GUI for this. (But you can do this via code)." But now at last it's possible to implement this GUI because we have a form for editing the flagging. All that's needed (on the GUI side) is to add an autocomplete field to the form, for picking the flagging user. (See the flag_flagging_form() function; the $is_privileged parameter would tell us if the acting user has permission for this). This is similar to what already happens with nodes: privileged users can choose a node author (again, using an autocomplete field).

  • One feature that's already there is the ability, for privileged users, to edit and delete flaggings. This is possible because flaggings have address and can therefore be accessed (e.g., ?q=flag/privileged-flagging/4571/edit).
Shadlington’s picture

Applied #26 to a clean install and so far so good... Kinda.

There's a few issues:
1. There is no dependency on flag form. There either should be or the interface needs to be changed to not allow you to add fields unless flag form is available, as whilst you can add them there is no way to do anything with them if the module isn't available. I noticed this because I forgot to install flag form along with the patch and was a little confused!

2. Does something odd to the Bartik theme. I had changed the colour set to 'Ice' instead of the default 'Blue Lagoon'. However, clicking on flag with the dialog enabled causes the theme to switch back to Blue Lagoon until the page is refreshed! Really odd...

3. Redundant UI element. 'Custom display settings' doesn't contain anything but is still visible. Meh. I'm clutching at straws with this one, really!

Generally seems pretty good though!
I've not tested it all that thoroughly to be honest (only tried a couple of field types) but it certainly seems to work quite well :D

mooffie’s picture

There's a few issues:
First off, there is no dependency on flag form.

(It's intentional. But I'll explain later; I have to go...)

Shadlington’s picture

Okay, got another, maybe. Not sure if this one is normal or not.

Completely uninstalled flag but the database still had the field definitions of the flag fields as well as the actual entries against flaggings in the field tables.
Not sure if this is a normal thing to do with how D7 does fields but it seems like flag should clean up after itself really. I really don't know though.

EDIT: In fact, I was able to re-enable the modules and the fields I'd added were still there.

rlmumford’s picture

So, did flags end up as entities?

mooffie’s picture

There's a few issues:
1. There is no dependency on "Flag Form". There either should be or the interface needs to be changed to not allow you to add fields unless flag form is available,

"Flag Form", which only provides an out-of-the-box user interface for editing fields, is a distinct component. I don't feel it'd be right to make it a required module.

Incidentally, fields are usable even without the "Flag Form" module; for example, they can be edited with editablefields (in theory), or set via custom code or Rules. When the form isn't used, the fields are assigned their default values (its a nice feature; it's mentioned in the help text).

But I certainly get your point! That's why when the "Flag Form" module isn't enabled, there's a bold message shown on the fields screen. You may have missed it. I guess it should be more prominent (e.g., shown in the message area and on the flag page).

Okay, got another, maybe. Not sure if this one is normal or not.
Completely uninstalled flag but the database still had the field definitions of the flag fields

Right. There are more omissions in this patch. Look for the "@todo" comments. For example, the fields data is kept also in the case where a flag is deleted. There's certainly more work to be done.

3. Redundant UI element. 'Custom display settings' doesn't contain anything but is still visible.

This is possibly a (usability) bug in Drupal itself.

(The "display settings" govern how a flagging is printed. Much like how a node is printed. This doesn't seem to make sense right now because nowhere flaggings are printed. But they are printable. This will make more sense with the Views integration -- when one constructs a view of type Flaggings (constructing such views is possible with Flag Vista but not with our own lousy Views support).)

@rlmumford wrote:

did flags end up as entities?

Flaggings are entities. Flags aren't. But the next logical step is to make flags too entities. No reason not to.

Note that our lousy Views integration won't allow one to fully utilize this feature; e.g., you won't be able to construct a view showing all flaggings whose flag has its "It's a user badge" field set. But Flag Vista will be able to do this.

(This will be my last message in this thread. We want to encourage somebody to step in.)

rlmumford’s picture

Ok, that makes sense. A flag is therefore like a flagging type? (I think in the code these are labelled flag-type and flag respectively)

Also, if flaggings are entities doesn't that close this issue?

Shadlington’s picture

Thank you for the informative response moofie.
That all makes perfect sense.
The only 'real' issue that I found then is the odd behaviour with the changing theme colors... But this could well be a problem with Dialog. I would test it but I can't get any other D7 implementation of Dialog to work! Oh well.

EDIT: Also, flag vista looks awesome! Pity there's no D7 yet and you're leaving :(

mooffie’s picture

Whatever, I think Flag 2.0 should be released asap and that #871064: Making flaggings fieldable (this issue) and #1035410: Flag any entity should be included in Flag 3.0 only. That's because these two features might make the code in the D7 branch (of Flag 2.0) too different than the code in the D6 branch (of Flag 2.0), something which won't ease the maintenance.

(Flag 3.0 should be for D7 only. Flag 2.0 should not get any new features.)

This shows that new (co)maintainer(s) are badly needed.

A flag is therefore like a flagging type?

Mmmmm... if flags will be fieldable entities, then yes. Since flags could then have attributes ("this is a user badge", "this is a folder") they (the flags) could effectively function as types for the flaggings.

(I think in the code these are labelled flag-type and flag respectively)

(No: "flag_type" stands for the object that can be flagged (by a particular flag); e.g., "node", "user", "comment".)

quicksketch’s picture

Yep, I totally agree with everything you've said mooffie. Thanks so much for all the work you've done on Flag and keeping the issue queue afloat. I'm hoping to start reviewing and getting out a final version of Flag in the near future. The fact that we don't have a stable 2.x branch despite the fact that we recommend 2.x over 1.x is quite silly.

Cyberwolf’s picture

Subscribing.

Shadlington’s picture

I was wondering if there was any update on this? Or flag D7 development in general?

I'm surprised no-one has come forward to co-maintain yet considering flag's wide usage - I know I would if I had the necessary ability.

rlmumford’s picture

I would be happy to co maintain. Just let me finish my degree in a few weeks.

TimelessDomain’s picture

subscribing

quicksketch’s picture

Status: Needs review » Postponed

From mooffie's suggestions (which I agree with):

I think Flag 2.0 should be released asap and that #871064: Making flaggings fieldable (this issue) and #1035410: Flag any entity should be included in Flag 3.0 only.

q0rban’s picture

Status: Postponed » Active

Would it be possible to get the changes to the API (except for the field/entity hooks) into 2.x so that users that do need fieldable flaggings can implement it in their own custom module? I think the API changes are fine for both 6 and 7.

q0rban’s picture

Status: Active » Postponed

Well, nevermind. I started to work on a patch for that, and it wasn't so trivial. Switching back to postponed.

Fidelix’s picture

Subscribing...

zandros’s picture

subscribing

zandros’s picture

When i apply the patch i get these warnings:

    *  Warning: include_once(/home/my_site/domains/my_site/public_html/sites/all/modules/flag/includes/flag.entity.inc) [function.include-once]: failed to open stream: No such file or directory in include_once() (line 9 of /home/my_site/domains/my_site/public_html/sites/all/modules/flag/flag.inc).
    * Warning: include_once() [function.include]: Failed opening '/home/my_site/domains/my_site/public_html/sites/all/modules/flag/includes/flag.entity.inc' for inclusion (include_path='.:/usr/local/lib/php') in include_once() (line 9 of /home/my_site/domains/my_site/public_html/sites/all/modules/flag/flag.inc).
zandros’s picture

And when i press manage fields it gets me to the edit flag page where no add field functionality is shown

zandros’s picture

any comments?

schrysan’s picture

I have the same problem. Is there a solution? Any update on that?

rosell.dk’s picture

subscribing

sslider999’s picture

sub

rafamd’s picture

subscribing, would be great to see a 3.0 with this feature.

ericduran’s picture

FileSize
14.62 KB

Here's a re-rolled of this patch.

Again as mention before this is not going to make it in till the 3.x branch.

So I'm just posting the latest re-rolled to keep track of it.

q0rban’s picture

Updated patch, same as #54 except that $reset is passed as TRUE to flag_get_flags() in flag_entity_info(). Without this, flags that have been exported to code are not loaded properly until a full cache clear, causing simpletests to throw notices.

partyp’s picture

subscribe

goron’s picture

Here's a re-rolled patch which applies cleanly to the latest 2.x dev. Same as the patch in #55, except takes out one section with a change which already exists in the current dev.

wroxbox’s picture

Title: Making flaggings fieldable » Some tokens missing for rules
FileSize
62.29 KB
27.47 KB
32.68 KB
43.1 KB
57.8 KB

I'll try to summary my settings and setup

I have latest flag.module with applied patch #57 and I have https://github.com/mooffie/flag_form and I have flag_abuse to mark comments abusive.

Form:
Fields
And I get tokens:
Tokens

The public site looks like
Report Comment as abusive form

This all works great! Just what was needed. Now I am for Rules.

I wan't to send email to reporter who reported about the comment. So I created rule and try to fetch the email-address.
Fetch Entity by property

It seems to find right entity with right field but the flagging:token is missing for value
token Value is missing

I assume it should be available as flagging:field-flag-email?

wroxbox’s picture

Accidentally renamed the issue. Sorry!

streche’s picture

Title: Some tokens missing for rules » Making flaggings fieldable

subscribe

goron’s picture

streche, please use the Follow button at the top of the issue instead of posting comments to subscribe!

wroxbox’s picture

Here is the same as the patch in #57, except adds rules support for tokens explained in #58

Token support for flagging rules

zilla’s picture

excellent ideas - i see that this discussion started nearly a year ago, but is this rolled into any recent dev or test release?

q0rban’s picture

Here's an updated patch from #62 with a few very small changes:

  • No longer calls ->new_flagging() inside of _insert_flagging(). Instead inside ->flag(), we create the new flagging. This allows us to pass other data in that _insert_flagging() may not have (in this case $uid).
  • Updated documentation on ->new_flagging().

It should be noted, I haven't tested any of the new work from #62, yet this patch is based off of that one.

@zilla, this work exists only in this issue queue, and in the flag_form module to go along with it, now found at https://github.com/q0rban/flag_form (moofie is no longer maintaining his version of flag_form, referenced above).

q0rban’s picture

Another slight tweak to this. We are detecting uid and sid in ->flag, but not setting it to the $flagging object that comes in. This means that in hook_field_attach_presave, et. al., those values are null. Attached patch fixes that.

I can't help but think that Flag module could use some ground up re-architecting as we continue to refine this patch.

MiSc’s picture

Just needed to add support for anonymous users, that will use timestamp for sid value. Pretty stupid, but that I needed to solve a specific problem.

IWasBornToWin’s picture

Is there a single patch we can apply to our current, and only, version of Flag? I'd enjoy these features.

MiSc’s picture

@IWasBornToWin: The patch in comment #65 should do the trick for you with the latest dev of flag 2.x.

yogeshchaugule8’s picture

let me take an opportunity to introduce a module which works same way:

The module provides the Drupal admin to attach a webform to a flag so that when user flag/unflag content on the Drupal site, they can add additional information using the webform.

The module page is http://drupal.org/project/flagcoolnote

MiSc’s picture

flag_form has moved from GitHub to d.o. and changed the name to flagging_form, and a first beta release is now out: http://drupal.org/project/flagging_form

@yogeshchaugule8, interesting solution, but it not the same way, it seems to be a totally other way, but similar result ;-)

juandelacruzm’s picture

The updated version (march 14 2012) of flag 7.x-2.x-dev needs the patch in comment #65?

wroxbox’s picture

@yogeshchaugule8 Is flagcoolnote working also for comments?

MiSc’s picture

@juandelacruzm: Yes, that patch work for the latest dev of Flag 7.x-2.x.

rupertj’s picture

The patch from #65 is working nicely for me (For some reason I had to apply it with patch instead of git apply, but that could be my issue...)

Could we get this commited to a -dev release?

Oleksa-1’s picture

Would like to see patch #65 commited as well

quicksketch’s picture

Could we get this commited to a -dev release?

Flag module has been sadly neglected for a while now on my part. For this to move forward we'll need to get 2.0 out first. #1543826: Applicants Wanted: Flag Maintainership

yogeshchaugule8’s picture

@wroxbox: Yes, the module will work with comments as well as users.

Scryver’s picture

I'm running into problems with rules and automated (un)flagging. If I want to unset a flag using rules a ajax error pops up with the following message:

An AJAX HTTP error occurred.
HTTP Result Code: 500
Debugging information follows.
Path: /climbcheck/flag/flagging/boulder_project/984/delete/ajax
StatusText: error
ResponseText: RulesEvaluationException: Argument <em class="placeholder">flagging</em> is missing. in RulesPlugin->setUpState() (line 648 of E:\xampp\htdocs\climbcheck\sites\all\modules\rules\includes\rules.core.inc).

Maybe because it doesn't use the EntityAPI functions?

Edit: With the normal flag actions it works.

MiSc’s picture

@Scryver, are sure this is related to making flag fieldable?

Scryver’s picture

Well that was the beginning of my problem after I added fields to flags, so I guess. Should try to undo the fields and backup to the flag module. But I really need this later on...
I've also tried to set some field values automatically with rules without a form popup or anything. But the fields won't save. That's why I said that with the EntityAPI functions you would include a lot of rules and token actions at a low cost. Being able to save the Entity with a rule is something I can't get to work at the moment.

funature’s picture

i use the patch of #65 and it works fine. my question is, will that be a problem if one day this feature is added to the flag module? can i update the module then as usual without losing the data?

Itangalo’s picture

@euroba: I'm not a Flag maintainer, but I would say the answer to your question is "no". Since the patch isn't comitted yet, it can still change (and it might not even be accepted).

joachim’s picture

I've been wondering whether this could go into 2.x or have to wait for a 3.x branch.

In theory, you can entitize any database table by just declaring hook_entity_info() on it and a few associated callbacks.

However, the latest patch for this looks like it changes quite a lot. Given this module tries to keep its 6.x-2.x and 7.x-2.x branches reasonably in sync, that suggests to me this should form the first new feature in a 3.x branch.

I plan on looking at it soon, but after #1035410: Flag any entity.

joachim’s picture

This patch is going to need a big reroll.

Also, I reckon we can maybe get field form elements into it in the flagging confirm form.

joachim’s picture

Just tried applying this patch. The good news: it applies!

The bad news: it creates a circular dependency!

Our hook_entity_info() goes to look at which flag types we have in order to know about flagging bundles. But since we've added #1035410: Flag any entity, we have a flag_entity class which calls entity_get_info() in its constructor...

Looks like lazy-loading the entity info into that class when we need it fixes the problem -- at least, commenting it out makes my site load again! :D

Here's a reroll of the last patch to keep it fresh; I've also moved functions from entity.inc to the main .module file, as think it's best to use entity.inc for just classes so it can be autoloaded.

joachim’s picture

Oleksa-1’s picture

Tested patch from #85 and faced with circularity. Patch from #1689704 solved this issue. Created and removed text fields without any problem, tested with views and it works fine.
I have only one doubts about flag_form submodule maybe it is better to put its basic functionality to flag module code. It is more logical than to have it separately.

joachim’s picture

Thanks for testing!

Do you mean this one: http://drupal.org/project/flag_form
I can only see a 6.x branch for that.

joachim’s picture

-  function flag($action, $content_id, $account = NULL, $skip_permission_check = FALSE) {
+   function flag($action, $content_id, $account = NULL, $skip_permission_check = FALSE, $flagging = NULL) {

Can't say I'm thrilled about this method getting yet another parameter. I can see why it's necessary, but it's very gradually making it into a monster.

I'm looking at making the confirmation form have the fields so they can be set on flagging, but for that to make sense the main flag() function needs the $flagging param too, and therefore we need #1689530: flag() should support $skip_permission_check to not have a WTF in our function definitions.

goron’s picture

The flag_form module that was used with this patch before is now flagging_form: http://drupal.org/project/flagging_form

Oleksa-1’s picture

FileSize
31.19 KB

On this page /admin/structure/flags/manage/bookmarks/fields this module is mentioned and without it enabled not possible to get form. To get forms working I downloaded flag_form module from mooffie post http://drupal.org/node/871064#comment-3496040
111

joachim’s picture

Version: 7.x-2.x-dev » 7.x-3.x-dev
Status: Postponed » Active

Thanks for the details! I'll look at it today.

Changing the version for this.

joachim’s picture

Status: Active » Needs review
FileSize
18.23 KB

New patch.

I've made it so that the Flag core confirmation form has the flag fields. Of course, once you've saved the fields there's no way to edit them again :D

You can see them though if you set up a view.

I've had a look at http://drupal.org/project/flagging_form. It works great with this patch. Not sure whether to suggest it move into this module -- it sort of makes sense, as it's what makes fieldable flags truly useful, though I'd really rather not have more code to maintain!!!

Status: Needs review » Needs work

The last submitted patch, 871064.93.flagging-entity.patch, failed testing.

joachim’s picture

Status: Needs work » Needs review

*sigh* That'll be because the 7-3 branch was a few commits back from 7-2.

joachim’s picture

#93: 871064.93.flagging-entity.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 871064.93.flagging-entity.patch, failed testing.

Oleksa-1’s picture

Status: Needs review » Needs work

1)I have error message after enable flaf module: Deprecated function: Call-time pass-by-reference has been deprecated in drupal_load() (line 1128 of C:\xampp\htdocs\ndb1\includes\bootstrap.inc).
2) on page /admin/structure/flags/manage/bookmarks/fields should be link to http://drupal.org/project/flagging_form instead of
"Note: You don't have the Flag Form module enabled. You'll have to enable it to have the "Form" link-type."
3)"Of course, once you've saved the fields there's no way to edit them again" Why? Imho it should be possible edit/delete these fields (handlers in views) see http://drupal.org/project/flag_note (d6) I used that module and it was very handy to have such functionality.

joachim’s picture

Status: Needs work » Needs review

Testing bot is on the fritz. The patch passes all tests perfectly for me!

joachim’s picture

Status: Needs work » Needs review

> Deprecated function: Call-time pass-by-reference has been deprecated in drupal_load() (line 1128 of C:\xampp\htdocs\ndb1\includes\bootstrap.inc).

Weird. Any chance you could do a backtrace and find out what's causing that?

> page /admin/structure/flags/manage/bookmarks/fields should be link to http://drupal.org/project/flagging_form instead

Yup, will do.

> "Of course, once you've saved the fields there's no way to edit them again"

What I mean is that it's not possible with just the core Flag module. Obviously flagging_form lets you edit the flagging after you've flagged content.

Thanks for reviewing the patch!

Oleksa-1’s picture

Ok, will try to backtrace, but with patch from #85 , I do not have such error

Oleksa-1’s picture

Seems fix for Deprecated function: Call-time pass-by-reference has been deprecated in drupal_load() (line 1128 of C:\xampp\htdocs\ndb1\includes\bootstrap.inc) is to change:

  if ($action == 'flag') {
    // If the action 'flag', we're potentially about to create a new
    // flagging entity. We need an empty new entity to pass to FieldAPI.
    $flagging = $flag->new_flagging($content_id);
    field_attach_form('flagging', $flagging, &$form, &$form_state);
    $form['#flagging'] = $flagging;
  }

for

  if ($action == 'flag') {
    // If the action 'flag', we're potentially about to create a new
    // flagging entity. We need an empty new entity to pass to FieldAPI.
    $flagging = $flag->new_flagging($content_id);
    field_attach_form('flagging', $flagging, $form, $form_state);
    $form['#flagging'] = $flagging;
  }

and some api function should be corrected
852
function flag_user_login(&$edit, &$account) {
>function flag_user_login(&$edit, $account) {

1123
field_attach_form('flagging', $flagging, &$form, &$form_state);
>field_attach_form('flagging', $flagging, &$form, &$form_state, $langcode = NULL);

1130
function flag_confirm_submit(&$form, &$form_state) {
>function flag_confirm_submit($form, &$form_state) {

488
function flag_field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode) {
>function flag_field_attach_form($entity_type, $entity, &$form, &$form_state, $langcode = NULL) {

joachim’s picture

Aha. That'll be my fault for copying function definitions from api.drupal.org...

Thanks for figuring it out! :)

joachim’s picture

Here's a new patch with those fixes.

It's only the calls to functions that mustn't have the &; function declarations need them for &$form and &$form state.

Oleksa-1’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looking forward 7.x-3.x-dev!

socketwench’s picture

Tests clean for me too.

joachim’s picture

Status: Reviewed & tested by the community » Fixed

Here goes!

The 7.x-3.x branch has now diverged! :D

Thanks to everyone who's worked on the patch and helped with reviewing and testing it.

Issue #871064 by mooffie, q0rban, ericduran, goron, wroxbox, joachim: Added flaggings as a fieldable entity.

Oleksa-1’s picture

Great news, Joachim! Flag now very powerful module and it can be used e.g. for e-commerce to make orders or has some fuctionality like webforms, really cool)
I care maybe on main project page should be description about difference between 2.x and 3.x ?

joachim’s picture

3.x is not production ready yet! I'll add details to the project page when there's a fixed release.

Oleksa-1’s picture

yep, makes sense )

forestmars’s picture

Status: Fixed » Needs review

Disregard #65: flag-871064-fieldable-65.patch queued for re-testing.

Looking forward however to seeing the patched version of 7-3x-dev, and notes as to where it's not production ready.

Didn't see anywhere in this thread why the patch is being committed to 3.x, not 2.0, would be interested in knowing that.

Edit: Given that the 3.x branch is D7 only, it makes sense to keep it separate from the 2.0 branch, which has to support D7 & D6 (as Joachim points out in the comment rafamd links to.) Although I thought the same approach was suggested for Flag Any Entity, the patch for which I think went straight to 2.0 and is now in beta8.

rafamd’s picture

@forestmars, think the latest 3.x dev should be the patched version you are looking for. Notes as to where it's not production ready? given that 3.x branch was created just a few days ago and hasn't even reached alpha, guess the notes aren't on the project page just because it's very new (suppose we'll have an official update on the project page sometime soon, maybe after 2.0 which is the next planned release after current beta8).

See comment #83 on this thread to answer your last question.

cheers !

joachim’s picture

Status: Needs review » Fixed

The status of the 3.x branch is that currently it's undergoing some huge changes and should thus be considered unstable. The plan for those is outlined here: #1703470: [meta] The Great Flag Cataclysm. In particular, right now I could use help with testing #1681540: replace 'content' with 'entity', eg $content_type with $entity_type.

After the big changes land, I'll make a 3.0-alpha.

forestmars’s picture

Status: Fixed » Needs review

(comment made in error)

Oleksa-1’s picture

Status: Needs review » Fixed

@forestmars This issue is fixed please open new one if needed

zilla’s picture

wow, this has become an EPIC thread! given the immense interest in making flags fieldable, would it make any sense to consider the feature first as a module on its own, a plugin to flags, such that users could test out and see how it suits the use case without creating a big core change to flags - or is that sort of a waste of time if the long term goal is to simply enhance the core flags module?

joachim’s picture

This issue is marked as fixed: the core change is already in!
And anyway, the changes run too deep for that to be possible, I think.

forestmars’s picture

Zilla- Joachim could speak to your suggestion with a greater depth of authority here (in fact he just did) however given how few issues I've seen (at least so far) with this integration, I'd say the decision to merge this in was well warranted. Also, I'd suggest that making objects fieldable is a core concept. (Field all the things!)

Status: Fixed » Closed (fixed)

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