Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#74 | system.mail_.inc_.patch | 6.09 KB | sun |
#72 | mail-fix.patch | 5.97 KB | mfb |
#71 | mail-fix-331180-71.patch | 2.75 KB | pwolanin |
#69 | mail-class.patch | 1.26 KB | mfb |
#66 | pluggable-smtp-331180-66.patch | 15.57 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedhere's the smtp patch
Comment #2
RobLoachOh man, this is awesome.
Marked #28604: Separate mail backend as duplicate.
Comment #3
RobLoachFor tests, what are your thoughts on something like this? The test, unfortunately, isn't passing and I'm not too sure why....
Comment #4
RobLoachThe test now works.... Should DrupalSmtpInterface be renamed to DrupalMailInterface because sometimes you're not using SMTP to send mail?
Comment #5
pwolanin CreditAttribution: pwolanin commentedIn 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.
Comment #6
pwolanin CreditAttribution: pwolanin commentedfor some reason this test is not working, but should be close to being done overall.
Comment #7
RobLoachThe 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.
Comment #8
pwolanin CreditAttribution: pwolanin commentedah, we are dealing with 2 instances of the object - that's why static is needed.
Comment #9
RobLoachUmm, that's what I said after fixing it in both #4 and #7 :-P .......
Comment #10
pwolanin CreditAttribution: pwolanin commented@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).
Comment #12
pwolanin CreditAttribution: pwolanin commentedapparently HEAD was broken for a bit. re-rolled patch to remove offset and re-tested.
Comment #13
pwolanin CreditAttribution: pwolanin commentedwhoops - patch missed the new test file.
Comment #14
RobLoachThe 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.
Comment #15
pwolanin CreditAttribution: pwolanin commented@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.
Comment #16
pwolanin CreditAttribution: pwolanin commentedgreatly expanded code comments.
Comment #17
keith.smith CreditAttribution: keith.smith commented:) Note there's an "incorrecly" in there. Déjà vu.
Comment #18
pwolanin CreditAttribution: pwolanin commentedhmm, that typo was in code comments I just copy/pasted- but fixed now.
Comment #19
pwolanin CreditAttribution: pwolanin commentedwhoops - omitted the new mail.test file from the patch.
Comment #20
RobLoachLooked great! Tests pass, lots of documentation is available. Setting this to RTBC.... This patch simply adds a code doc description to the DrupalMailSend class.
Comment #21
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe 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.
Comment #22
RobLoachDamZ: 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.
Comment #23
pwolanin CreditAttribution: pwolanin commented@Rob - I think DamZ was supporting this patch as a prerequisite for better handling of mail during tests.
Comment #24
RobLoachAh, cool. Thanks guys!
Comment #26
Dave ReidThe function "drupal_send" feels too ambiguous. Does it send mail? http requests? kittens? :)
Comment #27
pwolanin CreditAttribution: pwolanin commented@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.
Comment #28
Dave Reid1. 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.
Comment #29
pwolanin CreditAttribution: pwolanin commentedWell, 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.Comment #30
catchI think drupal_send()->mail() is good. Is there any chance this'll ever be used for non-email?
Comment #31
Damien Tournoud CreditAttribution: Damien Tournoud commentedI suggest drupal_smtp->send().
Comment #32
pwolanin CreditAttribution: pwolanin commented@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.
Comment #33
c960657 CreditAttribution: c960657 commentedIt 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: Ensure that the Return-Path is set when sending mail on both Windows and non-Windows systems.). 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.
Comment #34
Dries CreditAttribution: Dries commentedComment #35
chx CreditAttribution: chx commentedNote 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...Comment #36
pwolanin CreditAttribution: pwolanin commented@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.
Comment #37
pwolanin CreditAttribution: pwolanin commentedre-rolled patch with somewhat expanded code comments as suggested.
Comment #38
RobLoachThis 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.
Comment #39
c960657 CreditAttribution: c960657 commentedThe 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.
Comment #40
mfbSubscribe.
Comment #41
mr.baileysFYI, #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.
Comment #42
pwolanin CreditAttribution: pwolanin commentedgiven that handlers is still lingering we should get some version of this done asap
Comment #44
pwolanin CreditAttribution: pwolanin commentedreworked the simplest handing of e-mails so tests should pass for contact module.
Comment #45
sunExceeds 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.
Comment #46
pwolanin CreditAttribution: pwolanin commentedWith suggested (and more) doxygen fixes, plus make sure all implementations of mail() return a boolean.
Comment #47
webchickMoving to needs review so bot can take a crack at it.
Comment #49
RobLoachWhat does the test bot thing of this one?
Comment #50
moshe weitzman CreditAttribution: moshe weitzman commentedfwiw, 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,
Comment #51
pwolanin CreditAttribution: pwolanin commented@moshe - well, look at the greater flexibility this gives us, even re: testing.
Comment #52
pwolanin CreditAttribution: pwolanin commentedWith all test passing and doxygen fixes, I think this is good to go.
Comment #53
webchickI 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.
Comment #54
pwolanin CreditAttribution: pwolanin commentedputting to needs review for the bot - still need to clean up the doxygen a little.
Comment #56
pwolanin CreditAttribution: pwolanin commentedwhat!?
trying again with doxygen fixes.
Comment #58
pwolanin CreditAttribution: pwolanin commentedopps - forgot type hint in the interface and one implementation.
Comment #59
pwolanin CreditAttribution: pwolanin commentedI think all the comments above are addressed.
Comment #60
pwolanin CreditAttribution: pwolanin commentedthis is really a feature request.
Comment #61
pwolanin CreditAttribution: pwolanin commentedthis does change the API
Comment #62
pwolanin CreditAttribution: pwolanin commentedComment #63
Dries CreditAttribution: Dries commentedThe 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.
Comment #64
pwolanin CreditAttribution: pwolanin commented@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.
Comment #66
pwolanin CreditAttribution: pwolanin commentedsilly conflict with how git expands ID tags.
Comment #67
pwolanin CreditAttribution: pwolanin commentedok, new class and interface names and tests pass
Comment #68
webchickOk, looks like Dries's feedback was incorporated.
Committed to HEAD! :) Please document this addition in the module upgrade guide.
Comment #69
mfbLooks to me like the wrong class name is being used in mail.inc
Comment #70
pwolanin CreditAttribution: pwolanin commenteddoh, yeah - I was tired last night.
Comment #71
pwolanin CreditAttribution: pwolanin commentedapparently missing some other breakage in tests too.
Comment #72
mfbadded some more doc fixes
Comment #73
webchickDoh. :P Committed.
Back to needs work for docs.
Comment #74
sunI'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.
Comment #75
Dave ReidComment #76
catchLet's get the file rename in, moving this out of criticals queue though.
Comment #77
Dries CreditAttribution: Dries commentedRenamed in CVS. Thanks!
Comment #78
catchStill needs docs.
Comment #79
sun.
Comment #80
yettyn CreditAttribution: yettyn commentedI am wondering if this patch doesn't have a fundamental flaw, take this situation. User install smtp module to get that function, which replaces 'default-system' => 'DefaultMailSystem' with 'default-system' => 'SmtpMailSystem' to get the smtp functionallity. Now the user also get the idea of installing HTML Mail module to have some fancy html capabilities, which then set 'default-system' => 'HTMLMailMailSystem' to get access to and change the format functionality, but at the same time it switches Drupal back to use the default mail() function for delivery.
The problem is that the 2 modules need to manipulate 2 different areas of the same mail frame work, so one literally speak kicks the other out. I don't have any ready solution to this, just a reflexion, but obviously this opens up for lots of incompatibilities among mail modules. One thought springing though is that modules like HTML shouldn't create it's own class by implementing the interface but rather extending the base class. Likewise, a module like smtp could also offer html/text to Html Mail to set. I wonder if not some kind of abstraction is needed here so modules like HTML Mail asks for or extends the same class, no matter if it's Drupals default system or smtp providing it. Just thinking laud...
Comment #81
sunThanks. #80 summarizes exactly the reasons why PHP OOP is bad and harmful for Drupal on a concrete example. OOP in PHP is not modular. Any attempt to make it modular requires loads of abstractions to work around PHP's limitations. In turn, abstractions lead to loss of context, as well as over-engineered APIs, which, in turn, require expert PHP and OOP skills to work with. Effectively, entirely contradicting Drupal's design goals.
Comment #82
jhodgdonchanging tag to new scheme for update page
Comment #83
pillarsdotnet CreditAttribution: pillarsdotnet commentedSee related:
#1135262: drupal_mail() should support using different MailSystemInterface classes for format() and mail()
#1089956: Provide Administrative UI and Developers API for safely setting the mail_system variable.
Comment #84
jhodgdonLooking at the patch that was committed (we DESPERATELY need issue summaries)... I'm trying to figure out what the API change is that needs to be documented in the 6/7 module update guide.
What I'm seeing:
a) drupal_mail_send($message) ==> drupal_mail_sending_system($module, $key)->mail($message)
b) The new drupal_mail_sending_system() function uses some new classes/interfaces to do its work.
c) The variable governing what to use to send mail changed from variable_set('smtp_library') to variable_set('mail_sending_system').
d) The function drupal_mail_wrapper() went away, but this is a 7.x->7.x change (the function didn't exist in 6.x), so I'm not going to worry about it. There were also some simpletest changes, but simpletest was not core in d6, so we don't have to document them either.
SIGH. Actually, it looks like this changed after this patch was committed (no idea what issue that might have been on, as there's nothing else relevant tagged "needs documentation" or "needs update documentation". So I've made an attempt to document the actual changes at:
http://drupal.org/update/modules/6/7#mail-sending
Marking fixed for now, but if someone would like to review, that would be cool.
Comment #85
pillarsdotnet CreditAttribution: pillarsdotnet commentedCorrection:
variable_set('mail_system', array($key => $classname))
. But to avoid overwriting existing settings, you need something like the following:Then to clear just your module's setting on
hook_disable()
, you need:That's why I wrote and advocate using the Mail System module instead of setting that variable directly.
Instead of the above code blocks, simply call
mailsystem_set(array($key => $classname));
andmailsystem_clear(array($key => $classname));
respectively.Comment #86
jhodgdonRE #85 - did you look at what I actually wrote on the 6/7 module update guide page? I think it's correct there...
Comment #87
jhodgdonComment #88
pillarsdotnet CreditAttribution: pillarsdotnet commentedCorrect but incomplete.
Comment #89
jhodgdonRE #85 - I don't think I want to put all of that into the 6/7 module upgrade page. I think the information that is there is enough to get someone (a) to this issue and (b) to the necessary documentation on api.drupal.org. Or is there a doc page on drupal.org that I should link to?
Comment #90
pillarsdotnet CreditAttribution: pillarsdotnet commentedHere's the only doc page I could find; it's better than nothing: http://drupal.org/node/900794
Comment #91
jhodgdonOK, that's related to http://drupal.org/node/224333#email-html , which already has a link to that page.
So I think we're OK here... Really, the point of the update doc for *this* issue is to document that the drupal_send_mail function and variable_get('smtp_library', '') are gone, and to point people towards learning more. I think there are enough pointers.