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.

Files: 
CommentFileSizeAuthor
#74 system.mail_.inc_.patch6.09 KBsun
Passed: 14700 passes, 0 fails, 0 exceptions
[ View ]
#72 mail-fix.patch5.97 KBmfb
Failed: Failed to apply patch.
[ View ]
#71 mail-fix-331180-71.patch2.75 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#69 mail-class.patch1.26 KBmfb
Failed: Failed to apply patch.
[ View ]
#66 pluggable-smtp-331180-66.patch15.57 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#64 pluggable-smtp-331180-64.patch15.57 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#58 pluggable-smtp-331180-58.patch16.88 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#56 pluggable-smtp-331180-56.patch16.83 KBpwolanin
Failed: 12310 passes, 25 fails, 2 exceptions
[ View ]
#54 pluggable-smtp-331180-54.patch16.38 KBpwolanin
Failed: Failed to run tests.
[ View ]
#49 331180.patch14.88 KBRobLoach
Failed: 12348 passes, 4 fails, 0 exceptions
[ View ]
#46 pluggable-smtp-331180-46.patch14.67 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#44 pluggable-smtp-331180-44.patch14.08 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#37 pluggable-smtp-331180-37.patch9.86 KBpwolanin
Failed: 11849 passes, 4 fails, 2 exceptions
[ View ]
#20 331180.patch10.41 KBRobLoach
Failed: Failed to apply patch.
[ View ]
#19 pluggable-smtp-331180-19.patch10.51 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#18 pluggable-smtp-331180-18a.patch8.77 KBpwolanin
Unable to apply patch pluggable-smtp-331180-18a.patch
[ View ]
#16 pluggable-smtp-331180-16.patch8.77 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#13 pluggable-smtp-331180-14.patch9.59 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#12 pluggable-smtp-331180-12.patch7.86 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#8 pluggable-smtp-331180-7.patch9.59 KBpwolanin
Failed: Failed to apply patch.
[ View ]
#7 331180.patch9.43 KBRobLoach
Failed: Failed to install HEAD.
[ View ]
#6 pluggable-smtp-331180-6.patch9.46 KBpwolanin
Failed: Failed to install HEAD.
[ View ]
#4 331180.patch8.69 KBRobLoach
Failed: Failed to install HEAD.
[ View ]
#3 331180.patch7.76 KBRobLoach
Failed: Failed to install HEAD.
[ View ]
#1 pluggable-smtp-331180-1.patch6.6 KBpwolanin
Failed: Failed to install HEAD.
[ View ]

Comments

pwolanin’s picture

Category:task» bug
Status:Active» Needs review
StatusFileSize
new6.6 KB
Failed: Failed to install HEAD.
[ View ]

here's the smtp patch

RobLoach’s picture

Oh man, this is awesome.

Marked #28604: Separate mail backend as duplicate.

RobLoach’s picture

Status:Needs review» Needs work
StatusFileSize
new7.76 KB
Failed: Failed to install HEAD.
[ View ]

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

RobLoach’s picture

Status:Needs work» Needs review
StatusFileSize
new8.69 KB
Failed: Failed to install HEAD.
[ View ]

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

pwolanin’s picture

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.

pwolanin’s picture

Status:Needs review» Needs work
StatusFileSize
new9.46 KB
Failed: Failed to install HEAD.
[ View ]

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

RobLoach’s picture

Status:Needs work» Needs review
StatusFileSize
new9.43 KB
Failed: Failed to install HEAD.
[ View ]

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.

pwolanin’s picture

StatusFileSize
new9.59 KB
Failed: Failed to apply patch.
[ View ]

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

RobLoach’s picture

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

pwolanin’s picture

@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).

Status:Needs review» Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new7.86 KB
Failed: Failed to apply patch.
[ View ]

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

pwolanin’s picture

StatusFileSize
new9.59 KB
Failed: Failed to apply patch.
[ View ]

whoops - patch missed the new test file.

RobLoach’s picture

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.

pwolanin’s picture

@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.

pwolanin’s picture

StatusFileSize
new8.77 KB
Failed: Failed to apply patch.
[ View ]

greatly expanded code comments.

keith.smith’s picture

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

pwolanin’s picture

StatusFileSize
new8.77 KB
Unable to apply patch pluggable-smtp-331180-18a.patch
[ View ]

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

pwolanin’s picture

StatusFileSize
new10.51 KB
Failed: Failed to apply patch.
[ View ]

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

RobLoach’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new10.41 KB
Failed: Failed to apply patch.
[ View ]

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.

Damien Tournoud’s picture

Priority:Normal» Critical

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.

RobLoach’s picture

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.

pwolanin’s picture

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

RobLoach’s picture

Ah, cool. Thanks guys!

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Dave Reid’s picture

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

pwolanin’s picture

@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.

Dave Reid’s picture

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.

pwolanin’s picture

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.

catch’s picture

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

Damien Tournoud’s picture

I suggest drupal_smtp->send().

pwolanin’s picture

@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.

c960657’s picture

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: 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.

Dries’s picture

Issue tags:+Favorite-of-Dries
chx’s picture

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...

