Skip to:
Content

BuddyPress.org

Opened 9 years ago

Closed 4 years ago

Last modified 7 days ago

#3797 closed enhancement (maybelater)

Refactor database methods to accept array-style arguments

Reported by: boonebgorges Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Core Keywords: needs-patch
Cc:

Description

Right now, most of our database methods take a long list of arguments, eg
function get( $max = false, $page = 1, $per_page = 25, $sort = 'DESC', $search_terms = false, $filter = false, $display_comments = false, $show_hidden = false, $exclude = false, $in = false, $spam = 'ham_only' ) {

This is hard to read, and makes developing and troubleshooting more troublesome than it ought to be. Accordingly, where possible, these methods should be refactored so that if an array is passed as the first argument, the rest of the arguments are ignored and we parse the passed array with a set of defaults using wp_parse_args().

The same is true of some of the template class constructors (BP_Activity_Template::construct(), etc).

We probably should *not* formally deprecate the old way of passing arguments, as WP's _deprecated_argument() will essentially break any plugin that makes direct calls to these classes.

Patches - even partial patches! - are welcome. Easy props!

Attachments (4)

3797.01.patch (3.6 KB) - added by boonebgorges 8 years ago.
3797.02.patch (9.9 KB) - added by boonebgorges 8 years ago.
3797.core-members-template.diff (5.1 KB) - added by Mamaduka 21 months ago.
3797.core-members-template.2.diff (8.3 KB) - added by Mamaduka 21 months ago.

Download all attachments as: .zip

Change History (23)

#1 @DJPaul
9 years ago

_deprecated_argument() is what would be used here, I don't understand why it would break backwards compatibility. We're just using it to flag that an argument has been replaced.

Last edited 9 years ago by DJPaul (previous) (diff)

#2 @boonebgorges
9 years ago

You're right. I was just looking at some places in WP where they used _deprecated_argument() *and* broke backward compatibility. My misunderstanding.

So, my thought is something like this:

if ( count( func_get_args() ) > 1 ) {
	foreach( func_get_args() as $arg_key => $arg ) {
		if ( 0 != $arg_key ) {
			_deprecated_argument( $arg, __( 'Arguments should be passed as an array.', 'buddypress' ), '1.6' );
		}
	}
} else {
	$args = func_get_arg( 0 );
	$defaults = array( ... );
	$r = wp_parse_args( $args, defaults );
	extract( $r );
}

Seem right?

#3 @DJPaul
9 years ago

Something like that. Be nice to check for a deprecated var, and if found, turn all the individual arguments into a single array, then function carries on.

#4 @boonebgorges
9 years ago

Yeah, I guess that makes sense, actually - if we add more defaults to the array in the future, my method will break older plugins. So something like:

if ( count( func_get_args() ) > 1 ) {
	foreach( func_get_args() as $arg_key => $arg ) {
		if ( 0 != $arg_key ) {
			_deprecated_argument( $arg, __( 'Arguments should be passed as an array.', 'buddypress' ), '1.6' );
		}
	}
	
	$args = array(
		'type' => func_get_arg(0),
		'per_page' => func_get_arg(1),
		// etc
	);
} else {
	$args = func_get_arg( 0 );
}

$defaults = array( ... );
$r = wp_parse_args( $args, defaults );
extract( $r );

I'm using func_get_arg() because I assume that we will change the parameter names in the function definition to $deprecated1, $deprecated2 (or remove them altogether) so that devs aren't encouraged to use them when they look at the source.

#5 @boonebgorges
8 years ago

  • Keywords dev-feedback added

Not having this in place is causing an awful nightmare with fixing some other bugs, so I'd like to go ahead with it for this release (at least with the major database functions).

I have attached a patch 3797.01.patch that shows my method for BP_Activity_Activity::construct(). I'd like to get feedback on my method before applying it to other methods. Thanks.

#6 @boonebgorges
8 years ago

  • Keywords has-patch added; needs-patch removed

Based on a bit of feedback from DJPaul, here is a shot at a refactor, this time expanded for the entire bp_has_activities() stack (BP_Activity_Template::__construct(), BP_Activity_Activity::get(), and all the places where these methods are referenced, except for the deprecated BP_Activity_Activity::get_specific())

Note that I have abstracted out the utility function bp_core_parse_args_array(). Writing documentation for this function that makes sense was the hardest part of writing this patch :)

