module_invoke_all hook in add and remove functions

hi
the reciprocal buddy adding feature actually already works for me. The proposed hooks are a fine idea, I thought already of something like that. So a contribution for this would be welcome.

However your patch contains a lot of changes. Please make sure to create a clean patch without any string customizations or other stuff not belonging to this issue.
I think your editor does automatically cut trailing spaces - perhaps we could roll an extra patch to remove them.

Here is a patch with just the hook. I changed it a little bit, so it takes 4 parameters: the operation (add or remove), the two uid's, and a flag to say if this is a reciprocal operation. I also added it to buddylistinvite and buddylistautoadd as they both access the buddylist table directly. I didn't change buddylist_confirm_form_submit since AFAICT it doesn't seem to be used anymore (?)

one of my goals is to get all of the drupal_set_message out of the mainline buddylist code because 1) I don't always want them and 2) when I do they aren't easy to customize externally. This patch would be a first good part of moving those out since you could now write a small add-on module, say buddylist_msg, that could handle your new hooks and provide these messages. Furthermore, a custom handling could be easily added.

I'd suggest we commit this patch as is then make work to move out the drupal_set_message stuff into another sub module.

How does this sound?

If I'm right, and buddylist_confirm_form_submit is not used, then it should probably be removed.

Can anyone confirm this either way?

its definitely used and needed - removing it would break the whole module. It handles all the accept confirmations (accept/deny).

That's what I initially assumed. But I couldn't find any way to get that function to be called. I removed it, and the module still seems to work. Looking at the code, I think that buddylist_pending_requested_accept_submit() and buddylist_pending_requested_deny_submit() are doing its job now, instead.

If I'm wrong (which is a distinct possibility, as confirm_form() has always confused me), do you know what what I can do to make buddylist_confirm_form_submit() get called?

Looking at it again, you are right. I changed how that worked when the accept/deny actions patch http://drupal.org/node/108671 was written that removed the forms and added the links with the accept/deny options. The code from buddylist_confirm_form_submit() has been split up into the various _submit handlers that you mentioned.

So yes, it looks like buddylist_confirm_form_submit() could be removed - please submit this as a new issue independent of this one regarding the hooks.

I've recently been given commit access for this project so I hope to start rolling in some of the code for these various issues.

Thanks.

Okay, I have created a separate issue for that. #2 is now the patch to review for this issue.

Here it is without the offsets.

I'm done committing changes for today and I can test this out on Monday. If its not too much trouble can you re-roll against the current version v.1.68.2.30. Thanks for working with me on this. After we get the hooks in I will see about moving the drupal_set_messages out of the mainline code and into a buddylist_msg module. Hopefully, we have all the hooks needed to do that.

Here's a patch against v.1.68.2.30.

Wouldn't it make sense to use the user objects? - which are already there in most cases. So implemented modules need not run user_load() again.

nodestroy is already implementing the same hook for the buddy_api module of buddylist 2.x - so I think it would make sense to use unique function signature. If I have it in mind correctly, he is using

hook_buddy_api($op, $user1, $user2, $one_way)

$one_way is just a boolean representing the setting, which we can omit buddylist 1.x. Nevertheless buddylist 1.x implementations would work with buddylist 2.x too then, as php allows calls like

foo($arg1, $arg2) for a function

function foo($arg1)

yes, thats true.
my function looks like this: function buddy_api_invoke($op, $thisuser, $otheruser, &$oneway) {
in most cases the user object has to be load befor the call of this function (confirm form,..) so its obvious to use this userobjects as function parameters.

That makes much more sense. Here's a patch with the user objects.
I actually had a boolean parameter too, but its sense was reversed ($reciprocal rather than $oneway) - I have changed it in this patch, so it is now $oneway too.

Ok, finally had time to work this one.

I modified the patch to add some more ops in preparation for moving drupal_set_messages into a separate module. I add in support for request, cancel, deny used in approval mode.

I think I have all calls using the same order for the accounts so that the first user object is the target of the operation and the second is the logged in user (account being affected). This gets confusing so please check it closely.

I've tested both modes and it seems correct. Please verify - and also the two contrib modules which I didn't test.

I used a simple implementation of the hook to test.

function buddylistext_buddylist($op, $user, $thisuser, $oneway) {
  drupal_set_message("$op => " . $user->name . " " . $thisuser->name . " Oneway? " . drupal_to_js($oneway));
} 

I need some reviews of this patch please.

"the first user object is the target of the operation and the second is the logged in user (account being affected)"

I don't understand what this means. The logged in user might not be involved in the operation at all, and "target of the operation" and "account being affected" seem to say the same thing. For example, suppose an administrator approves or denies user X's pending request to be added to user Y's buddylist. Which user is the target of the operation, and which is the account being affected?

I see what you mean - I guess that's why it gets confusing. I hope that when the logged in user is making changes to there buddylist then the order is as I described it.

ie. if the logged in user requests to add userA then then the order would be - userA, $user.

Sorry for the confusion, let me know if the patch works as desired.

Thanks.

I agree, it does get confusing!

Going by the rule that the logged in user, if involved in the operation, should be second - then, everything in buddylist.module looks fine, but the one in buddylistautoadd.module seems to be the wrong way round. I haven't had time to test buddylistinvite.module, yet.

I reversed the order for the autoadd.

New patch.

Finally got around to installing the invite module, tested buddylistinvite with this and it seems to be fine; buddylistautoadd is now right too.

John,

I really appreciate your help and thorough testing. I'll give a day to see if there is any feedback from http://drupal.org/node/172955 and then I'll get this committed and get going on a couple of other patches in the queue and maybe some new stuff.

Thanks.

Revision 1.68.2.32

Thanks everyone