groups_create_group() improvements

[r-a-y]

While working on #5105, I needed to mass-create a bunch of groups programatically.

And, guess what? groups_create_group() isn't so great! :(

No validation is done whatsoever when using groups_create_group() by itself.

In the attached patch, I've added validation directly into BP_Groups_Group::save().

[r-a-y]

Updated patch to only use groups_check_slug() when creating a new group

[boonebgorges]

I guess I don't have a particular problem with these changes, but I think that we should be careful to attempt some sort of consistency across components. Generally speaking, we have three layers of functions for things like group creation:

1. template controllers (eg groups_action_create_group()), which call...
2. procedural functions (eg groups_create_group()), which call...
3. database save() methods (eg BP_Groups_Group::save())

There are lots of things that need to happen in terms of validation/sanitization when creating a group, and each of them should happen only in one of the places. Off the top of my head:

1. capabilities checks (should happen in (1), and for the most part we are good about this)
2. getting submitted data out of $_REQUEST (1)
3. sanitization that is directly related to $_REQUEST - stuff like urldecode() and stripslashes() (1)
4. sanitization against SQL injection (3)
5. sanitization against illegal markup/scripts (kses) (we generally do this in filters like groups_group_name_before_save, which happen in (3))
6. providing default/fallback values - stuff like the current time and the loggedin user ID
7. Checking to make sure the submitted data will create a valid group - this is stuff like ! empty( $name )
8. Checking to make sure that submitted data will properly be submitted into the table - stuff like making sure there is a value for NOT NULL columns in the db (should happen in (3))

[probably more?]

So, there are some things that we should be (and are) doing in (1). And there are some things that we should be (and are) doing in (3). The question for the remaining items is: should they happen in (2) or (3)?

I think the best principle is something like this: Do everything as late as possible without sacrificing the flexibility of the later function. For example, we do cap checks in (1) because doing them in (2) would prevent automated group creation (where ! current_user_can( 'bp_moderate' )). On the other hand, there's no reason why we should ever allow SQL injection, so we do that stuff in $wpdb->prepare() in (3).

r-a-y - I think that your patch 5214.01.patch is actually pretty good on these counts. The items you've moved to (3) seem like they are the kinds of things that should never be skirted. So, like I said, the patch looks fine :) I just want to impose some sanity on the way we make these changes in the future, across components, because it's currently somewhat of a mess. (Check out the differences between bp-activity and bp-groups on this count.) Maybe we could have a document that sketches out our best practices with respect to this stuff.

[r-a-y]

I love the detail in this post! We should definitely use it as a guide moving forward.

Our internal functions do need some auditing and is indeed in a mess (also see #5116).

As for this patch, if I can write some unit tests for this, I might be able to squeeze this into 1.9, but more likely this will be in 2.0 along with any other changes to clean up our functions.

[boonebgorges]

We should definitely use it as a guide moving forward.

Yes, it would be a good starting place!

As for this patch, if I can write some unit tests for this, I might be able to squeeze this into 1.9, but more likely this will be in 2.0 along with any other changes to clean up our functions.

Sounds prudent. Thanks!

[DJPaul]

Bumping to 2.0

[boonebgorges]

Had another look. This is mostly good. I'm going to make a few changes:

• We should not be bailing on if ( empty( $creator_id ) ). This is a change from our current behavior. It breaks dozens of our unit tests, which suggests it'll break real-life uses. In practice, a creator_id will always be set in the user interface because it falls back on bp_loggedin_user_id(); this, of course, will be 0 when groups are created programatically. That said, there is nothing particularly invalid about a group without a creator - it's not as if BP breaks in any way. So let's skip the check altogether.

• IMO, checking for a valid status is not appropriate for the DB class. It's not totally clear to me what the purpose of this whitelist is in general, but in any case, having an "invalid" status will not render a group unusable. So I'm going to move it up to groups_create_group().

[boonebgorges]

In 8157:

Improve validation and documentation for groups_create_group() and BP_Groups_Group::save()

Fixes #5214

Props r-a-y