Skip to:
Content

Opened 4 years ago

Closed 5 months ago

Last modified 5 months ago

#3083 closed enhancement (fixed)

Create message meta table and appropriate meta functions

Reported by: r-a-y Owned by: r-a-y
Milestone: 2.2 Priority: normal
Severity: normal Version: 1.5
Component: Component - Messaging Keywords: has-patch commit
Cc:

Description

Could be useful for attaching things to private messages.

If no time in 1.3, maybe for 1.4?

Attachments (2)

3083.01.patch (14.5 KB) - added by r-a-y 5 months ago.
3083.02.patch (21.8 KB) - added by r-a-y 5 months ago.

Download all attachments as: .zip

Change History (19)

comment:1 @cnorris234 years ago

I like the idea. The messaging component looks like a good candidate for CTPs which would add meta tables/functions automagically.

comment:2 @DJPaul4 years ago

  • Milestone changed from Awaiting Review to Future Release

comment:3 @boonebgorges12 months ago

  • Keywords needs-patch added
  • Severity set to normal

comment:4 @slaFFik5 months ago

Thinking on working on this task.

comment:5 @slackbot5 months ago

This ticket was mentioned in Slack in #buddypress by slaffik. View the logs.

comment:6 @slackbot5 months ago

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.

comment:7 @r-a-y5 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 2.2
  • Owner set to r-a-y
  • Status changed from new to assigned

First patch for message meta table is up.

Some notes:

  • Made changes to BP_Messages_Thread::populate() to accept a third parameter for extra arguments. For meta purposes, I added an update_meta_cache argument to mirror the group and activity template loops. Let me know what you think here.
  • Made changes to BP_Messages_Thread::delete(). Most notably to grab the $message_ids using $wpdb->get_col() since a message thread can contain more than one message. Previously, it was using $wpdb->get_var(), which is wrong.

In the patch, I've also included oEmbed caching for private messages so you can test the functionality, but I'll commit that separately for #5990.

Also, no unit tests yet, will add that in a subsequent patch.

Most of this was pretty straightforward due to boonebgorges's awesome work with migrating everything over to WP's metadata API.

@r-a-y5 months ago

comment:8 @boonebgorges5 months ago

r-a-y - Thanks for working on this! Looks pretty good.

  • There's some copypasta to be cleaned up - mostly in the docblocks, but see also the 'cache_key_prefix' in the meta prefetcher
  • The places where you've chosen to prefetch meta look good to me, as do the 'update_meta_cache' arguments.
  • You set 'update_meta_cache' to false in the inbox, with a note asking for feedback. My feedback: Don't :) I have no idea if anyone will actually use messagemeta in the inbox, but it's such a minor bit of overhead that I see no reason why we shouldn't prefetch it for every display loop.
  • I know you're going to work on unit tests, but I just wanted to add that the places where I'd most like to see them are surrounding the cache pre-fetch and the changes to delete()
  • On that note: passing an array of message_ids to 'messages_thread_deleted_thread' might break things. I know it's very unlikely that anyone is using this hook, but still, I think it's safest (and in keeping with the singular name of the hook) to loop through and fire it for each deleted message ID. You can also add a separate plural action if you want.

This will be pretty sweet!

comment:9 @r-a-y5 months ago

Thanks for the feedback, boonebgorges.

02.patch is refreshed for trunk and adds unit tests.

To reply to a few of your points:

You set 'update_meta_cache' to false in the inbox, with a note asking for feedback. My feedback: Don't :) I have no idea if anyone will actually use messagemeta in the inbox, but it's such a minor bit of overhead that I see no reason why we shouldn't prefetch it for every display loop.

If a BP install is not using an object cache, this adds an extra query to each thread in the inbox, for a maximum of 10 extra DB queries when on the inbox page.

I would suggest that we leave it false here. If a plugin really needs it, they can fetch the message meta then and it will be added to the cache.

passing an array of message_ids to 'messages_thread_deleted_thread' might break things.

I've done what you suggested here.

I also noticed that the 'messages_thread_deleted_thread' hook was removed in r9165, so I've added it back in, but I would be happy to remove it.

Also from r9165, I have renamed the 'bp_messages_thread_before_delete' hook to 'bp_messages_thread_before_mark_delete' and moved 'bp_messages_thread_before_delete' to when a message is really deleted. This is due to the way the BP_Messages_Thread::delete() method works. Let me know what you think.

@r-a-y5 months ago

comment:10 @boonebgorges5 months ago

  • Keywords commit added

If a BP install is not using an object cache, this adds an extra query to each thread in the inbox, for a maximum of 10 extra DB queries when on the inbox page.

D'oh, I'm a moron. I forgot that this was one level deep, and would do 10 queries rather than one. You are right. (If we decide it's needed in the future, we can write a prefetcher that does it all in a single query.)

I think this looks like a good first pass.

comment:11 @DJPaul5 months ago

Looks pretty decent, well done everyone

comment:12 @r-a-y5 months ago

In 9182:

Messages: Introduces meta table and related cache functions.

This commit alters the messages DB schema to add a meta table, which will
allow plugin developers to record information about a message.

The new message meta functions in bp-messages-functions.php mirrors those
in other BP components (activity, groups, etc.).

The DB version is also bumped to 9181.

See #3083.

comment:13 @r-a-y5 months ago

In 9183:

Prefetch message meta when in a message loop.

This reduces overhead when querying for message meta in the context of a
bp_thread_has_messages() loop.

Also introduces an 'update_meta_cache' parameter to the bp_has_thread_messages()
stack, so the message meta cache prefetch can be disabled by developers.

See #3083.

comment:14 @r-a-y5 months ago

In 9184:

Grab all message IDs before deleting a message thread.

In BP_Messages_Thread::delete(), we need to grab all message IDs before
a message thread is outright deleted. This is done by using
$wpdb->get_col() instead of $wpdb->get_var().

This commit also:

  • Deletes the message meta attached to the message (see #3083)
  • Reinstates the 'messages_thread_deleted_hook' action that was improperly removed in r9165
  • Adds $message_ids as an extra parameter to the 'bp_messages_thread_after_delete' action

comment:15 @r-a-y5 months ago

In 9186:

Add unit test cases for message meta.

See #3083.

comment:16 @r-a-y5 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

Marking this as fixed. Thanks for the feedback everyone!

comment:17 @r-a-y5 months ago

In 9195:

Do not fetch meta cache when getting a thread's total message count.

Also add defaults to the BP_Messages_Thread_Template constructor.

See #3083.

Note: See TracTickets for help on using tickets.