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

[slaFFik]

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 :)

[boonebgorges]

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

[slaFFik]

Working on this slowly.

[slaFFik]

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

[slaFFik]

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

[slaFFik]

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

[slaFFik]

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]

Some additions

[slaFFik]

Fixing conflicts with trunk

[boonebgorges]

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.

[boonebgorges]

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]

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

[slaFFik]

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 :(

[boonebgorges]

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 :)

[slaFFik]

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.

[boonebgorges]

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.

[boonebgorges]

In 9164:

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

Props slaFFik.
See #5193.

[boonebgorges]

In 9165:

Add some actions and filters to BP_Messages_Thread.

Props slaFFik.
See #5193.

[boonebgorges]

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.

[boonebgorges]

In 9166:

Fix spacing issue introduced in [9165].

Props tw2113.
See #5193.

[slaFFik]

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.

[r-a-y]

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.

[DJPaul]

Is this ticket complete now?

[slaFFik]

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.

[slaFFik]

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.

[slaffik]

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.

[slaffik]

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.

[slaffik]

In 11307:

Messages: improved code formatting for BP_Messages_Message.

Spaces, new lines etc, according to WordPress standards.

See #5193.

[slaffik]

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.

[slaffik]

In 11309:

Messages: improved code formatting for messages_new_message()

Spaces, new lines etc, according to WordPress standards.

See #5193.

[slaffik]

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.

[slaFFik]

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]

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

[slaFFik]

@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 :)

[slaffik]

In 11375:

Messages: more hooks and filters.

Couple of coding standards fixes that were reverted in r11308.

Props netweb.
Fixes #5193.