Provide api function for other modules to send messages.

Thats on the to do list.

A simple
^-^

/**
 * Send private message. Sender is the current user.
 *
 * @param int|object|array $recipients
 *  - A user ID;
 *  - A user object containing the recipient;
 *  - An array of user objects containing the recipient.
 * @param string $subject
 *  The subject of the message.
 * @param string $body
 *  The body of the message.
 * @param int $thread
 *  The thread this message belongs to (take care to preserve on replies).
 *
 * @return void
 */
function privatemsg_send_privatemsg($recipients, $subject, $body, $thread = NULL) {
  global $user;
  //1) save the message body first
  db_query("INSERT INTO {pm_message} (mid, subject, author, body) VALUES (null, '%s', %d, '%s')", $subject, $user->uid, $body);
  $mid = db_last_insert_id('pm_message', 'mid');
  $thread_id = $thread ? (int) $thread : $mid;
  //2) save message to recipients
  $timestamp = time();
  if (is_numeric($recipients)) {
    $recipients = user_load(array('uid' => $recipients));
  }
  elseif (!empty($recipients['uid']) && is_numeric($recipients['uid'])) {
    $recipients = user_load(array('uid' => $recipients['uid']));
  }
  $query = 'INSERT INTO {pm_index} (mid, thread_id, recipient, author, timestamp, new) VALUES (%d, %d, %d, %d, %d, 1)';
  if (is_object($recipients)) {
  	db_query($query, $mid, $thread_id, $recipients->uid, $user->uid , $timestamp);
  }
  elseif (is_array($recipients)) {
    foreach ($recipients as $recipient) {
      db_query($query, $mid, $thread_id, $recipient->uid, $user->uid , $timestamp);
    }
  }
}

why not just call the pm_send function?

Just thinking out loud...

Of course, there needs to be a send privatemsg api function, the signature could look like similiar to the one postet above, for example:

function privatemsgapi_send($title, $body, $recipients, $author = NULL)

- What does $recipients contain ? ID's, Strings, user objects ? => how intelligent should the function be ?
- I don't think we need a $thread_id, answering a existing thread doesn't need $title nor $recipients imho, as we don't want to allow to change this information. (Simply because the module is unable to handle something like this at this time... See the discussion in #324389: Show author(s) when viewing inbox), perhabs provide another, simplified function for answering an existing thread:

function privatemsgapi_answer($thread_id, $body, $author = NULL)

- If $author is null, then use global $user

- What about other interactions... ? delete, set_read, load... ?

- And, we need a hook to inform other modules about changes in a thread/message (notifications etc.) => only inform or do we want to give them the possibility to change the message?

function hook_privatemsg_alter(&$message, $action, $owner)

