Skip to:
Content

Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#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:

  1. friends_add_friend() should intval() its arguments.
  2. 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 of is_int() check?
  3. 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.

Attachments (1)

7506.01.patch (655 bytes) - added by r-a-y 5 months ago.

Download all attachments as: .zip

Change History (9)

#1 @r-a-y
5 months ago

  • Keywords dev-feedback added

I wanted to use is_numeric() as well in BP_Email_Recipient. See https://buddypress.trac.wordpress.org/ticket/6969#comment:2.

Related: #6977.

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


5 months ago

#3 @henry.wright
5 months 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.


5 months ago

#5 @r-a-y
5 months 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.

@r-a-y
5 months ago

#6 @boonebgorges
5 months 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.

#7 @r-a-y
5 months ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from new to closed

In 11539:

Friends: Fix issue with friendship email not being sent when using friends_add_friend() in some instances.

This commit ensures that user IDs passed to friends_add_friend() are
typecast as integers. This fixes issues with the friendship email not
being sent because the email sending logic requires the passed user IDs
to be integers and not numeric strings.

Props slaFFik, henry.wright.

Fixes #7506.

#8 @r-a-y
5 months ago

  • Milestone changed from Awaiting Review to 2.9
Note: See TracTickets for help on using tickets.