Continuing the work from #340031: Make real private flags usable by only the user creating the flag, this patch (against 6.x-2.0-dev) makes the 2 changes needed for the flag_custom module.

1. Add a hook_flag_delete() so that other modules can be notified before a flag is deleted.
2. Wrap the flag_get_flags() query in db_rewrite_sql() so other modules can restrict which flags are loaded if necessary for performance or other reasons.

CommentFileSizeAuthor
#10 flag.inc_.patch624 bytesawolfey
#7 flag.inc_.patch722 bytesawolfey
#5 flag_custom.tar_.gz7.86 KBawolfey
#4 flag.patch1.54 KBawolfey
flag-2.patch1.62 KBawolfey

Comments

awolfey’s picture

awolfey commented

Continuing the discussion, I agree that the arg checking in the rewrite of the query is hacky. I thought about a hook_get_flags where flag could let other modules tell flag what to load, but that would mean flag would have to pass the context in which the flags are being loaded.

That seemed like a pretty big api change that is probably not worth it.

It might, however, prevent some other duplication. As it is, with the query rewrite, I'm probably going to have to duplicate some of the views integration.

I think this patch works well enough to leave the rest alone.

Thanks for your help with this.

seehawk’s picture

seehawk commented

Subscribing.

quicksketch’s picture

quicksketch
he/him
commented

I'm fine with this patch but I'd like to see some product or benefit we gain from it in #340031: Make real private flags usable by only the user creating the flag.

Other than that, I can't see why we're using module_invoke() here instead of module_invoke_all(), seems like there's no reason for the manual looping.

awolfey’s picture

awolfey commented
StatusFileSize
new flag.patch1.54 KB

Here is the updated patch.

awolfey’s picture

awolfey commented
StatusFileSize
new flag_custom.tar_.gz7.86 KB

Attached is the flag_custom module. Requires the patch in #4. See config settings under admin/build/flags/custom

Todo #859822: Views integration for private flags module

awolfey’s picture

awolfey commented

The latest code is here: http://drupal.org/project/flag_lists

awolfey’s picture

awolfey commented
Title: Patch creates hook_flag_delete() and uses db_rewrite_sql on flag_get_flags() » Patch creates hook_flag_delete()
StatusFileSize
new flag.inc_.patch722 bytes

Here is a new version of the patch. Use this one if you are testing http://drupal.org/project/flag_lists .

quicksketch’s picture

quicksketch
he/him
commented

Hey awolfey thanks for the update! Normally when delete hooks are called, they're done after the DELETE queries by the main module. See:

http://api.drupal.org/api/function/node_delete/6
http://api.drupal.org/api/function/comment_delete_multiple/7

Would flag_lists still operate if the module_invoke_all() came after the DELETE queries?

awolfey’s picture

awolfey commented

Yes, as long as $flag is available it can go after the queries.

awolfey’s picture

awolfey commented
StatusFileSize
new flag.inc_.patch624 bytes

Here's a new patch.

maxxer’s picture

maxxer commented

Hi all,
I am really keen to try and test this out. However, a bit confused by the production timeline.

-->Should i still be getting flags 2 beta module from the flags page?
i.e. is flags_custom a modified version of flags 2, or an add-on module?
--> I assume i need patch #10, applied to flags 2 / flags_custom
--> do I still need #4 applied to flags 2 / flags_custom ?

I notice #4 patch has entries for .inc and .module. so I assume #4 must be applied before #10

For those of us just beginning to follow the action, a really quick summary for which packages/patches to go after would be really appreciated.

thanks for your efforts awolfey + quicksketch. Can't wait to (try!) and get this working.

awolfey’s picture

awolfey commented

maxxer, please see http://drupal.org/project/flag_lists

maxxer’s picture

maxxer commented

thanks awolfey, that was indeed the page which led me indirectly to this one.
please excuse me, I do not intend to clutter the thread, but for anyone else trying to get up to speed:

I downloaded and installed
flag 2 beta
-->http://drupal.org/project/flag
flag lists
-->http://drupal.org/project/flag_lists
flag-custom module
--> from comment #5
applied patch from #10 and #4 to flag module

working well here!

awolfey’s picture

awolfey commented

maxxer: You don't need flag_custom. It is replaced by flag_lists. You should uninstall flag_custom and delete it.

maxxer’s picture

maxxer commented

thanks. again, I do not wish to clutter the issues thread, can i ask where is the best place to ask a question about using views with flag lists? I've posted to the general forum 'post-installation' but i think it will get lost in the chaos.

awolfey’s picture

awolfey commented

The best place to ask about flag_lists is in the flag_lists issue queue: http://drupal.org/project/issues/flag_lists

awolfey’s picture

awolfey commented

I see we missed getting this patch into beta4. What can I do to help get it into dev and the next relase?

mooffie’s picture

mooffie commented

(It wasn't intentional. I'll commit it tomorrow. Nate, you see anything wrong with the patch?)

quicksketch’s picture

quicksketch
he/him
commented

I think this patch is a good idea, if it looks good to you please commit. Thanks as always mooffie.

mooffie’s picture

mooffie commented
Status: Needs review » Fixed

Committed.
http://drupal.org/cvs?commit=459764
http://drupal.org/cvs?commit=459762

awolfey’s picture

awolfey commented

Thanks mooffie!

Status: Fixed » Closed (fixed)

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