WebDevStudios
Bp email support
Thanks for this. Not ready to merge in yet, as I do have notes that I would like looked at and reviewed.
Backwards compatibility
We need to make sure we have a tested, working way to import existing customized messages. From what I saw, we create the default ones, and then if the BP Email functionality is available, we use those new ones with default values. So we need to find a way to grab the saved values, parse out the shortcodes in favor of the BP Email placeholders, and inject those as the email content etc. Ideally, in case something goes wrong, and the user needs to or wants to revert to the previous version of the plugin, they should be able to. So, leaving the existing options in the database for the moment would also be an appreciate detail.
Importance of the singleton
I'm one of those developers who tries to avoid singletons as much as possible. It helps make the code testable in the event we want or need to implement unit tests, and generally makes lives easier, imho. I do understand the desire to have the class be pretty dedicated, but it's been my experience more often than not, that people aren't going to be trying to extend or use the class in some other way, making duplicate calls.
If we were to remove the singleton, we would need to at least at one point, create an object, and run the setup_actions() method once. With the only action being run being one that sets up the emails, I'm curious if we could put it in a callback that runs upon update to the newest version, or at least on like plugins_loaded. It is definitely good that we check if we need to first. Do you know how frequently the bp_core_install_emails action runs? is that a one time thing or is that technically on every page load, whether frontend, admin, or both? Curious if the current implementation here would force the emails from being able to be deleted. Things to consider. The email sending would then be possible with a different object instantiation without the setup_actions method invoking and still have access to the necessary methods.
Versioning
We use emantic versioning with BPRO, so that would make the @since tags at least 4.4.0. Only other note is that @since tags don't need the class name associated with it.
Minimum BP version.
I'm more than willing to bump minimum version of BP for this. We're notoriously old at the moment for minimum version required.
String translations.
Please use appropriate esc_* functions whenever possible for string security in translations. If it's an attribute, esc_attr__ or esc_attr_e, if html, esc_html__ or esc_html_e, etc. I would say don't pass user-saved values through them, as they may have custom html already in place for their version. However the default texts for the emails will be just fine for them.
That's all the notes I have at the moment. I must say I appreciate the help with this and look forward to seeing what this turns into as a whole for the plugin.
Thank you for your extensive feedback. I agree to your points. We are doing this as a part of a new project and will try to incorporate your feedback with regards to our schedule.
To your question regarding bp_core_install_emails. This is only run on fresh installs, when the user chooses the "Reinstall Emails" option in the BuddyPress Tools page or on update to version 2.5. It is not run on init or on every page load.
Regarding the minimum BP Version. Does this mean you are OK with removing the legacy functions if we bump the minimal required BP Version to 2.5.0?
Awesome to hear, regarding the install emails hook and such. It's not a regular thing.
Regarding the legacy functions, since this is done as an opt in thing, and not something we've explicitly forced onto the user, it would be best to leave the legacy email methods in place. If we decide to force the plugin to use BP_Email, then we can remove those at that time. Something we can definitely test for ahead of time before mass release.
Regarding the singleton issue, code like this
BP_Registration_Emails::instance()->send_moderation_email( $user, $action );
would become
$email = new BP_Registration_Emails;
$email->send_moderation_email( $user, $action );
Is this what you expect?
Yeah. If/when all of the emails needing to be sent share the same scope, like all within 1 callback on a hook, then that $email object would only need to be created once, and then re-used each time. Whenever that scope changes, then a new object would need to be made. However, given the size of the class itself, that should be really low impact, especially when it's not self-hooking up a bunch of actions.