Error sending private messages in 1.9 Beta 2

[henrywright]

I'm getting an error message There was an error sending that message, please try again when sending a private message. It only seems to happen in 1.9 Beta 2.

[boonebgorges]

I can't reproduce this. Can you give more details?

• are you working with an Admin level user?
• are the sender and recipient friends?
• are you composing a new message or replying to an old one?

[henrywright]

• No. I'm working with two standard members
• No. They're not friends. Actually the friends component is deactivated.
• It was a new message. I clicked on "Private Message" on the member's profile page.

I just tried again, this time by manually typing the member's username in the To field on the compose screen and got the same result. There was an error sending that message, please try again. Interestingly, I logged in as the recipient and the messages have arrived, even though the sender was shown that error message template notice.

[henrywright]

I have more feedback on this:

The recipient just received a email notification for the message which was sent. Here's the email message word for word:

Peter Wright sent you a new message:

Subject: Howdy everybody

""

To view and read your messages please log in and visit: http://jobtain.co.uk/members/henrywright/messages/

---------------------
To disable these notifications please log in and go to: http://jobtain.co.uk/members/henrywright/settings/notifications/

This indicates the private message body was empty "", or at least BP thought it was empty. I did write something in the 'Message' field - promise :)

[r-a-y]

henrywright: I followed your steps in comment:2 and also cannot duplicate.

Are you using any plugin or code snippet that manipulates the PM contents?

[boonebgorges]

Also, what theme are you using? Could possibly be related.

[henrywright]

r-a-y, boonebgorges, I'm using BP 1.9 Beta 2 with WP 3.7.1 and Twenty Thirteen. No custom code snippets. I did have root profiles enabled but removed that 2 days ago. ​http://jobtain.co.uk is the site I've been testing on.

