#7192 closed defect (bug) (fixed)
Improve $args included in `bp_current_user_can` and `bp_user_can` filters.
Reported by: | dcavins | Owned by: | dcavins |
---|---|---|---|
Milestone: | 2.7 | Priority: | normal |
Severity: | normal | Version: | 1.6 |
Component: | Core | Keywords: | has-patch commit |
Cc: |
Description
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.
Attachments (5)
Change History (27)
#1
@
8 years ago
This change seems fine to me, though it should get some documentation in the filter docblock, since it's technically a compatibility break.
#2
@
8 years ago
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.
#3
@
8 years ago
@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.
#4
follow-up:
↓ 5
@
8 years ago
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).
#5
in reply to:
↑ 4
@
8 years ago
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 WPcurrent_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.
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
8 years ago
#11
@
8 years ago
- Owner set to dcavins
- Resolution set to fixed
- Status changed from new to closed
In 11052:
#12
@
8 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
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
#13
@
8 years ago
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!
#16
@
8 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
[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.
#17
@
8 years ago
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.
#18
@
8 years ago
@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?
#19
@
8 years ago
- Keywords dev-feedback removed
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.
#20
@
8 years ago
$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.
Avoid changing the $args variable.