fix pluggable smtp/mail framework

here's the smtp patch

Oh man, this is awesome.

Marked #28604: Separate mail backend as duplicate.

For tests, what are your thoughts on something like this? The test, unfortunately, isn't passing and I'm not too sure why....

The test now works.... Should DrupalSmtpInterface be renamed to DrupalMailInterface because sometimes you're not using SMTP to send mail?

In D6 we called the library the "smtp_library" regardless of how it was implemented. So, the naming was basically an extension of that existing convention.

That being said, I don't really care waht we call the interface and the facory function if you can think of something else reasonable and reasonably short.

for some reason this test is not working, but should be close to being done overall.

The problem is that you're running a different instance, so you're using the wrong $sent_message. Making it static, and referencing the static variable makes us reference the correct value.

ah, we are dealing with 2 instances of the object - that's why static is needed.

Umm, that's what I said after fixing it in both #4 and #7 :-P .......

@Rob Loach - I must have cross-posted with you (I had the window open for a long time) - didn't see your comment/patch in #7 when I made mine (which was more of a response to dimitrig01 in IRC).

The last submitted patch failed testing.

apparently HEAD was broken for a bit. re-rolled patch to remove offset and re-tested.

whoops - patch missed the new test file.

The only thing I'm not too fond of in this patch, or the Drupal mail system in general, is the $module and $key mentality....

Although it might be out of scope for this patch, there must be a better way of using this $module and $key implementation. Also, it would be nice to have some documentation saying how to use the mail system through variable get and DrupalMailSendingInterface..... That's a much better name then DrupalSmtpInterface, by the way.

@Rob Loach - this seems to me to be the simplest logic to get the desired behavior from the id/module. However, you may be right that some more code comments should be supplied to direct the developer.

greatly expanded code comments.

:) Note there's an "incorrecly" in there. Déjà vu.

hmm, that typo was in code comments I just copy/pasted- but fixed now.

whoops - omitted the new mail.test file from the patch.

Looked great! Tests pass, lots of documentation is available. Setting this to RTBC.... This patch simply adds a code doc description to the DrupalMailSend class.

We need this to properly implement mail catch-up in simpletest (the test framework currently try to send mail outsite... not that great). Raising to critical.

DamZ: I think testing the mail system itself is out of scope for this issue and can live nicely in a post-patch here: #276409: Tests needed mail.inc. This issue is just to make the mail framework pluggable.

@Rob - I think DamZ was supporting this patch as a prerequisite for better handling of mail during tests.

Ah, cool. Thanks guys!

The last submitted patch failed testing.

The function "drupal_send" feels too ambiguous. Does it send mail? http requests? kittens? :)

@Dave Reid: Webchick and I struggled for some time to come up with even this (still somewhat lame) naming. If you have a better suggestion, please make it.

1. drupal_mail_send()->mail()
2. drupal_send_mail()->mail()
3. drupal_send_mail()->send()
4. drupal_mail()->send() - Conflicts with existing function name

Numbers 2 or 3 seem easiest to figure out what it's doing and follows the drupal_verb_noun syntax.

Well, I'm not sure any of those are better than what's already in the patch, to be honset.

This is only ever used as drupal_send()->mail() in core, so we'll already have the verb-noun there to see.

I think drupal_send()->mail() is good. Is there any chance this'll ever be used for non-email?

I suggest drupal_smtp->send().

@Damien - I has something like that originally - but was pushed to find something else since (as you know) this may actually log the mail or do something else with it rather than send it out via smtp.

It is important that all implementors of DrupalMailSendingInterface work in the same way so that callers know exactly how the mailer behaves regardless what implementation is used. PHP's mail() has some rather subtle features that are inherited by DrupalMailSend. These should be properly documented, so that alternative implementations can replicate these.

It should be documented, that mail() looks for Cc and Bcc headers and sends the mail to addresses in these headers too. Note that splitting a Cc header, e.g. "Lorem, ipsum" <foo@example.org>, bar@example.org, into separate addresses is not completely trivial, so perhaps it would be better to make the caller supply all recipients in a structured array, e.g.

According to the PHP manual page, the latter format is not supported on Windows.

It should be documented whether this array contains all headers or only those not specified by other arguments (to, subject).

