Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce EDD_Email class #1810

Closed
pippinsplugins opened this issue Dec 18, 2013 · 41 comments

Comments

@pippinsplugins
Copy link
Member

commented Dec 18, 2013

Sending a custom email in EDD that uses the same templates as the purchase receipt is a serious pain.

We should introduce an EDD_Email class that makes this nice and simple.

To send an email with the standard EDD template should be as simple as:

$to = 'some@email.com';
$message = 'My email message text';

EDD()->emails->send( $to, $message )
@spencerfinnell

This comment has been minimized.

Copy link
Member

commented Dec 18, 2013

Okay, that's creepy. I was thinking about this this morning and almost started writing my own class. I'll start playing around with some ideas and share some thoughts as I continue.

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2013

Ha, glad to know I'm not the only one.

Note, it will tie in closely with the new email template tags: #1451

@evertiro

This comment has been minimized.

Copy link
Member

commented Dec 18, 2013

@pippinsplugins Any issue with adding a third argument to that like so:

function send( $to = '', $message = '', $html = false ) {}

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2013

I like the idea of being able to disable HTML, so no, I don't have any issues with that.

@evertiro

This comment has been minimized.

Copy link
Member

commented Dec 18, 2013

😄

@spencerfinnell

This comment has been minimized.

Copy link
Member

commented Dec 18, 2013

I like the idea of very minimal arguments for the final send method. Some good defaults set (basically what are used now) with the ability to override before sending.

$email = new EDD_Email;
$email->send( 'to@example.com', 'message with {template_tags}' );

or

$email = new EDD_Email;
$email->set( 'content_type', 'plain_text/html' );
$email->set( 'attachments', 'some attachments' );
$email->send( 'to@example.com', 'message with {template_tags}' );
@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2013

I like that @spencerfinnell

If we do that, we probably will not make EDD_Emails accessible through the main EDD() function.

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2013

Instead of setting the content_type (that would work), I'd like to see HTML / plain text handled with a simple flag:

$email->set( 'html', false );
@spencerfinnell

This comment has been minimized.

Copy link
Member

commented Dec 18, 2013

Related: #1736

I think all emails should apply the same template. And I really also like the idea of using real templates for emails as well. So the message would be wrapped with templates/emails/email-default-header.php and templates/emails/email-default-footer.php

And if a new email template is registered and used, it would look in the registered template locations for email-xxx-header.php so a plugin that registers a new template would just add their directory to the array of template locations.

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2013

I completely agree on using real template files, though that's a separate discussion.

@spencerfinnell

This comment has been minimized.

Copy link
Member

commented Dec 18, 2013

https://gist.github.com/spencerfinnell/8030539

Proof of concept. Though there is really no proof since it's not tested at all (literally not once). But the idea is there. Inspiration from WooCommerce, but less crazy.

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2013

Excellent.

One thing we have to take into account with actual template files is the Email Templates add-on. It will have to be updated at the same time. If possible, I'd like to support the old template method as well. Perhaps look to see if the template file exists and then load it the current way if not.

@chriscct7

This comment has been minimized.

Copy link
Member

commented Dec 19, 2013

I'd love to see a param for the subject on this one ;)

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2013

The example @spencerfinnell put up has one.

@chriscct7

This comment has been minimized.

Copy link
Member

commented Dec 19, 2013

Ah okay. Didn't see that.

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented May 19, 2014

Might need to punt this to 2.1. I'm running out of time and this one is crucial that it be extremely thoroughly tested.

@pippinsplugins pippinsplugins modified the milestones: 2.0.1, 2.0, 2.1 May 19, 2014

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented May 19, 2014

Punted to 2.1.

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2014

After some discussion with @sumobi we have decided to NOT maintain backwards compatibility for email templates with this issue.

The plan as of now is:

  • Build new template files
  • If An email template other than the default (or a new one with 2.1) is used, fallback to that template and bypass the new templating system
  • After 2.1 has been out for a bit, rebuild the existing email template extension to use the new template system and completely deprecate the existing method
pippinsplugins added a commit that referenced this issue Aug 9, 2014
pippinsplugins added a commit that referenced this issue Aug 9, 2014
@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2014

I'm merging this into release/2.1 now.

Thanks @spencerfinnell

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2014

Just needs extensive testing and unit tests now.

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2014

Need to fix a couple of unit tests related to this: https://travis-ci.org/easydigitaldownloads/Easy-Digital-Downloads/jobs/33739027

pippinsplugins added a commit that referenced this issue Aug 28, 2014
pippinsplugins added a commit that referenced this issue Aug 28, 2014
pippinsplugins added a commit that referenced this issue Aug 29, 2014
pippinsplugins added a commit that referenced this issue Aug 29, 2014
@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2014

@leewillis77 I think the best way to fix your issue with attachments, and also issues with other data being persistent, is to just not instantiate EDD_Emails in the main EDD class.

I think I will write an edd_email() function that wraps the class.

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2014

Ah, nevermind. That prevents me from pulling the heading info from within the template files.

@leewillis77

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2014

I've got a few ideas here, will try and pull together a PR tonight.

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2014

Great, thanks!

On Sunday, August 31, 2014, Lee Willis notifications@github.com wrote:

I've got a few ideas here, will try and pull together a PR tonight.


Reply to this email directly or view it on GitHub
#1810 (comment)
.

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2014

Something is up with the default templates: http://screencloud.net/v/gTgs

That's how they come through on the EDD site.

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2014

It works fine on PippinsPlugins.com: http://screencloud.net/v/3kiY

My guess is that it's caused by the WPMandrill plugin.

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2014

Yep, issue was caused by this setting in WPMandrill: http://screencloud.net/v/d66X

@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2014

Mandrill has a filter called mandrill_nl2br that lets us disable the option so I'm going to drop that in plugin-compatibility.php to prevent issues with anyone using WPMandrill.

pippinsplugins added a commit that referenced this issue Sep 1, 2014
@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2014

And fixed!
image

pippinsplugins added a commit that referenced this issue Sep 1, 2014
pippinsplugins added a commit that referenced this issue Sep 2, 2014
@pippinsplugins

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2014

@leewillis77 should fix the persistent heading: 0eccd28

@leewillis77

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2014

Yep - cheers @pippinsplugins (And congrats on 2.1!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.