#7298 closed defect (bug) (fixed)
xprofile fied user visibility settings regression
Reported by: | hnla | Owned by: | djpaul |
---|---|---|---|
Milestone: | 2.7.1 | Priority: | high |
Severity: | major | Version: | |
Component: | Extended Profile | Keywords: | has-patch 2nd-opinion |
Cc: |
Description (last modified by )
Previously if custom xprofile field created and user control allowed for them to be changed for visibility i.e 'me', 'my freinds', all members' etc if these fields were added to the base group on the registration page the new user could opt to change the selection visibility of a field.
In 2.7 we have lost this ability with only a default visibility message shown.
Tested by reverting to a 2.6 tag where it worked as expected.
(Reported in https://buddypress.org/support/topic/problems-with-xprofile-field-visibilty-on-2-7-0/)
Attachments (1)
Change History (20)
#2
@
8 years ago
- Milestone changed from Awaiting Review to 2.7.1
Confirmed. Introduced by r11052.
To clarify, this report relates to the "Change" link after the "This field can seen by: Everyone" -- it doesn't show.
#4
@
8 years ago
Two issues. Our code relied on something in WordPress that I don't think should have worked, so here's a WordPress patch just for reference and reporting upstream if someone wants to do that: https://gist.github.com/paulgibbs/1f0c9b60374c45c4f0f1ed27bcd9f952
wp_get_current_user
returns a blank WP_User
object when you are not logged in, but the empty
check here inadequate to follow the intention of this code block (to bail out early). As a consequence, a later call to WP_User->has_cap
was being made, which invokes map_meta_cap
which calls the filter of the same name, eventually letting our bp_xprofile_map_meta_caps
, which allows anonymous users to have the bp_xprofile_change_field_visibility
capability, which is directly related to the bug @hnla found.
We changed how bp_current_user_can
and friends work in 2.7, which removed the call to current_user_can_for_blog
(which is where that WP patch applies).
I am still looking how best to fix this in BuddyPress.
#5
@
8 years ago
My thoughts on different ways to address this:
1) Revert the bp_current_user_can
change and rely on the "broken" behaviour with current_user_can_for_blog
... which will last until if/when WordPress fixes that function. Just punting the problem down the calendar.
2) Add special edge-case handling to bp_user_can
specifically for bp_xprofile_change_field_visibility
. Probably calling it directly. This is a bit of a poor long term solution but would fix the problem for now.
3) Do something like 2, but accommodate anonymous user capabilities properly. I don't know if bbPress has anything smart for this, I know it uses capabilities for logged-out users a lot more than WordPress does (WordPress doesn't, really; there is that odd read
capability for a few things).
4) Decide that the profile field visibility on the registration form isn't something we want to support, and call this a feature.
#6
@
8 years ago
1) Sounds viable if we knew or know when / if WP would attend to some solution, if they intend to then roll with this option ( does this have other ramifications for us though in the interim? ).
4) Sort of discount this as the 'feature' is out there and has been for a while and the sudden lack of noticed - however in many ways think maybe it is slightly odd to have on a reg form, more a feature to be accessed once registered and logged in, but then again I can see cases where it's nice to allow users this ability on registration so no to four :)
3) Is probably the route to follow however if we're to fix as a 2.7.1 is this not too long winded to pull together in a shortish space of time?
#7
@
8 years ago
(Reported in https://buddypress.org/support/topic/problems-with-xprofile-field-visibilty-on-2-7-0/)
Copying this to the opening comment ~hnla
#9
@
8 years ago
- Keywords has-patch 2nd-opinion added; needs-patch removed
Thanks for the research on this so far.
I agree that something like 2 or 3 is probably the best route for the time being. 7298.diff is a somewhat less horrendous technique than 2 (a special case hardcoded into bp_user_can()
) while avoiding a huge amount of infrastructure to handle logged-out user caps. At the moment, the cases where we need to add caps for logged-out users are quite limited, and using a separate callback function for each makes it very explicit.
It seems to me that WP will not easily be able to eliminate the inconsistency noted by DJPaul, for back compat reasons. BP demonstrates why cap checks for user 0 need to go through the same capability mapping filters as other cap checks.
A side note: if we go with something like 7298.diff, we might want to consider adding a dynamic filter bp_user_can_{$capability}
. This will make the callbacks cleaner, and will reduce greatly reduce overhead ('bp_user_can'
can be called hundreds of times on a pageload).
#10
@
8 years ago
@boonebgorges I don't really agree that a quick function, not doing any significant work, even being called hundreds of time, is not by itself any significant overheard. I also don't feel I can offer much further technical wisdom regarding best ways to fix this; my best effort in this area was #7176 -- which I was hoping would start moving that ball rolling more than it has -- so more than happy to go with whatever you think a good fix is.
#11
follow-up:
↓ 12
@
8 years ago
I don't really agree that a quick function, not doing any significant work, even being called hundreds of time, is not by itself any significant overheard.
@DJPaul Calling a function vs not calling it does add overhead, though whether it's "significant" is, I guess, subject to debate. https://gist.github.com/boonebgorges/a4f40af56d3e9263575daee5cd6d4c3c
I also don't feel I can offer much further technical wisdom regarding best ways to fix this; my best effort in this area was #7176 -- which I was hoping would start moving that ball rolling more than it has -- so more than happy to go with whatever you think a good fix is.
Like you, I spent many hours looking at #7176, so please don't think I'm ignoring the efforts there. As far as I can see, that ticket wouldn't directly address this one anyway - the assumption there is that we'll use 'map_meta_cap' to implement BP caps, while the inconsistent application of the 'map_meta_cap' filter for non-logged-in users is the source of the difficulty here.
If others have strong feelings about a general strategy for anonymous user caps, please offer them up. Otherwise I guess I'll go with something like 7298.diff for 2.7.1.
#12
in reply to:
↑ 11
@
8 years ago
Replying to boonebgorges:
If others have strong feelings about a general strategy for anonymous user caps, please offer them up. Otherwise I guess I'll go with something like 7298.diff for 2.7.1.
I'm sorry that I caused this mess with the changes to the _can()
functions.
Should the check only return true
when bp_is_register_page()
is true? There are other bits in place to prevent anonymous users from visiting profile edit screens, so maybe it's not important. (But when I see users with id 0 can do something, my impulse is to be overly specific about when it's allowed.)
In addition:
If admin creates a field sets that to e.g 'Me Only' but does NOT enforce visibility i.e lets user change on registration and login that field does not respect the initial default setting but displays field to all general members.
These issues are introduced with 2.7, having progressed through the tags checking each.