- $message is an array containing all information of the message that is currently "worked on" => does not integrate nicely with the api calls, any other ideas ?
- $action is clear, possible values (this does have influence on the contents of $message): new, answer, delete, read (?), for answer/new we could also provide before and after actions (on after we do know things like mid, timestamp and thread_id, before would allow to change the message
- $owner is the current user... the autor, the reader, the one whose message gets deleted (doesn't need to be the current user)

Ideas, feedback ?

Also thinking out aloud

- What does $recipients contain ? ID's, Strings, user objects ? => how intelligent should the function be ?

I think we are moving towards using the full object. It saves other modules from having to reload any user data when they hook into processes.

As for scope or what to do, I have no idea.

A good way to decide what is needed may be to alter pm_send to pass data to "privatemsgapi_send". That should give an idea as to what is needed.

We do need a $thread = NULL in there, and maybe a way to pass additional variables that are not central to privatemsg, or we have not thought of yet. I would prefer if there was a single function that could be called for as many reasons as possible - why have two apis, one for a new message and another for a reply?

PS if there is ever a need to prefer no threading, the current module can be allowed very easily to allow that. Just put the thread info inside an if check for a variable. if a thread_id is not passed along, the module will create a new thread.

I think there is one reason for splitting up the api calls -> Making code more understandable. If we have one method for "everything" we end up with something like the following if we answer an existing thread:
privatemsgapi_send(null, $body, array(), null, $thread_id);
This is exactly what webchick talked about at http://webchick.net/6-pass-patch-reviews (pass 2).

Another approach would be the use of an array.
privatemsgapi_send($message);

The advantages are that it is very flexible and would already be in the right form for my proposed hook function. And array-parameters are somewhat "usual" in drupal... menu, fapi and so on. It would however need very clear documention (if thread_id is empty it starts a new thread and so on) while privatemsgapi_send($title, $recipients, $body) is quite self-speeking.

And yes, user objects are probably the best way, the only problem could be memory consumption (if a module provides a send to all function on a big site, for example)

Ok, here is what I currently have as a patch. The functions are currently in privatemsgapi.inc, but can easily be moved to privatemsg.module.

- There are two levels of abstraction. Easy-usable functions for other modules to create threads and answer on them and an internal function that does the actual sending

- Internal: _privatemsg_send($message). This is more or less a copy of the current pm_send() function, it simply takes the array, calls some hooks and inserts the data into the db. No validation or whatsoever is done in here. In a second step, pm_send can be rewritten to use this function. The main difference between them is that _privatemsg_send does not output anything while pm_send would call _privatemsg_send() and add a message indicating that the message was sent.

- Top-Level: privatemsg_new_thread($subject, $body, $recipients, $author = NULL) and privatemsg_answer($thread_id, $body, $author = NULL). These functions validate the message, create a array and call _privatemsg_send. privatemsg_answer does also read the subject and recipients from the selected thread.

- privatemsg_get_link($recipient, $account = NULL): This function does return a link which can be used by url/l to generate a link. It does *not* use l itself because it's impossible to be flexible enough, see http://drupal.org/node/326979#comment-1111420

- Hooks: I'm currently using the presave, insert and validate hooks I already described at #350601: Creating a hook, allowing other modules to add functionality

Open/Next things to work on...
- Generic review of the current code...
- Are there additional functions/hooks we need? Probably delete...
- Maybe supporting multiple recipients for privatemsg_get_link (privatemsg_new would need to be changed too..)
- Rewrite privatemsg.module to use _privatemsg_send, validation hook and so on...
- ...

Do we really need different functions for new thread and reply and message send? i'm not sure we want to force other developers decide which one to use. i think all message sending interface should be done (if at all possible) through a single send function and then privatemsg.module would then figure out if its a reply or a new thread or what ever.

Hm..

I am not sure about this. Actually, I'm not even sure if we need the possibility to answer existing threads via an api call at all. But if we do, a separate function is imho better, because of the following reason:

We could also add an additional thread_id attribute to privatemsg_new_thread and rename it to privatemsg_send or something like this. Then we have 2 possibilites:

1. Still let the user set subject and recipients. This means that someone using the api needs to know about privatemsg internals (how recipients are stored and so on), imho, because we don't want that, we are defining api functions at all... so I would say, this is a no-go.

2. Ignore those to parameter and load them, just like privatemsg_answer this currently does. However, an api call to answer an existing thread would then look like this (assuming the above): privatemsg_send(NULL, 'some text', NULL, NULL, $thread_id). This looks just ugly to me.I already mentioned webchick's blog post above, that's exactly what she was talking about:

This is a great way to catch things like some_function(NULL, TRUE, array(), $booger, 'aardvark'). Without benefit of having seen the PHPDoc for some_function() ahead of time, if the calling code makes no sense, we have some work to do. This work could involve renaming some_function() to something that makes it more obvious what it's for, or it could involve refactoring some_function() into several functions that each take parameters specific to their functionality, or it could just involve re-formatting the way options are passed in. At the very least, it means we need some inline comments to explain what it does.

The "...refactoring some_function() into several functions that each take parameters specific to their functionality... " is what I was trying to do...

However, the current patch is just hacked together and I'm sure that it can be improved. The validation part could be joined into one function, so that both functions would look like:

  // .. function header....

  // put $message together

  $errors = _privatemsg_validate_message($message)
  if (!empty($errors)) {
     return $errors;
  }
   else {
     return _privatemsg_send($message);
  }
}

But, this is only my point of view...

I'm assuming the above validation example is part of privatemsg_send function. otherwise it doesnt make sense to trust developers to perform correct validation every time. imho.

Yes of course, this is part of the two top-level functions...

I updated them as written above, this time with patch, forgot it above...

I also created a test case for the API functions, but there isn't much there currently. And SimpleTest seems to be quite broken on d6...

Edit: As I was changing privatemsgapi.inc, I also fixed some Coding Style Issues and removed those group-functions as they are not used (anymore ?)

Some updates...

- moved api functions to privatemsg.module, to reflect changes in #350344: Remove privatemsgapi.inc from module
- changed pm_send and pm_send_validate to use the api functions -> seems to work but not very well tested

No changes to te UnitTest, but it has to be saved in the tests directory to be recognised by SimpleTest.

I started to work on #342733: Create upgrade path from privatemsg_attachments to filefield to see if the API works for such things. Several changes/improvements to make it work, in no partical order..

- Added #weight attribute to the buttons in privatemsg_new. This does make it possible to add things between the buttons and the textfields.
- Added a new attribute "#privatemsg_message" to the privatemsg_new form. This is similiar to #node in the node form and allows to save things like uploads in the cache while previewing/multiple upload.
- Added a privatemsg_message_delete hook, by loading the message before deleting it and calling the hook after the message has been marked as deleted. This needs still work, see below
- Some small changes in the validation logic
- hook privatemsg_message_insert called by module_invoke_all instead of drupal_alter.. no need to alter $message at this point
- Added privatemsg_message_load hook in _privatemsg_load. Does work like nodeapi load, return values are merged with $message

Open things...

Delete:
- There are two different levels of "delete"... 1 => User x deleted message y, he simply doesn't see it anymore. 2 => All users have deleted message y. Stuff like a attachments can now be deleted. With the current implementation, the hook privatemsg_message_delete does not distinguish between them, I am not sure if and how this should be done. Maybe an additional parameter $deleted_by_all. Other ideas?

This is the code I currently use in privatemsg_attachments to check this:

  $result = db_query("SELECT MIN(deleted) AS deleted_by_all FROM pm_index WHERE mid = %d", $message['mid']);
  $deleted = db_fetch_array($result);
  if ($deleted['deleted_by_all'] == 0) {
    return;
  }

- There is still no delete api call, should we provide one and if yes, on thread and/or message level?

- Should we load the message for the delete hook, or let the sub modules load additional $message data if they need it: hook_privatemsg_message_delete($message) or hook_privatemsg_message_delete($pmid, $account)?

Show:
To show the attachments, my attachments module currently has a module_preprocess_privatemsg_view function, where it does extend $vars['message_body']. However, this does only work if privatemsg_preprocess_privatemsg_view does get called first. We may need a view/show hook to, one possibilty would be to replace the current privatemsg_pm_controls hook with something that can alter the whole $message, instead of just adding action links. Ideas ?

Caching/performance:
With multiple hooks, actions etc, a thread display can get quite heavy. Probably not in this issue (it's already big enough...), but we should think about caching and other things to improve performance...

Some updates, I think it is pretty ready now...

Changes... (in the way the they occur in the v3-> v4 patch diff :) ):
- Added a 'privatemsg_message_view_alter'-hook in privatemsg_preprocess_privatemsg_view. This one does replace the action-specific "privatemsg_pm_controls"-hook. The hook has a single parameter "$vars". This is needed because the privatemsg preprozess function is the first that needs to be run. If we would explicitly set the weight of privatemsg and all related modules, we could get rid of it.

- Added a privatemsg_message_delete function. It gets called by privatemsg_delete_submit.

- Moved the delete_by_all check (see above) to privatemsg_message_delete and added a new parameter $deleted_by_all to hook_privatemsg_message_delete

- Removed the privatemsg_per_message_actions function, $vars['message_actions'] is a array now and submodules can add things with hook_privatemsg_message_view_alter. Moved the generation of the html to the template file

- Removed privatemsg_privatemsg_pm_control, moved to privatemsg_preprocess_view (It's just one line and with the above change we don't need to use a hook in privatemsg.module itself

- Updated privatemsg_get_link to check uid of recipient and author

- Updated privatemsg.author-pane.inc to use privatemsg_get_link

- Updated pm_block_user.module to use hook_privatemsg_view_alter instead of hook_privatemsg_pm_control. While doing this, I saw that the module currently makes 3 queries per message display. Thanks to the $vars parameter and a hook_privatemsg_load_alter I managed to switch 3 single queries against 1 LEFT JOIN.

Follow-Ups/TODO's:
-Document all the ways one can now extend and alter privatemsg. Probably as a page in the Drupal Documentation, I need to find time for that...

- Add API functions for thread load and deletion. Maybe as part of #348907: Per thread/Multiple thread actions or #328049: No headline shown when i send a message with user ID.

- Think about caching/performance: I did a short test and cached the $message array generated by privatemsg_load, but it wasn't really useful. I think, we should cache on thread level instead.

- Get testers and feedback :)

----

About the feedback part... I know, this issue is harder to understand and seems to be less important than others as it is not directly UI related. However, important features like email notification and attachments depend on this functionality. So It would be great if some people could install the patch and report if it works. I'll even attach all changed files so the only thing to do is unzip them. However, I don't recommend to do it on a live site :)