More feedback - on submitting a new message I get 2 debug notices (I didn't see these when I tested earlier as they were half obscured by the WP admin bar). The notices read:

Notice: Undefined variable: content in /wp-content/plugins/buddypress/bp-messages/bp-messages-notifications.php on line 63

Notice: Trying to get property of non-object in /wp-content/plugins/buddypress/bp-messages/bp-messages-functions.php on line 119

Not sure if these are related?

[r-a-y]

In 7647:

Fix empty content in private message notification emails.

See #5273.

[r-a-y]

henrywright: My bad, I didn't check the emails that were generated by the PMs. I've fixed the empty email content bug in r7647.

Are you still experiencing the problem you originally noted?

[henrywright]

r-a-y, thanks!

I just re-tested. The first debug notice is gone and the email notification now contains the private message content. However, I'm still getting Notice: Trying to get property of non-object in /wp-content/plugins/buddypress/bp-messages/bp-messages-functions.php on line 119.

I'm still seeing the original problem.

[henrywright]

Done some digging and found commenting out line do_action_ref_array( 'messages_message_sent', array( &$message ) ); of bp-messages-functions.php resolves the original problem so the route of the issue must be something hooked to messages_message_sent

Now, messages_notification_new_message() and bp_messages_message_sent_add_notification are the two functions hooked to it so what inside those functions could be manipulating the $message object to cause it to not be an object?

A $message variable is used inside messages_notification_new_message() - could that be overwriting the $message object?

[r-a-y]

I haven't been able to duplicate the notice, henrywright.

What version of PHP are you using?

What happens if you change:
do_action_ref_array( 'messages_message_sent', array( &$message ) );

to:
do_action_ref_array( 'messages_message_sent', array( $message ) ); (EDIT: Sorry, typo!)

or:

do_action( 'messages_message_sent', $message );

[henrywright]

Changing it to either:

do_action_ref_array( 'messages_message_sent', array( $message ) );

or

do_action( 'messages_message_sent', $message );

solves the problem.

What's the reason for prefixing $message with an ampersand? Interesting why nobody else is seeing the debug notice.

[r-a-y]

We were passing the $message variable as a ​byref to the 'messages_message_sent' hook. This means that $message could be manipulated by other functions.

Not sure what the original rationale was as I think, in this case, the byref is unnecessary.

[boonebgorges]

I'm not sure why the param was originally passed by reference, but if we change it now, there's a slight possibility that we'll break people's customizations.

I'm not totally clear on why none of us are able to reproduce the error message that henrywright is giving - maybe there's an issue with the order in which WP is processing the callbacks that are hooked to 'messages_message_sent'? In any case, I'm guessing that 5273.patch will fix the issue (by not modifying the $message object passed to the function). henrywright, would you test please?

[henrywright]

boonebgorges: Just arrived in Spain for the weekend armed only with my tablet. Back at my computer Monday afternoon so will report back on the patch then if that's ok?

[henrywright]

With reference to trying to reproduce the issue, perhaps try with root profiles enabled. I had root profiles enabled originally on my test site but this was disabled during all tests I did. Aside from that customisation the test site was very ordinary (totally uncustomised).

[boonebgorges]

Sure, not a huge rush. Thanks, henrywright. (And yes, I'll try with root profiles.)

[joesell89]

I have the same error here. I would happily try to apply the patch but I don't know how. If it is important that it is tested then let me know and I will try.

[boonebgorges]

joesell89 - Thanks for checking in.

If you're on a *NIX machine (like Linux or OSX), do the following in a terminal (replace "/path/to/wordpress/" with the path to your WP installation and "/path/to/5273.patch" with the path to the downloaded patch)

cd /path/to/wordpress/wp-content/plugins/buddypress
patch -p0 < /path/to/5273.patch

If you only have FTP access or whatever, open up the file wp-content/plugins/buddypress/bp-messages/bp-messages-notifications.php, and manually make the changes you see in ​https://buddypress.trac.wordpress.org/attachment/ticket/5273/5273.patch. (Lines in red are the "old" versions, lines in green are the "new" ones. Basically, you're just adding the "raw_" prefix in a few places.) Don't do this on a production site unless you really know what you're doing!

[joesell89]

Thanks for talking me through that. Good news!

Before and after:

​https://cloudup.com/ctQ7y1mvUUC

[boonebgorges]

joesell89 - Thanks very much for testing! I'm going to go ahead and commit this fix.

henrywright, please do test when you get a chance. Thanks!

[boonebgorges]

In 7650:

Cast args to a new variable in messages_notification_new_message()

r7621 made it possible for this function to accept a $message object in
addition to an array, by casting the arguments to an array. However, this
caused downstream problems when the $message object was passed by reference,
as in the case of 'messages_message_sent'. This changeset leaves the passed
object untouched.

Fixes #5273

[johnjamesjacoby]

Apologies for the regression; thanks for your diligence with this one.

[henrywright]

boonebgorges: Just tested the patch - works great! Original issue resolved. All debug notices resolved.

[henrywright]

Hi guys - apologies for having to reopen. I've found another downstream problem caused by this.

'item_id' => $message->id is being done inside bp_messages_message_sent_add_notification(), which is resulting in item_id holding an erroneous value.

Background info: $message is passed to bp_messages_message_sent_add_notification() which is hooked to messages_message_sent.

[r-a-y]

I can't duplicate this on latest trunk.

To be sure, I dumped the contents of $message on the 'messages_message_sent' action and the $message->id was set for me when composing a new message and also when replying to one.

What's the exact debug notice you are seeing, henrywright?

Can anyone else test?

[henrywright]

r-a-y, i'm not seeing a debug notice.

How I came across the problem:

I'm filtering bp_messages_single_new_message_notification to make single new message notifications link to the message thread and not to the inbox (inbox being the default link destination). To do this, I needed to use $item_id in my filter function (it holds the value of the message thread ID).

i.e. I wanted to do something like this:
<a href="/members/username/messages/view/' . $item_id . '">visit message thread</a>

When I use $item_id in my filter function, it doesn't seem to hold the value of the message thread ID. Hence how I discovered there may be a problem. And I think this is related to what we've been discussing in this ticket.

This may be a long shot but perhaps try to reproduce with root profiles enabled?

[henrywright]

Just to add $message->id does seem to be set, however, the value it holds isn't the expected message thread id.

[boonebgorges]

henrywright - Would you mind testing 5273.02.patch?

[boonebgorges]

Scratch 5273.02.patch, I misread your report. I'll look more into it.

[henrywright]

Hey Boone, thanks for looking into this. I think I've found the root of the problem. It wasn't related to the $message object being passed by reference after all. Please see the attached patch which ensures the proper value (the message thread ID) is assigned to 'item_id'.

[r-a-y]

Okay, thanks for explaining that you're attempting to use the 'bp_messages_single_new_message_notification' filter, henrywright.

We should probably create a new ticket for this, but the problem you state is not a regression.

BP 1.8 also set the 'item_id' to the $message->id (see the ​source).

There are two approaches to address this:

1) Change the notification 'item_id' to $message->thread_id

If we use henrywright's patch so the notification'item_id' is set to $message->thread_id, current plugins that are already using the 'item_id' look up to check for the $message->id will no longer work. (I think the chances of this happening are pretty slim, but just stating the consequences.)

On the other hand, this would also fix bp_messages_screen_conversation_mark_notifications() as that is referencing the thread ID and not the message ID.

2) Grab the thread ID when formatting notifications

Ideally, when formatting single message notifications, we should be using the message thread permalink so we can take advantage of the new mark as read feature like you stated, henrywright.

I've attached an initial patch with this in mind. I also changed the strings a bit, which is kind of a no-no since we're so close to release, but if this isn't desired, we can remove the string changes.

bp_messages_screen_conversation_mark_notifications() also works with the message ID now, but is a little hacky and I don't like the usage of the foreach loop. BP_Notification_Notification::update() (and consequently wpdb::update()) is a little limited as we can't pass item_id as an array, which is why I had to resort to this.

I'm not a fan of the multiple DB queries in the foreach loop as well as when using new BP_Messages_Message() to get the thread ID.

Until I get some feedback from an additional dev, what you could do in the meantime is grab the message thread ID by looking up the message in your filter override function. Something like this:

$message = new BP_Messages_Message( $item_id );
$thread_id = $message->thread_id;

[henrywright]

r-a-y of course! I should never have assumed item_id was supposed to hold the message thread id! I think I'll just take note never to assume in future :)

Grateful if you guys let me know what you decide on this as then I'll know if I need to create a new object in my filter function (thanks r-a-y for the suggestion!) or simply use item_id as the message thread id.

My vote is to ignore me and leave things as they are. Now I know what item_id is intended to be, it actually makes far better sense because plugins can do things like r-a-y's suggestion (i.e create a new message object).

[boonebgorges]

This is a pretty big change this close to 1.9. Since it's not a regression, let's consider r-a-y's patch as an improvement on a future milestone (and a separate ticket, since it only seems marginally connected to the original purpose of this one). Thanks!

[r-a-y]

As per comment:36, I've created a new ticket for my changes - #5479. Closing this one!