Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 3 years ago

#5193 closed enhancement (fixed)

More hooks, more power! (in the messages component)

Reported by: slaFFik Owned by: slaFFik
Milestone: 2.8 Priority: normal
Severity: normal Version:
Component: Messages Keywords:
Cc:

Description

Hello,

Faced with a problem - can't intrude into the way messages work (like BP_Messages_Thread class).

Could you please add more hooks "here and there"? Filtering sql array, results etc. In ALL methods of that class, as there is currently none at all.

I would be really thankful :)

Attachments (9)

bp-messages-classes.php.patch (17.3 KB) - added by slaFFik 5 years ago.
bp-messages-classes.php.02.patch (17.3 KB) - added by slaFFik 5 years ago.
5193.01 (28.4 KB) - added by slaFFik 5 years ago.
1 cumulative patch, please ignore all other non-5193 patches
5193.01.patch (28.4 KB) - added by slaFFik 5 years ago.
1 cumulative patch, please ignore all other non-5193 patches (oops, forgot file extension)
5193.02.patch (30.9 KB) - added by slaFFik 5 years ago.
Some additions
5193.03.patch (31.8 KB) - added by slaFFik 5 years ago.
Fixing conflicts with trunk
5193.04.patch (31.8 KB) - added by slaFFik 5 years ago.
Last thing from the trunk that was left out of the board
5193.patch (29.3 KB) - added by boonebgorges 5 years ago.
5193.05.patch (926 bytes) - added by netweb 3 years ago.

Download all attachments as: .zip

Change History (41)

#1 @boonebgorges
6 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Summary changed from More hooks, more power! to More hooks, more power! (in the messages component)

Sure, it'd be great to have more hooks. If we had a patch, it'd happen faster :)

#2 @slaFFik
5 years ago

Working on this slowly.

#3 @slaFFik
5 years ago

  • Keywords has-patch added

Decided to attach patches in a smaller chunks, as it becomes harder to manage all that.

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


5 years ago

@slaFFik
5 years ago

1 cumulative patch, please ignore all other non-5193 patches

#5 @slaFFik
5 years ago

  • Milestone changed from Future Release to 2.2

@slaFFik
5 years ago

1 cumulative patch, please ignore all other non-5193 patches (oops, forgot file extension)

#6 @slaFFik
5 years ago

  • Keywords dev-feedback added

There are some places like this:

$this->sender_id = apply_filters( 'messages_message_sender_id_before_save', $this->sender_id, $this->id );
$this->thread_id = apply_filters( 'messages_message_thread_id_before_save', $this->thread_id, $this->id );
$this->subject   = apply_filters( 'messages_message_subject_before_save',   $this->subject,   $this->id );
$this->message   = apply_filters( 'messages_message_content_before_save',   $this->message,   $this->id );
$this->date_sent = apply_filters( 'messages_message_date_sent_before_save', $this->date_sent, $this->id );

do_action_ref_array( 'messages_message_before_save', array( &$this ) );

which don't require the first part, with individual params filters, as everything is covered via do_action_ref_array and & sign. But as I understand, we need to leave this as is for backcompat, right?

@slaFFik
5 years ago

Some additions

@slaFFik
5 years ago

Fixing conflicts with trunk

#7 @boonebgorges
5 years ago

everything is covered via do_action_ref_array and & sign. But as I understand, we need to leave this as is for backcompat, right?

do_action_ref_array() is not required in this case. (It was a PHP4 thing.) In PHP5, objects are passed by reference by default. Thus: do_action( 'messages_message_before_save', $this ); Only use do_action_ref_array() if you need a parameter that is *not* the first one to be passed by reference.

#8 follow-up: @boonebgorges
5 years ago

Also, a small request. It's fine to leave all the hooks in one patch, but this also contains a lot of whitespace changes, and a couple functionality changes (like the reorganization of logic in get_total_threads_for_user()). Could we get separate patches for these latter things? Ideally, any refactoring would be accompanied by unit tests.