Hrm... the patch itself is bigger than the zipped version of all changed files :)

Can you attach the patch again please?

I have been on the sidelines for a while now but I need to jump back in. This should be as good a point as any other.

What the f... I'm going crazy, I'm 100% sure that I attached the patch.

with this patch, I cannot seem to view the message contents.

I can see the action buttons but nothing else - the actions do not have a pmid at the end either?

That bug I mention above is when the block user messages module is enabled.

Ok, I'll look into it.

Hm, I have an idea why it does break on your site. Can you please execute the following execute the following sql statement?

UPDATE system SET weight =5 WHERE name = 'pm_block_user'

Note: This is a temporary workaround. Instead of again hacking around this issue with array_unshift an similar tricks, I'd like to fix this via: #360021: Improvement to _privatemsg_assemble_query. Comments are welcome ;)

yup, that fixes it. as for the other issue, it does seem to make things simpler/nin-hackish so I do agree, but since I do not know too much about databases, I considered that an uninformed "me too"...

since the bug was not here, setting back to cnr for now.

I am still uncomfortable for different functions for start a new conversation and to reply to one. I do not think those using the API should have to care either which way...

@nbz: ping me in IRC when you get a moment.

Hm. Seems that I wasn't really successful in convincing you :)

My primary reason for splitting them up is still to make those functions easier to understand/use, see comment #7.

