Refactor database methods to accept array-style arguments

[boonebgorges]

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!

[DJPaul]

_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.

[boonebgorges]

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?

[DJPaul]

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.

[boonebgorges]

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.

[boonebgorges]

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.

[boonebgorges]

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.

[r-a-y]

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!

[boonebgorges]

(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

[boonebgorges]

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.

[boonebgorges]

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

[boonebgorges]

(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

[boonebgorges]

In 7930:

Refactor bp_group_has_members() stack to use array-style parameters

See #3797, #921

[r-a-y]

Commit typo: Meant to reference this ticket in r9301.

[DJPaul]

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

[boonebgorges]

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.

[Mamaduka]

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.

[Mamaduka]

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

[boonebgorges]

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.