#7506 closed defect (bug) (fixed)
friends_add_friend() should convert accepted arguments
Reported by: | slaFFik | Owned by: | r-a-y |
---|---|---|---|
Milestone: | 2.9 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Friends | Keywords: | has-patch |
Cc: | djpaul |
Description
Currently when in code you do something like this:
friends_add_friend( $user_one, $user_two, true );
where $user_one
and $user_two
are like this: string(1)"1"
- the success email won't be sent to $initiator_id
.
Reason: a kind-of-broken logic in BP_Email_Recipient::_construct()
, where we check $email_or_user
. Because of this token for recipient.email
is not generated and bp_email_set_default_headers()
generates notices.
So there should be several improvements:
friends_add_friend()
shouldintval()
its arguments.BP_Email_Recipient::_construct()
should be aware (or degrade gracefully) of a user id coming as a string, not an integer. Perhaps,is_numeric()
instead ofis_int()
check?bp_email_set_default_headers()
should have$user
check to prevent generating those notices in case user ID is a string passed tobp_send_email()
, because$user->ID
doesn't exist.
Attachments (1)
Change History (9)
This ticket was mentioned in Slack in #buddypress by hnla. View the logs.
8 years ago
#3
@
8 years ago
When a string is provided, my vote goes to using is_numeric()
over degrading gracefully. I've seen loose comparison used a lot in plugins so "1" and 1 will mean the same thing to many.
This ticket was mentioned in Slack in #buddypress by henrywright. View the logs.
8 years ago
#5
@
8 years ago
- Keywords has-patch added
In #6969, Paul wanted to typecast numeric strings into integers, so that's what 01.patch
does.
I'll leave the is_numeric()
change for a lead dev to comment on.
#6
@
8 years ago
- Keywords dev-feedback removed
7506.01.patch seems good to me.
Using is_numeric()
when handling $email_or_user
seems problematic to me, because it's possible to have numeric user logins in WordPress. This is going to be a problem whenever we do "fuzzy" lookups like this. A better practice would probably be for BP_Email_Recipient
to have something like a set_user( $field, $value )
, which would use get_user_by()
internally, and then for BuddyPress to use this exclusively rather than relying on the loosey-goosey checks (which we probably have to leave in for BC, but whose use could be discouraged through best practices in our own codebase). Not sure if this matches Paul's vision for how the email API works, so I'd let him weigh in on this idea, and keep the fix more localized and conservative for the time being.
bp_email_set_default_headers() should have $user check to prevent generating those notices in case user ID is a string passed to bp_send_email(), because $user->ID doesn't exist.
Yes, because it's possible to send emails to non-members, right? The unsubscribe link should only be added to the email footer if the get_user_by()
check succeeds.
I wanted to use
is_numeric()
as well inBP_Email_Recipient
. See https://buddypress.trac.wordpress.org/ticket/6969#comment:2.Related: #6977.