I wasn't able to come up with a function definitition that works for both use cases, so I splitted them up. I also thought if we really need an api function to answer threads, but it could be nice for a auto-responder/away-message system for example.

Another thing that's not really good are the function names. Maybe privatemsg_reply would be better as privatemsg_answer, now idea how to improve new_thread.

No probs - that was just my concern.

Another thing - in privatemsg_answer, the participants are set, but the blocked users are not removed. This will just send notices of undelivered messages in cases where it may not really needed to do that.

As for button weights - should there be a numericla gap between them just in case other modules want to insert something in between?

line 42 od patch as available spelt wrong, but I never looked at such things past that.

Apart from that it looks good to me.

For my original problem above, do we need an update function or is the problem something else?

Another thing - in privatemsg_answer, the participants are set, but the blocked users are not removed. This will just send notices of undelivered messages in cases where it may not really needed to do that.

Hm, that part is a bit tricky, I haven't found a better solution yet. Currently, it works in the following way:

- I moved the privatemsg_block_message stuff to _privatemsg_validate_message, because we need to do this in answer, new_thread and the form validation.

- However the messages about blocked users should only be displayed when a users tries to send a message with the form. So I added a $show_warnings flag to _privatemsg_validate_message, so that the function only displays blocked user notices when it's set to true

- So, privatemsg_answer adds all recipients and maybe some of them are removed at a later point, but this shouldn't be a big performance hit, because we need to user_load them anyway.

As for button weights - should there be a numericla gap between them just in case other modules want to insert something in between?

Hm, good point. I doubt that anything like this will ever be done, but there is no downside of doing it so, i changed them to 10, 15 and 20 and also added #weight to the other fields.

For my original problem above, do we need an update function or is the problem something else?