Feedback/tests welcome. If all are happy with this approach, I'll put these particular changes in right now, and we can roll the changes across BP more generally either now or in the next dev cycle. Thanks.

#7 @boonebgorges
8 years ago

  • Milestone changed from Future Release to 1.6

#8 @r-a-y
8 years ago

Gigantic +1!

I just tested 02.patch lightly by using the old constructor arguments and it works well.

I only have one small tidbit for the PHP notice.

The full PHP notice currently reads like this:

PHP Notice: BP_Activity_Template::__construct was called with an argument that is <strong>deprecated</strong> since version 1.6! Arguments passed to BP_Activity_Template::__construct should be in an associative array. See the inline documentation for more details. in \PATH\wp-includes\functions.php on line 3551

The wp-includes/functions.php part can throw some people off.

Perhaps amend line 115 in 02.patch from:

Arguments passed to %s should be in an associative array. See the inline documentation for more details.

to:

Arguments passed to %s should be in an associative array. View the inline documentation in /bp-activity/bp-activity-classes.php for more details.

Other than that, great job, Boone!

#9 @boonebgorges
8 years ago

(In [5993]) Refactors bp_has_activities() stack to support the passing of arguments as associative arrays.

  • Maintains backward compatibility by parsing old-style parameters using bp_core_parse_args_array(), converting to new-style array, and throwing _deprecated_argument() error
  • Changes calls to BP_Activity_Template::construct() and BP_Activity_Activity::get() to use the new format throughout BuddyPress
  • See #3797

#10 @boonebgorges
8 years ago

  • Keywords needs-patch added; dev-feedback has-patch removed
  • Milestone changed from 1.6 to Future Release

Thanks for the sanity check, r-a-y!

What you see in r5993 can be used as a template for future refactoring. I'll do it myself on an as-needed basis. Anyone who wants to chip in and grab a couple of methods - patches welcome :) For now, bumping to Future Release, since this was all I needed for 1.6.

#11 @boonebgorges
8 years ago

(In [5994]) More consistent _deprecated_argument() warning in BP_Activity_Template::construct(). See #3797

#12 @boonebgorges
8 years ago

(In [6321]) Converts bp_has_groups() stack to accept parameters as an array.

Arguments passed in the old style are converted to an array automatically, for
backward compatibility.

See #3797

#13 @boonebgorges
6 years ago

In 7930:

Refactor bp_group_has_members() stack to use array-style parameters

See #3797, #921

#14 @r-a-y
6 years ago

Commit typo: Meant to reference this ticket in r9301.

#15 @DJPaul
4 years ago

@boonebgorges @r-a-y Can we close this ticket?

#16 @boonebgorges
4 years ago

  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from new to closed

I think there's still a handful of places where this could be done, but they're clearly not holding up the project, so let's handle them on an as-needed basis.

#17 @Mamaduka
21 months ago

Added patch to refactor BP_Core_Members_Template::construct() to use array-style arguments.

P.S. Not really sure, if I need to update milestone or status for this ticket.

#18 @Mamaduka
21 months ago

The updated patch uses new array style arguments in templates and tests.

#19 @boonebgorges
7 days ago

In 12692:

Convert BP_Core_Members_Template to accept array of arguments.

This changeset also eliminates the use of extract() in
BP_Core_Members_Template::__construct().

Props Mamaduka.

See #3797, #7999.

Note: See TracTickets for help on using tickets.