It should be documented how to specify the envelope sender, i.e. the address used in the MAIL FROM: SMTP command. This address is used for bounce messages. mail() does this using a fifth parameter (see #131737: Return-Path overwritten by the PHP mail() function). The PEAR Mail package (and possibly others?) uses the Return-Path header for this. Note that according to RFC 2821, section 4.4, "[a] message-originating SMTP system SHOULD NOT send a message that already contains a Return-path header field", so if this header is used, it should not actually be sent on the wire.

Note that I used module_invoke(variable_get('field_storage_module', 'field_sql_storage'), $op, $arg1, $arg2); in field module. That's like a Drupal-ish solution...

@chx - as much as that is more Drupalish in some ways, I think an "Interface" is exactly the right thing for these cases - it clearly defines exactly which methods need to be implemented in order to plug in.

re-rolled patch with somewhat expanded code comments as suggested.

This is looking RTBC! Well done, Peter. So this is how you spend your theming seminar? Hahaha.... Crell just posted #363787: Plugins: Swappable subsystems for core, so I think we should use that when it goes in. The code changes won't be much from the current patch.

The mail interface seems to be modeled after how PHP's mail() works. I don't think mail() has a very clean interface. It gives the impression that the envelope sender and recipient(s) has to appear in the message headers, and that the subject header has some kind of special status. The requirement that implementations should be able to parse a "Doe, John" <jd@example.net>, foobar@example.org style address in order to get the recipient list seems like a needlessly complex requirement.

I don't expect the pluggable SMTP handler to be called directly by modules, so it doesn't have to be very "user friendly", i.e. the subject header might as well be submitted as part of the header array. Also, the requirement that implementations should accept both LF and CR LF seems like a convenience feature that should be moved to drupal_mail() instead of being duplicated in every implementation.

I suggest that the $message parameter should contain the following entries: envelope_sender (in simple format as accepted by valid_email_address()), envelope_recipients (array of simple addresses), headers (array), and body (string). The headers array should mention all headers, including To, From, Subject etc.

Subscribe.

FYI, #321771: Make drupal_mail_wrapper a hook. is trying to achieve the same goal but via a different solution. It might make sense to select the best approach and close that issue or this one.

given that handlers is still lingering we should get some version of this done asap

The last submitted patch failed testing.

reworked the simplest handing of e-mails so tests should pass for contact module.

Exceeds 80 chars.

Should use @see in both cases.

Wrong indentation.

Description should follow after a colon of the element property.

(and elsewhere)

Exceeds 80 chars.

Should use the regular list syntax to list stuff.

Double space.

Exceeds 80 chars.

Trailing white-space.

Double space.

Trailing white-space.

15 days to code freeze. Better review yourself.

With suggested (and more) doxygen fixes, plus make sure all implementations of mail() return a boolean.

Moving to needs review so bot can take a crack at it.

The last submitted patch failed testing.

What does the test bot thing of this one?

fwiw, i would be happy with a variable_get('mail_inc') solution like hx proposed. thats what we do in d7 for session.inc, cache.inc,

@moshe - well, look at the greater flexibility this gives us, even re: testing.

With all test passing and doxygen fixes, I think this is good to go.

I see that c960657's comments about structuring the data coming into drupal_mail() so they are more sensible are not taken into account in this patch. I talked to Peter about this and he pointed out that this existing patch just basically takes the mail API we have and makes it pluggable, rather than making it actually make _sense_. Doing that would be a separate issue. I think I agree with this, even though I so very desperately loathe our current mail handling API. ;P

This looks a little bit weird. We bothered to abstract the name of this thing to "drupal_send" so it wasn't tied to mailing, but yet the function you call is called "mail()". That makes me wonder why we don't just leave the function the same name.

Except now I think I remember. It's because it's not actually sending anything, it's instantiating an MailSendingThingy object and calling its mail() method.

In that case, I think we should name this to drupal_mail_sending_system() because that would be way more obvious.

There is no need to duplicate this PHPDoc in two places. Putting it in the interface is fine.

These do not belong in core's includes/mail.inc. They belong in mail_test.module or similar.

(Peter pointed out, actually simpletest.module since they need to handle mail in several tests.)

Though I know what you mean, that sentence is a bit awkward. Could you slightly rephrase?

I agree with Moshe and chx that this might be overkill, and it's fancier than what we do in any other variable replacement API in Drupal. OTOH, I can see this opening the door for some very interesting things, like sending event notification emails via SMS, but not user registration emails. This gives us additional flexibility without bloating the system too badly, so I support its inclusion.

There's a lot of knowledge in this issue about use cases around this, and it'd be really nice to have more background as to why a module might implement this interface.

Missing a /** @file */ thing here.

No PHPDoc here because they're inherited from the base class.

No t()s around getInfo strings.

6 days to code freeze. Better review yourself.

putting to needs review for the bot - still need to clean up the doxygen a little.

The last submitted patch failed testing.

what!?

trying again with doxygen fixes.

The last submitted patch failed testing.

opps - forgot type hint in the interface and one implementation.

I think all the comments above are addressed.

this is really a feature request.

this does change the API

The name of the class (DrupalMailSend) and the name of the interface (DrupalMailSendingInterface) are not 100% IMO. I'd prefer to see something like DefaultMailSystem and MailSystemInterface.

That said, what was wrong with a variable_set()/variable_get()? It feels like this solution is a bit over-engineered -- it didn't came across as an elegant solution.

@Dries - renaming is fine - new patch attached. The use of an array versus a scalar for the variable value is similar to the DB or caching system where we have routing to multiple possible targets.

The last submitted patch failed testing.

silly conflict with how git expands ID tags.

ok, new class and interface names and tests pass

Ok, looks like Dries's feedback was incorporated.

Committed to HEAD! :) Please document this addition in the module upgrade guide.

Looks to me like the wrong class name is being used in mail.inc

doh, yeah - I was tired last night.

apparently missing some other breakage in tests too.

added some more doc fixes

Doh. :P Committed.

Back to needs work for docs.

I'm also not quite sure why an implementation of System module uses a filename of mail.sending.inc. That's not following our current standard. We can discuss this standard for D8, but for D7, we should get things consistent.

Attached patch renames the file. No other changes.