@slaFFik
5 years ago

Last thing from the trunk that was left out of the board

#9 in reply to: ↑ 8 @slaFFik
5 years ago

Replying to boonebgorges:

contains a lot of whitespace changes,

According to WordPress Coding Standards.

and a couple functionality changes (like the reorganization of logic in get_total_threads_for_user()).

Yeah, because of new ability to filter $box in all places.

Could we get separate patches for these latter things?

So you want to split whitespaces/phpdoc changes and that function refactoring from the one patch? So there should be like 3 patches:

  1. whitespaces/phpdoc
  2. function refactoring
  3. actions/filters

Am I right?

Ideally, any refactoring would be accompanied by unit tests.

That will be a pain :(

#10 @boonebgorges
5 years ago

I definitely have no objection to coding standards, but having lots of those changes mixed up with new hooks in a single patch makes it harder to review. Whether you provide separate patches or not, I will *commit* it separately, to keep the changelog clean. If you provide separate patches, it will mean less work for me, which means it's more likely to be committed sooner :)

That will be a pain :(

It's a bigger pain to deal with regressions!

If it's a ton of work to split this up into different patches, that's fine, but it will just take a bit longer to review and commit, because like I said above, I'm going to have to comb through it line by line. Thanks for understanding :)

#11 @slaFFik
5 years ago

Crafty Boone :)


It's a bigger pain to deal with regressions!

I understand. At least I will try to dive into BP_UnitTestCase and factory. Wrote unit-tests last time like 3 years ago, need to refresh the knowledge.

If it's a ton of work to split this up into different patches, that's fine

Not ton, but I believe that splitting will take more time than you will spend on checking all that.

#12 @boonebgorges
5 years ago

Not ton, but I believe that splitting will take more time than you will spend on checking all that.

Well, I think this is in part because you're not using your tools correctly. git add -p is the most important powerful tool in a developer's arsenal. (I really need to write a blog post about this.) That being said, I will make a first pass at picking out the bits that can be committed, and will repost a patch of what's left.

#13 @boonebgorges
5 years ago

In 9164:

Improve some docblocks in bp-messages-classes.php.

Props slaFFik.
See #5193.

#14 @boonebgorges
5 years ago

In 9165:

Add some actions and filters to BP_Messages_Thread.

Props slaFFik.
See #5193.

@boonebgorges
5 years ago

#15 @boonebgorges
5 years ago

Hi slaFFik, it's me again :) As you can see above, I started working through your patch. There is some good stuff in there, but I'm going to take a break and pass the ball back to you, with a few comments:

  • For the time being, I'm going to skip all of the suggested changes that involve changing the internal logic of a method, until there's unit test coverage. If you write the unit tests, it may get done faster ;), but if not, I may write them myself. (Some of them already have tests, I think.)
  • You'll see in [9165] that I added documentation for the hooks. Gotta have this documentation to put them in.
  • In [9165], you'll see I opted not to use your suggested hooks in populate(), and replaced them with a single post_populate hook. The object gets passed by reference, so someone hooking there can do whatever they want to it. Let's try to do a similar consolidation in other areas - I would rather add a single hook that allows for many different kinds of customization than a half-dozen that are super-specific.

5193.patch contains all the parts of your last patch that have not yet been commited.

#16 @boonebgorges
5 years ago

In 9166:

Fix spacing issue introduced in [9165].

Props tw2113.
See #5193.

#17 @slaFFik
5 years ago

  • Keywords dev-feedback removed

you're not using your tools correctly

Yeah, perhaps. But I'm using svn and patches are svn-styled, not sure how git add -p will help me here :)

I really need to write a blog post about this.

Do, please!

Gotta have this documentation to put them in.

Will note that for myself for future reference.

Let's try to do a similar consolidation in other areas

Agree with you, and ...

