Skip to:
Content

BuddyPress.org

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#6997 closed enhancement (fixed)

Add item ID to bp_get_member|group_class() filters

Reported by: Offereins Owned by: boonebgorges
Milestone: 2.6 Priority: normal
Severity: normal Version:
Component: Members Keywords:
Cc: lmoffereins@…

Description

For improved filtering of the bp_get_member_class() and bp_get_group_class() functions filters, send along the current item's ID.

Patch attached.

Attachments (10)

6997.patch (1.6 KB) - added by Offereins 4 years ago.
6997-bp-members-template.patch (4.7 KB) - added by Offereins 4 years ago.
Filter updates for bp-members-template.php
6997-bp-groups-template.patch (6.8 KB) - added by Offereins 4 years ago.
Filter updates for bp-groups-template.php
6997-bp-activity-template.patch (4.5 KB) - added by Offereins 4 years ago.
Filter updates for bp-activity-template.php
6997-bp-blogs-template.patch (10.0 KB) - added by Offereins 4 years ago.
Filter updates for bp-blogs-template.php
6997-bp-friends-template.patch (11.6 KB) - added by Offereins 4 years ago.
Filter updates for bp-friends-template.php
6997-bp-messages-template.patch (5.3 KB) - added by Offereins 4 years ago.
Filter updates for bp-messages-template.php
6997-bp-notifications-template.patch (2.0 KB) - added by Offereins 4 years ago.
Filter updates for bp-notifications-template.php
6997-bp-xprofile-template.patch (2.9 KB) - added by Offereins 4 years ago.
Filter updates for bp-xprofile-template.php
6997-bp-members-template2.patch (5.4 KB) - added by Offereins 4 years ago.
Filter updates for bp-members-template.php v2

Download all attachments as: .zip

Change History (32)

@Offereins
4 years ago

#1 @DJPaul
4 years ago

  • Milestone changed from Awaiting Review to 2.6

#2 @Offereins
4 years ago

Adding the item's ID might also apply to other template tag filters. Perhaps this could be done in a single sweep?

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


4 years ago

#4 @DJPaul
4 years ago

  • Keywords 2nd-opinion added

There's a fine line between adding global variables to filters, instead of those callbacks just accessing the global variable directly (or using an existing wrapper function).

#5 @dcavins
4 years ago

  • Keywords reporter-feedback added

Hi @Offereins-

Thanks for your patch. We were discussing this ticket this morning: https://wordpress.slack.com/archives/buddypress/p1459867704001333

and leaning toward not fixing it because the $members_template and $groups_template are global variables that can already be accessed in any function hooked to those filters.

What do you think?

#6 @Offereins
4 years ago

  • Keywords reporter-feedback removed

I noticed the talk. I'm not against it, though this was mainly coming from the view that touching globals is unfavorable, to say the least. There are indeed a lot more filters to change if we're going this route. But there is of course the family of bp_get_member_user_id() and the likes to read from the global template, so its fine with me.

Perhaps it is a (small) matter of education/inline docs on why/how the filters differ from the norm that we've come to know from WP and other filters.

#7 @boonebgorges
4 years ago

  • Keywords needs-patch added; has-patch removed

touching globals is unfavorable, to say the least.

I agree with this. I've generally advocated with *not* adding the values to the filters just to avoid code churn, but I could easily be persuaded otherwise.

As noted in the chat, I think, on balance, that it would be preferable to pass the member object (group object, etc) to the filter rather than the ID. But this introduces its own complications, such as the fact that, as far as I know, $members_template->member is a stdClass object and not something with proper structure. In any case, we should be consistent.

@Offereins If you want to take this up as a project, I'd encourage you to look through the bp-*-template.php files to get a sense of what the general strategy should be. For example, if there are lots of places where we're already passing IDs to filters in these sorts of functions, then maybe we should continue using IDs. Once we have broad decisions made, someone will need to begin generating patches, maybe one component at a time. These patches should include the necessary filter documentation changes (@since and @param).

