Improve $args included in `bp_current_user_can` and `bp_user_can` filters.

[dcavins]

in bp_current_user_can() and bp_user_can(), the originally passed $args array gets overwritten for use with WordPress's functions like this:

$args = array( $blog_id, $capability, $args );
$retval = call_user_func_array( 'current_user_can_for_blog', $args );

Then, when they're included in the final filter, this compounded version is used. So, in bp_current_user_can()`, the filter parameters are actually:
apply_filters( 'bp_current_user_can', $retval, $capability, $blog_id, array( $blog_id, $capability, $args ) )

Similarly, in bp_user_can(), the filter parameters are actually:
apply_filters( 'bp_user_can', $retval, $user_id, $capability, $site_id, array( $user_id, $capability, $args ) )

I'd like pass the original $args array back out for easy manipulation by plugins. (And the other elements in the compounded array are duplicated in the filter parameters, so don't add value.)

My patch removes the modification of the $args variable in the course of each function, so that the original $args variables are passed out as they were passed in.

[dcavins]

Avoid changing the $args variable.

[boonebgorges]

This change seems fine to me, though it should get some documentation in the filter docblock, since it's technically a compatibility break.

[r-a-y]

Good catch, dcavins!

Perhaps, we should think about using the new bp_user_can() function in bp_current_user_can(). 02.patch is an attempt at merging this functionality. In an ideal world, we would drop support for the 'bp_current_user_can' filter in place of the 'bp_user_can' filter instead.

The XProfile cap changes are related to #6730.

[dcavins]

@r-a-y I'm totally in favor of user bp_current_user_can() as a wrapper/helper for bp_user_can(). Your patch looks good to me, and works as expected.

[DJPaul]

Patch missed removing $blog_id var on line 266 (as i look at it on trac with its line number).

We also need to catch if the user isn't logged in. current_user_can_for_blog took care of this but we aren't using it any more. (line 280 of the patch in the trac view, again). I think an 0 might cause problems in WP current_user_can (it seems like relying on that would be undefined behaviour).

[dcavins]

Replying to DJPaul:

Patch missed removing $blog_id var on line 266 (as i look at it on trac with its line number).

@DJPaul I was reading that as backcompat. If they pass an $args['blog_id'], use it for the $args['site_id'] but unset it. Maybe we should throw a deprecation notice there in 2.8?

We also need to catch if the user isn't logged in. current_user_can_for_blog took care of this but we aren't using it any more. (line 280 of the patch in the trac view, again). I think an 0 might cause problems in WP current_user_can (it seems like relying on that would be undefined behaviour).

Are you talking about passing a user_id of 0 to user_can()? Passing a 0 for user_id should result in the same behavior as calling current_user_can() when no one is logged in:
user_can(): ​https://core.trac.wordpress.org/browser/tags/4.5.3/src/wp-includes/capabilities.php#L505
current_user_can(): ​https://core.trac.wordpress.org/browser/tags/4.5.3/src/wp-includes/capabilities.php#L427

Huh, why is there no user_can() analog for current_user_can_for_blog(). I was wondering why we're doing multisite work in bp_user_can(). Interesting.

@r-a-y: Let's work this out and get it committed to trunk. I'll attach a version of your patch that adds info to the filter docblock per @boonebgorges' suggestion.

[dcavins]

Add note about changed filter arg.

[r-a-y]

Let's get this in! Thanks @dcavins!

[dcavins]

In 11052:

Improve bp_current_user_can().

• Deprecate $args['blog_id’] in favor of $args['site_id’].
• Improve the structure of the passed $args array in the

bp_current_user_can filter.

Props r-a-y, dcavins.

Fixes #7192.

[DJPaul]

I am guessing this change is the cause for our test_bp_current_user_can_should_interpret_integer_second_param_as_a_blog_id to fail -- it works on normal WordPress, but fails on multisite.

​https://travis-ci.org/buddypress/BuddyPress/jobs/156703464#L509

[dcavins]

Fix backcompat for old-style arguments.

[dcavins]

I'll commit this fix once I'm back at my SVN repo (unless anyone has any comments about it before then). Thanks for pointing out the Travis failure!

[dcavins]

In 11058:

bp_current_user_can(): Fix backward compatibility.

The changes to bp_current_user_can() in r11052 broke backward
compatibility for passing the site id as the $arg instead of an
array. This commit restores the old behavior.

See #7192.

[boonebgorges]

[11052] broke the ability to access wp-admin/edit.php?post_type=bp-email - the bp_current_user_can( 'bp_moderate' ) check during post type registration is returning false. MS, network-activated.

[r-a-y]

get_current_user_id.patch fixes the bp_current_user_can( 'bp_moderate' ) issue.

Problem was due to using bp_loggedin_user_id() instead of get_current_user_id(). If we use bp_current_user_can() earlier than when bp_loggedin_user_id() is initialized, the user ID would be 0, which is wrong.

[dcavins]

@r-a-y Thanks for chasing that problem down. I was unaware of the limitations of bp_loggedin_user_id().

@boonebgorges Does Ray's patch solve the issue on your system?

[boonebgorges]

Yup, patch fixes the issue for me.

Are there any future implications we should be thinking about, knowing that we can't use our filtered bp_loggedin_user_id() in the context of cap checks? I see that this has likely always been an issue (hidden behind current_user_can_for_blog()) but it might be something to flag for future research.

[r-a-y]

$bp->loggedin_user->id is initialized on 'bp_setup_globals', which is quite late in the action firing sequence:
​https://buddypress.trac.wordpress.org/browser/tags/2.6.2/src/bp-core/classes/class-bp-core.php#L215

We should probably consider initializing $bp->loggedin_user->ID earlier, but for 2.7, let's just leave this alone.

I've refreshed my get_current_user_id.patch to add the 'bp_loggedin_user_id' filter.

[r-a-y]

In 11107:

Caps: In bp_current_user_can(), use get_current_user_id() instead of bp_loggedin_user_id().

We're switching to get_current_user_id() because bp_loggedin_user_id()
is initialized on the 'bp_setup_globals' hook, which fires too late in
certain instances. We also apply the 'bp_loggedin_user_id' filter for
devs that are using it.

Fixes #7192.