pwolanin’s picture

@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.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new9.86 KB
Failed: 11849 passes, 4 fails, 2 exceptions
[ View ]

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

RobLoach’s picture

Status:Needs review» Postponed

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.

c960657’s picture

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.

mfb’s picture

Subscribe.

mr.baileys’s picture

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.

pwolanin’s picture

Status:Postponed» Needs review

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

Status:Needs review» Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new14.08 KB
Failed: Failed to apply patch.
[ View ]

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

sun’s picture

Status:Needs review» Needs work
+++ includes/mail.inc
@@ -140,43 +140,43 @@ function drupal_mail($module, $key, $to, $language, $params = array(), $from = N
+   * More information in the <a href="http://php.net/manual/en/function.mail.php">

Exceeds 80 chars.

+++ includes/mail.inc
@@ -140,43 +140,43 @@ function drupal_mail($module, $key, $to, $language, $params = array(), $from = N
+   * PHP function reference for mail()</a>. See drupal_mail() for information on
+   * how $message is composed.

Should use @see in both cases.

+++ includes/mail.inc
@@ -140,43 +140,43 @@ function drupal_mail($module, $key, $to, $language, $params = array(), $from = N
+   *   - id
...
+   *   - to
...
+   *   - subject
...
+   *   - body
...
+   *   - headers

Wrong indentation.

Description should follow after a colon of the element property.

(and elsewhere)

+++ includes/mail.inc
@@ -140,43 +140,43 @@ function drupal_mail($module, $key, $to, $language, $params = array(), $from = N
+   *      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>
+   *       User <user@example.com>, Another User <anotheruser@example.com>

Exceeds 80 chars.

Should use the regular list syntax to list stuff.

+++ includes/mail.inc
@@ -195,6 +195,142 @@ function drupal_mail_send($message) {
+   * @param  $message
...
+   * @param  $message

Double space.

+++ includes/mail.inc
@@ -195,6 +195,142 @@ function drupal_mail_send($message) {
+   *   An e-mail message. See drupal_mail() for information on how $message is composed.

Exceeds 80 chars.

+++ includes/mail.inc
@@ -195,6 +195,142 @@ function drupal_mail_send($message) {
+/**
+ * Returns an object that implements DrupalMailSendingInterface.
...
+ *

Trailing white-space.

+++ includes/mail.inc
@@ -195,6 +195,142 @@ function drupal_mail_send($message) {
+ * name as the value for the key corresponding to the module name.  For example

Double space.

+++ modules/simpletest/tests/mail.test
@@ -0,0 +1,57 @@
+   * Implementation of setUp().

Trailing white-space.

15 days to code freeze. Better review yourself.

pwolanin’s picture

StatusFileSize
new14.67 KB
Failed: Failed to apply patch.
[ View ]

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

webchick’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch failed testing.

RobLoach’s picture

Status:Needs work» Needs review
StatusFileSize
new14.88 KB
Failed: 12348 passes, 4 fails, 0 exceptions
[ View ]

What does the test bot thing of this one?

moshe weitzman’s picture

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,

pwolanin’s picture

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

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community

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

webchick’s picture

Status:Reviewed & tested by the community» Needs work
+++ includes/mail.inc 18 Aug 2009 21:23:56 -0000
@@ -61,10 +61,10 @@
  * @param $to
  *   The e-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>
- *    User <user@example.com>, Another User <anotheruser@example.com>
+ *   - user@example.com
+ *   - user@example.com, anotheruser@example.com
+ *   - User <user@example.com>
+ *   - User <user@example.com>, Another User <anotheruser@example.com>

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

+++ includes/mail.inc 18 Aug 2009 21:23:56 -0000
@@ -127,7 +127,7 @@
-    $message['result'] = drupal_mail_send($message);
+    $message['result'] = drupal_send($module, $key)->mail($message);

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.

+++ includes/mail.inc 18 Aug 2009 21:23:56 -0000
@@ -140,43 +140,38 @@
+class DrupalMailSend implements DrupalMailSendingInterface {
+  /**
+   * Send an e-mail message, using Drupal variables and default settings.
+   * @see http://php.net/manual/en/function.mail.php the PHP function reference
+   *   for mail().
+   * @see drupal_mail() for information on how $message is composed.
+   *

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

+++ includes/mail.inc 18 Aug 2009 21:23:56 -0000
@@ -195,6 +190,144 @@
+class DrupalTestMailSend implements DrupalMailSendingInterface {
...
+class DrupalDevNullMailSend implements DrupalMailSendingInterface {

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.)

+++ includes/mail.inc 18 Aug 2009 21:23:56 -0000
@@ -195,6 +190,144 @@
+ * is taken from the value corresponding to the 'default-system' key. To use
+ * a different system for all mail sent by one module, specify also a class
+ * name as the value for the key corresponding to the module name.  For example

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

+++ includes/mail.inc 18 Aug 2009 21:23:56 -0000
@@ -195,6 +190,144 @@
+ * @code
+ * array(
+ *   'default-system' => 'DrupalMailSend',
+ *   'user' => 'DevelMailLog',
+ *   'contact_page_autoreply' => 'DrupalDevNullMailSend',
+ * );
+ * @endcode

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.

+++ includes/mail.inc 18 Aug 2009 21:23:56 -0000
@@ -195,6 +190,144 @@
+/**
+ * An interface for pluggable mail back-ends.
+ */
+interface DrupalMailSendingInterface {

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.

+++ modules/simpletest/tests/mail.test 1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,57 @@
+<?php
+// $Id$
+
+/**
+ * Test the Drupal mailing system.
+ */
+class MailTestCase extends DrupalWebTestCase implements DrupalMailSendingInterface {

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

+++ modules/simpletest/tests/mail.test 1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,57 @@
+  /**
+   * Implementation of getInfo().
+   */
...
+  /**
+   * Implementation of setUp().
+   */

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

+++ modules/simpletest/tests/mail.test 1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,57 @@
+    return array(
+      'name' => t('Mail system'),
+      'description' => t('Performs tests on the pluggable mailing framework.'),
+      'group' => t('System'),
+    );

No t()s around getInfo strings.

6 days to code freeze. Better review yourself.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new16.38 KB
Failed: Failed to run tests.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new16.83 KB
Failed: 12310 passes, 25 fails, 2 exceptions
[ View ]

what!?

trying again with doxygen fixes.

Status:Needs review» Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new16.88 KB
Failed: Failed to apply patch.
[ View ]

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

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community

I think all the comments above are addressed.

pwolanin’s picture

Category:bug» feature

this is really a feature request.

pwolanin’s picture

Issue tags:+API change

this does change the API

pwolanin’s picture

Assigned:Unassigned» pwolanin
Dries’s picture

Status:Reviewed & tested by the community» Needs work

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.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new15.57 KB
Failed: Failed to apply patch.
[ View ]

@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.

Status:Needs review» Needs work

The last submitted patch failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new15.57 KB
Failed: Failed to apply patch.
[ View ]

silly conflict with how git expands ID tags.

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community

ok, new class and interface names and tests pass

webchick’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs Documentation

Ok, looks like Dries's feedback was incorporated.

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

mfb’s picture

Status:Needs work» Needs review
StatusFileSize
new1.26 KB
Failed: Failed to apply patch.
[ View ]

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

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community

doh, yeah - I was tired last night.

pwolanin’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.75 KB
Failed: Failed to apply patch.
[ View ]

apparently missing some other breakage in tests too.

mfb’s picture

StatusFileSize
new5.97 KB
Failed: Failed to apply patch.
[ View ]

added some more doc fixes

webchick’s picture

Status:Needs review» Needs work

Doh. :P Committed.

Back to needs work for docs.

sun’s picture

Category:feature» task
Status:Needs work» Needs review
StatusFileSize
new6.09 KB
Passed: 14700 passes, 0 fails, 0 exceptions
[ View ]

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.

Dave Reid’s picture

Component:base system» mail system
catch’s picture

Priority:Critical» Normal
Status:Needs review» Reviewed & tested by the community
Issue tags:+Quick fix

Let's get the file rename in, moving this out of criticals queue though.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Renamed in CVS. Thanks!

catch’s picture

Status:Fixed» Needs work

Still needs docs.

sun’s picture

Issue tags:-Quick fix, -API change

.

yettyn’s picture

I 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...

sun’s picture

Thanks. #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.

jhodgdon’s picture

changing tag to new scheme for update page

jhodgdon’s picture

Status:Needs work» Fixed

Looking 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.

pillarsdotnet’s picture

Status:Fixed» Needs work

... variable_set('mail_sending_system').

Correction: variable_set('mail_system', array($key => $classname)). But to avoid overwriting existing settings, you need something like the following:

<?php
$mail_system
= variable_get('mail_system', array('default-system' => 'DefaultMailSystem'));
$mail_system = array_diff_key($mail_system, array($key => $classname));
$mail_system += array($key => $classname);
variable_set('mail_system', $mail_system);
?>

Then to clear just your module's setting on hook_disable(), you need:

<?php
$mail_system
= variable_get('mail_system', array('default-system' => 'DefaultMailSystem'));
$mail_system = array_diff_key($mail_system, array($key => $classname));
if (
$key == 'default-system') {
 
$mail_system['default-system'] = 'DefaultMailSystem';
}
variable_set('mail_system', $mail_system);
?>

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)); and mailsystem_clear(array($key => $classname)); respectively.

jhodgdon’s picture

RE #85 - did you look at what I actually wrote on the 6/7 module update guide page? I think it's correct there...

jhodgdon’s picture

Status:Needs work» Fixed
pillarsdotnet’s picture

RE #85 - did you look at what I actually wrote on the 6/7 module update guide page? I think it's correct there...

Correct but incomplete.

jhodgdon’s picture

RE #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?

pillarsdotnet’s picture

Here's the only doc page I could find; it's better than nothing: http://drupal.org/node/900794

jhodgdon’s picture

OK, 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.

Status:Fixed» Closed (fixed)
Issue tags:-Favorite-of-Dries, -Needs Update Documentation

Automatically closed -- issue fixed for 2 weeks with no activity.