@Offereins
4 years ago

Filter updates for bp-members-template.php

@Offereins
4 years ago

Filter updates for bp-groups-template.php

#8 @Offereins
4 years ago

@boonebgorges I'll refrain from adding filter parameters that are accessible through global template variables, to keep the single developer experience.

However, I just uploaded two patches for the members and groups template files. Added filter parameters are variables that are passed as function parameters or those that may have been touched within the scope of the filter's function. These are not accessible through globals or other template vars.

@Offereins
4 years ago

Filter updates for bp-activity-template.php

@Offereins
4 years ago

Filter updates for bp-blogs-template.php

@Offereins
4 years ago

Filter updates for bp-friends-template.php

@Offereins
4 years ago

Filter updates for bp-messages-template.php

@Offereins
4 years ago

Filter updates for bp-notifications-template.php

@Offereins
4 years ago

Filter updates for bp-xprofile-template.php

@Offereins
4 years ago

Filter updates for bp-members-template.php v2

#9 @boonebgorges
4 years ago

  • Owner set to boonebgorges
  • Status changed from new to reviewing

#10 @boonebgorges
4 years ago

In 10779:

Pass the $user_id param to the bp_get_last_activity filter.

Props Offereins.
See #6997.

#11 @boonebgorges
4 years ago

In 10780:

Pass the $group_id to the bp_get_profile_group_name filter.

Props Offereins.
See #6997.

#12 @boonebgorges
4 years ago

In 10781:

Pass $thread_id to a number of Messages template filters.

Props Offereins.
See #6997.

#13 @boonebgorges
4 years ago

In 10782:

Pass object IDs to a number of Friends template filters.

Props Offereins.
See #6997.

#14 @boonebgorges
4 years ago

In 10783:

Pass $user_id to the bp_get_total_blog_count_for_user filter.

Props Offereins.
See #6997.

#15 @boonebgorges
4 years ago

In 10784:

Pass $user_id to a number of Activity filter templates.

Props Offereins.
See #6997.

#16 @boonebgorges
4 years ago

In 10785:

Pass parsed function parameters to a number of template filters.

The template filters in question are ones that get their main context from
a template global (say, $members_template), but get auxiliary information
about how to build markup from the function params. As such, it's useful to
have these parameters available via filter.

Props Offereins.
See #6997.

#17 @boonebgorges
4 years ago

In 10786:

Add missing parameter documentation to bp_loggedin_user_id and bp_displayed_user_id filters.

Props Offereins.
See #6997.

#18 @boonebgorges
4 years ago

In 10787:

Correct parameter documentation for bp_get_activity_avatar filter.

Props Offereins.
See #6997.

#19 @boonebgorges
4 years ago

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

I broke these up into what seemed like meaningful chunks:

  • fixes to documentation
  • instances where $r or $args is being passed as a chunk to filters
  • instances where the object ID is being passed to a filter

I've left out the formatting mods. We follow WP's policy of not making these sorts of edit just to meet our own standards - it causes churn in the changelog that makes debugging and blaming more difficult.

Thanks for your patches, here - there are a couple places where I think that these new params will be really useful.

#20 @Offereins
4 years ago

  • Keywords needs-patch 2nd-opinion removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

@boonebgorges Thank you for taking this up and implementing it! Going through the commits, I noticed you just missed adding the actual $r parameter to the bp_get_member_profile_data filter - you only added the DocBlock.

Also thank you for explaining the code format policy. I just can't help myself scratching that itch when going through the code and suggesting improvements ;)

#21 @boonebgorges
4 years ago

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

In 10796:

Pass argument array to bp_get_member_profile_data filter.

Missed in [10785].

Props Offereins.
Fixes #6997.

#22 @boonebgorges
4 years ago

Going through the commits, I noticed you just missed adding the actual $r parameter to the bp_get_member_profile_data filter - you only added the DocBlock.

Oops! To be fair, you did the same thing in a previous patch in a couple of places :-P

Note: See TracTickets for help on using tickets.