If the mentioned issue about _privatemsg_assemble_query above gets in first, I can re-roll that patch and the update isn't needed anymore. Would imho be easier

Also fixed two small E_NOTICE bugs when previewing a message.

re-roll of patch as there were some minor conflicts.

- privatemsg_get_link does now check the "read privatemsg" permission of the recipient
- some code style issues in the changed code

It works and looks good to me (there is a small offset for one of the patch hunks though). I will set it to rtbc.

IMO there is a need to pass through from load to display wether the user has read that message before or not, but that would be a separate feature request which should not hold this patch up.

PS the RTBC is conditional that this should go in AFTER #360021: Improvement to _privatemsg_assemble_query.

IMO there is a need to pass through from load to display wether the user has read that message before or not, but that would be a separate feature request which should not hold this patch up

Care to explain this a bit more ? $message['is_new'] / $row['is_new'] is 1 if the user hasn't read the message, isn't that what you mean?

Note: The patch needs to be re-rolled after #360021: Improvement to _privatemsg_assemble_query goes in, as the function name of the sql hook does change.

PS: I started to document all those functions and hooks, still much to do but it is a start: http://drupal.org/node/369399

yup - it will need to be rerolled.

Care to explain this a bit more ? $message['is_new'] / $row['is_new'] is 1 if the user hasn't read the message, isn't that what you mean?

yup, but added into privatemsg_privatemsg_messages_alter/privatemsg_sql_messages and then passed into the theming layer so that for each individual message can have a "new" display/anchor when viewing the message. Actually this is totally different from this patch anyway.

Just re-rolling this atm.

I've already done it, uploading soon...

Ok, rerolling patch as #360021: Improvement to _privatemsg_assemble_query has been commited (Hooray! :) )

Some minor changes:
- renamed sql hooks and assemble calls
- added first bits of apidoc, more will follow in an own patch later
- added $message['user'] = $account in privatemsg_load, so that hooks know which user is doing stuff with the message

yup, it works.

Forgot to add the updated test file. It doesn't have much tests, only some simple validation/permission tests. It should be added to the tests folder.

Hi, I'm using the "Flag Friend"-Module which has been developed by sirkitree and provides a very nice friendlist-function for D6. I think it would be very useful if the to modules "Privatemsg" and "Flag Friend" could be combined.

Unfortunately I'm no programmer so I don't know anything about the module coding but I assume that combining "Privatemsg" with other modules is what you are talking about here :) ?

I posted my request already in the "Flag Friend"-Project (http://drupal.org/node/372226) and sirktree replied he would review a possible patch.

Now my question is: Is it possible that any of you guys would have a closer look on this issue? That would be great!

(Please note: I'm quite new in the Drupal community so please forgive me, if this is a unusual way to post a request)

Thanks & regards!

removed double post

@joe9: Move feature request to new issue to avoid highjacking original issues ;)

Ok, hopefully the last update.

- Changed $uid parameter to $account in sql_load, sql_messages and _privatemsg_load()
- Use $account parameter instead of $fragments['query_args'][0] in privatemsg_filter_privatemsg_sql_list() - Don't ask me what I was thinking then.. :-/
- added apidoc for privatemsg_get_link

@joe9
Feel free to use http://drupal.org/node/373044 as a start for integrating your module with Privatemsg. If there are any question, just ask and I'll try to improve the example.

Get this to test...

I have tested this - it applies cleanly and seems to work well, but I will give it another once over tomorrow before marking rtbc (unless I am beaten to it by someone else).

extra tests never hurt :)

@litwol: o.k. :) I have already opened a feature request in the "Flag Friend" - Project (http://drupal.org/node/372226) and I won't handle this issue within this node anymore. My intention was to match the right people for this issue :)

@Berdir: I'm afraid I won't have any questions since I don't have a clue about this :) But as you have posted your comment also in the feature request for this (http://drupal.org/node/372226) there might be some other people coming up with questions. Thank you very much & best regards!

Works well but restriction for non-empty subject is not good, IMO if subject is empty it should filled with default - "no subject" for example in listing of new message

After IRC there's idea to leave this to another issue to reduce this patch

To explain this a bit more, the current version allows to make the subject and body field to be optional by using hook_form_alter. However, _privatemsg_message_validate doesn't allow that anymore.

The idea is to create a follow-up patch which adds a configuration option to make the subject optional and prefills it.. either with "no subject* or with substr($body, 0, 20).

However, if I keep adding features to that patch, it will never be commited :)

