I was attempting to implement message_digest in one of the sites I'm working on and noticed
that the digests did in fact send as expected but the contents of the message were incorrect.

On the site we have a variety of different message types and a single node insert can create
many messages of many different types that need to be digested. I started digging a little more
and noticed that message_digest, when inserting messages into the message_digest table, selects
the mid for the message to insert by querying mids from the message table that have the same timestamp
as $this->message and takes the first result it finds.

In my case multiple messages of varying types fall under that timestamp criteria but recipients
would simply all receive the same message. What I ended up doing to ensure that recipients received the
right type was add another condition to the query(line 26, abstract.inc): query ->condition('type', $message->type), which
ensures the correct type is sent. I wanted to get your thoughts on this solution as I am new to drupal.

Also I noticed this comment (line 24, abstract.inc):

// Our $message is a cloned copy of the original $message with the mid field removed to
// prevent overwriting (this happens in message_subscribe) so we need to fetch the mid manually.

Would you mind explaining what you mean by 'prevent overwriting' and why this happens in message_subscribe?
(I am using both message_subscribe & message_notify directly to send mail.)

Idealy I would like to send the message based on the mid of $this->message but the current implementation
suggests that this is not possible, would you mind clarifying?

CommentFileSizeAuthor
#7 message_digest-support-for-multiple-message-types-2115387-7.patch664 bytesanemirovsky
#6 message_digest-support-for-multiple-message-types-2115387-6.patch1.14 KBanemirovsky
#2 message_digest-support-for-multiple-message-types.patch1.11 KBbenstjohn
#1 message_digest-support_for_multiple_message.patch1.22 KBbenstjohn

Comments

benstjohn’s picture

benstjohn commented
StatusFileSize
new message_digest-support_for_multiple_message.patch1.22 KB

This a possible solution to the above mentioned issues. This patch only runs the query to grab the mid if it is a message_subscribe message. The query has also had a condition added to ensure the correct message type is retrieved from the message table. Any input would be greatly appreciated :)

benstjohn’s picture

benstjohn commented
StatusFileSize
new message_digest-support-for-multiple-message-types.patch1.11 KB

Created patch from the wrong directory, here is the correct patch.

mcrittenden’s picture

mcrittenden commented

Thanks a lot! Committed and pushed to 3773257

Re: the cloning, I'm not really sure why message_subscribe does it that way. The relevant code is here (from message_subscribe_send_message in message_subscribe.module):

  foreach ($uids as $uid => $values) {
    $last_uid = $uid;
    // Clone the message in case it will need to be saved, it won't
    // overwrite the existing one.
    $cloned_message = clone $message;
    unset($cloned_message->mid);
    $cloned_message->uid = $uid;

    $values += array('notifiers' => array());

    // Send the message using the required notifiers.
    foreach ($values['notifiers'] as $notifier_name) {
      $options = !empty($notify_options[$notifier_name]) ? $notify_options[$notifier_name] : array();
      $options += array(
        'save on fail' => FALSE,
        'save on success' => FALSE,
      );  

      message_notify_send_message($cloned_message, $options, $notifier_name);

      // Check we didn't timeout.
      if ($use_queue && $subscribe_options['queue']['end time'] && time() < $subscribe_options['queue']['end time']) {
        continue 2;
      }   
    }   
  }

See how it unsets the mid and then passes the cloned message to message_notify_send_message? This makes our life a lot harder, as you've seen. The comment obviously says that that's so we don't overwrite the existing one if/when saving, but I'm not exactly sure what that means. Either way, that is why I had to go roundabout with the timestamp crappyness.

mcrittenden’s picture

mcrittenden commented
Status: Active » Fixed
benstjohn’s picture

benstjohn commented

Yeah I was looking at that myself the comment is clear enough but I'm not entirely sure when this message could be 'saved' anyways its not a big problem as message subscribe only creates one message and sends it out to all subscribers, assuming the message type is correct people will receive the right mail.

benstjohn’s picture

benstjohn commented
Issue summary: View changes

Fixing typo

anemirovsky’s picture

anemirovsky commented
Issue summary: View changes
Status: Fixed » Needs review
StatusFileSize
new message_digest-support-for-multiple-message-types-2115387-6.patch1.14 KB

Downloaded the latest beta which includes the patch from #2115387-2: Support for multiple message types. and am getting the following error:

Notice: Undefined property: Message::$mid in deliver() (line 24 of /var/www/mqgsocial/sites/all/modules/contrib/message_digest/plugins/notifier/abstract.inc).

I've added an updated patch that should correct this.

anemirovsky’s picture

anemirovsky commented
StatusFileSize
new message_digest-support-for-multiple-message-types-2115387-7.patch664 bytes

Re-rolled the patch against 7.x-1.0-beta1 in case that's more helpful.

mcrittenden’s picture

mcrittenden commented
Status: Needs review » Fixed

Thanks! Pushed #6 to .dev with commit 09e0d.

Status: Fixed » Closed (fixed)

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