5193 contains all the parts of your last patch that have not yet been commited.

... I will adopt this patch according to everything written above.

#18 @r-a-y
5 years ago

In 9185:

Introduce 'bp_messages_thread_before_mark_delete' action.

This commit renames the 'bp_messages_thread_before_delete' action added in
r9165 to 'bp_messages_thread_before_mark_delete' and moves the
'bp_messages_thread_before_delete' hook where the message thread is
actually deleted.

The phpDoc for BP_Messages_Thread::delete() is also updated to accurately
outline what is happening in the class method.

See #5193.

#19 @DJPaul
5 years ago

  • Keywords has-patch removed

Is this ticket complete now?

#20 @slaFFik
5 years ago

Not yet. Need to take Boone's 5193.patch (which contains what's left from my original patch) and make some changes, and resubmit it again.

#21 @DJPaul
5 years ago

  • Keywords needs-patch added
  • Milestone changed from 2.2 to Future Release

#22 @slaFFik
3 years ago

  • Milestone changed from Future Release to 2.8
  • Owner set to slaFFik
  • Status changed from new to assigned

After reviewing patches, I see that not so many changes that are worth to be added are left.
I will process and close this ticket for 2.8.

#23 @slaffik
3 years ago

In 11293:

Messages: New filters in BP_Messages_Notice class.

Provide several new filters and actions for different methods, that give ability for plugin developers to further fine-tune Notices in Messages component.

See #5193.

#24 @slaffik
3 years ago

In 11306:

Messages: new filter for get_recipients_ids()

Give ability to filter the array of recipients IDs. For BC we can't explicitly make it always return array, so if no usernames provided, false will be returned.

See #5193.

#25 @slaffik
3 years ago

In 11307:

Messages: improved code formatting for BP_Messages_Message.

Spaces, new lines etc, according to WordPress standards.

See #5193.

#26 @slaffik
3 years ago

In 11308:

Messages: improved code comments for bp_messages_delete_meta()

Spaces, new lines etc, according to WordPress standards. Always define a retval variable as false.

See #5193.

#27 @slaffik
3 years ago

In 11309:

Messages: improved code formatting for messages_new_message()

Spaces, new lines etc, according to WordPress standards.

See #5193.

#28 @slaffik
3 years ago

In 11310:

Messages: improvements to thread mark as read/unread.

Added new actions, so developers can now do their jobs when thread was marked as read/unread. Made wrappers like messages_mark_thread_read() meaningful by actually providing a return value.

See #5193.

#29 @slaFFik
3 years ago

  • Cc slaffik@… removed
  • Keywords needs-patch removed
  • Resolution set to fixed
  • Status changed from assigned to closed

I reviewed the leftover of 5193.patch, and committed some of its changes.
I decided not to commit things that I don't have a clear understand of how they can be used.
Right now I think that we can close this tickets as fixed, as lots of improvements were introduced.

If any new things are required, lets open a separate ticket for them for easier tracking.

Thanks Paul and Boone for participating in this ticket journey!

@netweb
3 years ago

#30 @netweb
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening with 5193.05.patch for a couple of coding standards fixes that were reverted in r11308

See https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#docblock-formatting : ... ensure that items in each tag group are aligned according to the examples.

The examples in the coding standards don't really highlight the alignment spacing that well, r11311 is a better example

#31 @slaFFik
3 years ago

@netweb

Makes sense, but we are totally inconsistent with this in BuddyPress core as far as I can see. Some writes with that extra intendation to align descriptions, others - without it.

Anyway, I'm not saying that we should ignore it, lets reuse that from Handbook, although it's much more handy to not make manually those intendation :)

#32 @slaffik
3 years ago

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

In 11375:

Messages: more hooks and filters.

Couple of coding standards fixes that were reverted in r11308.

Props netweb.
Fixes #5193.

Note: See TracTickets for help on using tickets.