fix pluggable smtp/mail framework
pwolanin - November 7, 2008 - 01:40
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | postponed |
| Issue tags: | Favorite-of-Dries |
Description
Branched from: http://drupal.org/node/259103
With the advent of the code registry, several of the existing pluggable frameworks can no longer work as intended since they assume multiple include files with the same functions defined.
The patch will basically follow the outline at http://www.garfieldtech.com/blog/pluggable-systems-howto with the addition of a declared interface to make it a well-defined API.
#1
here's the smtp patch
#2
Oh man, this is awesome.
Marked #28604: Separate mail backend as duplicate.
#3
For tests, what are your thoughts on something like this? The test, unfortunately, isn't passing and I'm not too sure why....
#4
The test now works.... Should DrupalSmtpInterface be renamed to DrupalMailInterface because sometimes you're not using SMTP to send mail?
#5
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.
#6
for some reason this test is not working, but should be close to being done overall.
#7
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.
#8
ah, we are dealing with 2 instances of the object - that's why static is needed.
#9
Umm, that's what I said after fixing it in both #4 and #7 :-P .......
#10
@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).
#11
The last submitted patch failed testing.
#12
apparently HEAD was broken for a bit. re-rolled patch to remove offset and re-tested.
#13
whoops - patch missed the new test file.
#14
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....
<?php
function drupal_send($module, $key) {
static $instances = array();
$id = $module . '_' . $key;
$configuration = variable_get('mail_sending_system', array('default' => 'DrupalMailSend'));
// Look for overrides for the default class, starting from the most specific
// id, and falling back to the module name.
if (isset($configuration[$id])) {
$class = $configuration[$id];
}
elseif (isset($configuration[$module])) {
$class = $configuration[$module];
}
else {
$class = $configuration['default'];
}
if (empty($instances[$class])) {
$interfaces = class_implements($class);
if (isset($interfaces['DrupalMailSendingInterface'])) {
$instances[$class] = new $class;
?>
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.
#15
@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.
#16
greatly expanded code comments.
#17
:) Note there's an "incorrecly" in there. Déjà vu.
#18
hmm, that typo was in code comments I just copy/pasted- but fixed now.
#19
whoops - omitted the new mail.test file from the patch.
#20
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.
#21
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.
#22
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.
#23
@Rob - I think DamZ was supporting this patch as a prerequisite for better handling of mail during tests.
#24
Ah, cool. Thanks guys!
#25
The last submitted patch failed testing.
#26
The function "drupal_send" feels too ambiguous. Does it send mail? http requests? kittens? :)
#27
@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.
#28
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.
#29
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.#30
I think drupal_send()->mail() is good. Is there any chance this'll ever be used for non-email?
#31
I suggest drupal_smtp->send().
#32
@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.
#33
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.array('to' => 'foobar@example.com',
'cc' => array(
array('Lorem, ipsum', 'foo@example.org'),
'bar@example.org'
)
)
+ * - to+ * The mail address or addresses where the message will be sent to. The
+ * formatting of this string must comply with RFC 2822. Some examples are:
+ * user@example.com
+ * user@example.com, anotheruser@example.com
+ * User <user@example.com>
According to the PHP manual page, the latter format is not supported on Windows.
+ * - headers+ * Associative array containing all mail headers.
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.
#34
#35
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...#36
@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.
#37
re-rolled patch with somewhat expanded code comments as suggested.
#38
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.
#39
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.orgstyle 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.
#40
Subscribe.
#41
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.