Database Errors for multiple notifications

[mike80222]

Wordpress 5.9.2
BuddyPress 10.2.0 Legacy or Nouveau
Either 2022 or 2020 Themes.

(1) Send two private messages to someone.

(2) That person clicks on one of the notifications, and the error happens
Sometimes it happens if the recipient just refreshes their profile (and hence the notifications)

Note: There need to be multiple private message notifications. If there’s just one notification the error doesn’t appear to happen.

More info in: ​https://buddypress.org/support/topic/database-errors/#post-323944

This is the way the error looks with Nouveau:

[31-Mar-2022 17:41:07 UTC] WordPress database error You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ') AND user_id = 2 AND component_name = 'messages' AND component_action = 'new...' at line 1 for query UPDATE wp_bp_notifications SET is_new = 0 WHERE item_id IN () AND user_id = 2 AND component_name = 'messages' AND component_action = 'new_message' made by do_action('wp_ajax_messages_get_thread_messages'), WP_Hook->do_action, WP_Hook->apply_filters, bp_nouveau_ajax_get_thread_messages, bp_thread_the_message, BP_Messages_Thread_Template->the_message, do_action('thread_loop_start'), WP_Hook->do_action, WP_Hook->apply_filters, bp_messages_screen_conversation_mark_notifications, bp_notifications_mark_notifications_by_item_ids, BP_Notifications_Notification::update_id_list

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/8676

[oztaser]

Hi there,

Welcome to BuddyPress Trac! Thanks for the report.

I think the reason of this error is fetching new_message notifications from other threads instead of displayed one.

Here is my debug data to see how we find empty $message_ids array:

<?php
$unread_message_ids_from_notifications = array( 4, 5, 6 );
$message_ids_from_thread                           = array( 1, 2, 3 );

$message_ids = array_intersect( $unread_message_ids_from_notifications, $message_ids_from_thread );
print_r( $message_ids );

[imath]

hi @mike80222, thanks a lot for your report 😍.
Hi @oztaser thanks a lot for your patch 💪.

I'm going to look at it asap, in the meantime could you include one or more unit tests to show a failure without the patch which is resolved once the patch is applied 🙏?

[imath]

I wasn't able to replicate. When I send more than one Private message to a user, there's only one screen notification displayed into the WP Admin Bar, see below:

If I click on it, everything is fine for me.

[mike80222]

@imath I was talking about in the user's profile, not in the admin bar. Sorry I wasn't clear about that.

[oztaser]

Hi @imath,

To see this PHP warning, send a some user private messages from two different user (that means two thread) and open the single view page one of the threads. https://buddypress.test/members/test2/messages/view/5/

As I said my previous comment we are fetching other thread message ids in thread_loop_start action hook to mark message notifications as read but these message ids are not belongs to current thread so we're using empty array.

I don't know how to write unit test for this situation I think we need to write some test for BP_Notifications_Notification::update_id_list function.

[mike80222]

@imath, @oztaser,

I believe you should also be able to cause the database error by sending two messages from a single user, with the two steps I originally mentioned -- but bearing in mind that in (2) I meant that the recipient clicked on the notification in their *profile* not in the admin bar at the top.

[imath]

Hi thanks a lot to you both for the complementary explanation. I’m looking at it asap 👍

[imath]

I've been able to reproduce. I think this case is happening because at the time we're trying to get all notifications, the one corresponding to the displayed thread has already been marked as read. So we get the one that is left out of the 2 notifications and this one doesn't belong to the thread.

@oztaser I agree your PR is fixing the issue and we could write a unit test out of what I just described above.

But I believe we need to understand why "the one corresponding to the displayed thread has already been marked as read" before the 'thread_loop_start' is fired. Because this means we could avoid 2 superfluous queries.

[imath]

Bingo > ​https://github.com/buddypress/buddypress/blob/master/src/bp-messages/actions/view.php#L51|L58

So I believe a better fix is to return early from bp_messages_screen_conversation_mark_notifications() if we're viewing a single message. Maybe by using a more precise condition at line 228, something like:

if ( ! bp_is_my_profile() || ( bp_is_current_action( 'view' ) && bp_action_variable( 0 ) ) ) {
	return;
}

@oztaser could you have a look at it and if you have the same diagnostic I have (then we don't need unit test), can you update the PR accordingly ?

[oztaser]

Hi @imath,

Thanks for review and suggestion. I've updated the PR. This new approach make more sense to me as well.

[imath]

In 13269:

Avoid 2 superfluous private message notification queries

When viewing the single view of the thread, the potential notifications about it are marked as read into the messages_action_conversation() function. We don't need to do it again when looping into the thread messages.

This was revealed by a database error which was appearing when a user who had received more than 1 message from two different threads was directly viewing the single view of one of the threads.

Props mike80222, oztaser
Closes ​https://github.com/buddypress/buddypress/pull/14
See #8676 (trunk)

[imath]

In 13270:

Avoid 2 superfluous private message notification queries

When viewing the single view of the thread, the potential notifications about it are marked as read into the messages_action_conversation() function. We don't need to do it again when looping into the thread messages.

This was revealed by a database error which was appearing when a user who had received more than 1 message from two different threads was directly viewing the single view of one of the threads.

Props mike80222, oztaser

Fixes #8676 (10.0 branch)