1) removed

      || $recipient->uid == $account->uid

from privatemsg_get_link because we should allow sending messages to self ? i know we used this function in places such as user profile and we dont want to show link to send message to oneself, however i feel that this API should be pure and return what is asked of it, a link to message to recepient's account and since the core of the module allows sending messages to self thus i felt its important to let this function do the same.

if it still make sense to not show link to message oneself in places like user profile page, then we should implement checks there to prevent calling this function.

2)
in privatemsgapi.inc we have this code:

function _privatemsg_load($pmid, $account = NULL) {
  if (empty($account)) {
    global $user;
    $account = drupal_clone($user);
  }

  $query = _privatemsg_assemble_query('load', $pmid, $account);

  $result = db_query($query['query']);
  $message = db_fetch_array($result);
  $message['user'] = $account;
  $returned = module_invoke_all('privatemsg_message_load', $message);
  if (!empty($returned)) {
    $message = array_merge_recursive($returned, $message);
  }
  return $message;
}
turn $message;
}

Notice the call : module_invoke_all('privatemsg_message_load', $message);
I scanned privatemsg code base and nowhere does it define hook_privatemsg_message_load. is this a deprecated or broken feature ?

great stuff ! This went in but now we need to fix the following critical bug:
critical bug: #376034: #288183 followup: user blocking doesnt work when sending to group of users that contains at least one non blocked user.

other patch review comments:
#376023: #288183 followup: change hook_privatemsg_block_message to work on multiple recipients
#376020: #288183 followup: possibility to allow empty subject on messages: in validate _privatemsg_validate_message
#376005: #288183 followup: _privatemsg_send always returns TRUE. change that to return more relevant information.
#375999: #288183 followup: Developer friendlify privatemsg_new_thread API.
#375980: #288183 followup: remove complex php logic from privatemsg-view.tpl.php

Thank you everyone!

> 2) I scanned privatemsg code base and nowhere does it define hook_privatemsg_message_load. is this a deprecated or broken feature ?
It is a not yet used feature :) Some things can't be done with hook_privatemsg_sql_load_alter, because there can be multiple elements, for example, privatemsg_attachments does use it to check and load attachments,

subscribe

I'm trying to use the new api function in a migrate integrations module, so that people can load in privatemsg messages programatically from a view , file or database (even an external one). I updated to DEV to use it, but looking at the code, I see a possible flaw.

function privatemsg_new_thread($recipients, $subject, $body = NULL, $options = array()) {
  global $user;
  $author = drupal_clone($user);

  $message = array();
  $message['subject'] = $subject;
  $message['body'] = $body;
  $message['recipients'] = $recipients;

IF the custom options are added before the defaults, won't they get overridden? ( i want to set timestamp myself)
// set custom options, if any

  if (!empty($options)) {
    $message += $options;
  }
  // apply defaults
  $message += array(
    'author' => $author,
    'timestamp' => time(),
  );

No, actually not.

I know, it's confusing, but "adding" arrays doesn't overwrite existing values. See http://ch2.php.net/manual/en/language.operators.array.php

ah, that's a handy bit of code. I can certainly use that in my development. sorry for my confusion :)

What is the reason to open this issue agian ?

sorry, didn't mean to reopen. I need to get the mid right after I send the message, how can i do that?

Asking questions in closed threads isn't a good idea neither, please open new issues for questions.

Currently, the only way to do that is either with "db_last_insert_id('pm_message', 'mid')" or with http://blog.worldempire.ch/de/api/function/hook_privatemsg_message_insert/1, the $message array will contain the mid at that point. But then you'd have a problem accessing your other data.

However, I think it makes sense to either return $message or $mid in _privatemsg_send() and the two functions that use it. Currently, the function only returns TRUE. We should continue that in #376005: #288183 followup: _privatemsg_send always returns TRUE. change that to return more relevant information..