Skip to:
Content

Opened 2 years ago

Closed 21 months ago

#7024 closed defect (bug) (wontfix)

BP no longer filters `wp_mail_from_name` when using `wp_mail()`

Reported by: boonebgorges Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.5.0
Component: Emails Keywords:
Cc:

Description

Prior to 2.5, BP filtered wp_mail_from_name to replace with bp_get_option( 'blogname' ). In 2.5, if you force the use of wp_mail(), this filtering no longer takes place.

Was it taken out just to clean things up, or for some more principled reason? It's no biggie to replace it on client sites, but it involved some annoying debugging.

#6999 is related, but is not specifically about wp_mail().

Attachments (2)

7024.01.patch (1.1 KB) - added by DJPaul 2 years ago.
7024.02.patch (3.1 KB) - added by DJPaul 2 years ago.

Download all attachments as: .zip

Change History (19)

#1 @johnjamesjacoby
2 years ago

An example from the forums:
https://buddypress.org/support/topic/change-the-email-from-name-buddypress-email-templates/


For anyone that's already familiar with customizing WordPress's wp_mail() function, BuddyPress introduces an entirely new set of conventions that do not inherit WordPress's.

Here are the commonly used WordPress filters wp_mail() has that BuddyPress does not have:

  • wp_mail_from (Done #6999?)
  • wp_mail_from_name (Done #6999?)
  • wp_mail_charset
  • wp_mail_content_type
  • wp_mail_failed
  • wp_mail_original_content

For reference, BuddyPress avoided using wp_mail() because of its pluggable nature. I'd like to suggest this blocker be revisited (here or elsewhere's.) wp_mail() is widely used and relied upon, and not using it:

  • creates a negative developer experience
  • increases the learning curve of interacting with BP Email
  • increases our maintenance overhead of BuddyPress's codebase
  • duplicates a pretty significant portion of PHPMailer & friends
Last edited 2 years ago by johnjamesjacoby (previous) (diff)

#2 @boonebgorges
2 years ago

@johnjamesjacoby - Your requests for retaining backward compatibility with existing filters are not without merit. But, just for clarity, my issue in this ticket is a mostly separate one: I am choosing to use wp_mail() instead of the new email functionality, and BP has stopped filtering some values in wp_mail() that it always filtered in the past.

#3 @johnjamesjacoby
2 years ago

@boonebgorges - This is why I mention both the list of filters that are missing, and mention retaining wp_mail() because then we wouldn't need to maintain our own separate whitelist of WordPress core filters in BuddyPress's API.

#4 @boonebgorges
2 years ago

OK. I just don't want my modest ticket, which is intended to be a questions as much as a bug report, to get buried by an extensive discussion about the architecture of the mail feature (or your legitimate concerns to get buried in a ticket about a minor bug).

#5 @DJPaul
2 years ago

@boonebgorges I think it was just removed as part of tidying up. Re-instating it as it was is going to affect the new implementation after #6999. How about something like this?

  • Put back bp_core_email_from_name_filter() to wp_mail_from_name filter.
  • And then apply the patch I'm about to upload.

I'm on vacation for a few weeks so if you think this is close, you are very welcome to run with it. :)

@DJPaul
2 years ago

#6 @boonebgorges
2 years ago

Thanks, @DJPaul. Your plan sounds OK, except I don't really understand your patch. What is it about reinstating the filter that means we have to run the title through wp_specialchars_decode()? As far as I see, that's the only change you're proposing.

This can wait until after you're back :)

#7 @DJPaul
2 years ago

https://github.com/paulgibbs/BuddyPress/commit/2a621518d4f2eb39c848c2b96999e952b853fefe was the commit for this during development.

As things stand, we use the site name (option blogname) for the default value of the "From name" property. When you set blogname in WordPress, it's sanitized with esc_html via sanitize_option before being stored in the database. Email from/reply to/to/bbc/cc "names" should not be HTML escaped otherwise you can end up with odd-looking values in the email, which in my testing sometimes caused deliverability issue.

In this patch, as we've fixed wp_mail_from_name support for your edge case (IMO - it wasn't a use case I had considered during development), I figured falling back to use its default value of "WordPress" rather than blogname made sense for consistency with wp_mail.

And then the now-deprecated function you found which filters in the blogname value for us, results in no change from current 2.5 behaviour, except we fix the problem you reported for people using wp_mail on a BuddyPress site.

(The patch obviously needs to reinstate the old function, I was just looking at the filtering aspect).

Last edited 2 years ago by DJPaul (previous) (diff)

#8 @DJPaul
2 years ago

  • Component changed from API to API - Emails
  • Milestone changed from Awaiting Review to 2.5.4

@DJPaul
2 years ago

#9 @DJPaul
2 years ago

The updated patch is a full, tested patch for this issue on the 2.5, with the changes described previously. However, @boonebgorges, I am not sure about doing this because I expect most BP sites to use BP email APIs. This patch means we are messing with wp_mail's from name, even though we aren't using wp_mail. I do agree however that this has been a small backwards compatibility break in 2.5.

At any rate, I do not mind significantly either way (whether we commit it or not), so I'll leave it up to you to make the call.

#10 @DJPaul
2 years ago

  • Milestone changed from 2.5.4 to 2.6.1

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


2 years ago

#12 @boonebgorges
2 years ago

7024.02.patch looks OK to me, though we should remove the _deprecated_function() call. I have not tested, so I'll leave it to someone who can test to do the committing :-D

#13 @dcavins
2 years ago

  • Milestone changed from 2.6.1 to 2.6.2

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


23 months ago

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


23 months ago

#16 @dcavins
23 months ago

  • Milestone changed from 2.6.2 to 2.7

#17 @boonebgorges
21 months ago

  • Milestone 2.7 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I haven't heard any uproar about this, so let's not do it.

Note: See TracTickets for help on using tickets.