Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6125 closed defect (bug) (fixed)

Small improvements to Member Types functions

Reported by: Offereins Owned by: boonebgorges
Milestone: 2.2 Priority: normal
Severity: normal Version:
Component: Members Keywords: has-patch
Cc:

Description

In BP 2.2-beta2, checking out the new Member Types API functionality, I encountered some bugs and redundant code:

  • Modification of the type args $r without storing it in the type object
  • Checking for type (array key) existence with empty() instead of isset()
  • Declaring a $field variable without using it

A patch is attached for bp-members/bp-members-functions.php.

Attachments (2)

bp-members-functions.patch (1.7 KB) - added by Offereins 5 years ago.
Patch for bp-members-functions.php
6125.02.patch (5.3 KB) - added by johnjamesjacoby 5 years ago.

Download all attachments as: .zip

Change History (11)

@Offereins
5 years ago

Patch for bp-members-functions.php

#1 @DJPaul
5 years ago

  • Milestone changed from Awaiting Review to 2.2

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


5 years ago

#3 @johnjamesjacoby
5 years ago

The $output parameter is only used in user_admin_load(), but not for the names specifically anyways. We could eliminate it completely to simplify the function and remove the hardcoded names bit entirely IMO.

Looking at this more makes me think the bp_get_member_types() function is doing too much compared to other similar utility functions. 6125.02.patch simplifies bp_get_member_types() and introduces another helper to filter those results if it ever becomes necessary.

#4 @boonebgorges
5 years ago

In 9360:

Parse arguments correctly in bp_register_member_type().

Custom labels were not being properly set due to a logic error in the
function.

Props Offereins.
See #6125.

#5 @boonebgorges
5 years ago

In 9361:

Fix incorrect variable name in docblock of bp_get_member_type_object().

Props johnjamesjacoby.
See #6125.

#6 follow-up: @boonebgorges
5 years ago

Thanks, Offereins and johnjamesjacoby.

Checking for type (array key) existence with empty() instead of isset()

This is copied right from get_post_type_object(). There should be no natural case in which a member type key is set to an empty value (like an empty string). That being said, there's no harm in making the change to isset().

Looking at this more makes me think the bp_get_member_types() function is doing too much compared to other similar utility functions. 6125.02.patch simplifies bp_get_member_types() and introduces another helper to filter those results if it ever becomes necessary.

One of the goals here was to make the API look exactly like WP's post type API. So bp_get_member_types() has a function signature identical to get_post_types(). Unless there's a powerful argument to the contrary (like a bug that it introduces), I'd prefer not to break the filter logic out into a separate function.

#7 @boonebgorges
5 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 9362:

Remove copypasta from bp_get_member_types().

Props Offereins.
Fixes #6125.

#8 in reply to: ↑ 6 @johnjamesjacoby
5 years ago

Replying to boonebgorges:

One of the goals here was to make the API look exactly like WP's post type API. So bp_get_member_types() has a function signature identical to get_post_types(). Unless there's a powerful argument to the contrary (like a bug that it introduces), I'd prefer not to break the filter logic out into a separate function.

I figured it was something like this. My only concern with it how it is now, is the hardcoded limitation we put on names vs. any other possible field that someone may want to pluck by, and the odd position it puts the filter in. As it sits now, we've whitelisted names for no reason, as each usage overrides it to objects, which makes it look accidental or unfinished.

#9 @boonebgorges
5 years ago

My only concern with it how it is now, is the hardcoded limitation we put on names vs. any other possible field that someone may want to pluck by, and the odd position it puts the filter in. As it sits now, we've whitelisted names for no reason, as each usage overrides it to objects, which makes it look accidental or unfinished.

Not for no reason: because this is how get_post_types() does it. When pulling up a list of member types, what you generally want is either (a) to have a flat list of the member types (ie names), or (b) to have all available information about them (ie objects). I just happened to use wp_list_pluck() to make it happen. IMHO, it would add little value to support any member type key (like, say, 'labels') when it's just as simple for devs to pull the objects and wp_list_pluck() whatever they want themselves. By limiting what's officially supported in terms of parameter values (and, importantly, by making the documentation more limited - 'names' and 'objects', period), we make it clearer what the purpose of the function is. That said, if it bothers you a lot, I don't have a huge problem with just passing 'output' to wp_list_pluck() and having a special case for 'objects'.

The filter comes before the pluck so that filtering functions don't need to worry about checking to see if the values passed to the filter are objects or strings. If you think about the various ways you might use this filter in real life, I think that this setup makes more sense.

Note: See TracTickets for help on using tickets.