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.
Problem/Motivation
We're calling out to procedural code from OO stuff to use drupal_mail, eg Drupal\contact\MessageForm.
Proposed resolution
Move drupal_mail to MailManager::mail
Move all the other mail.inc stuff to a new \Drupal\Core\Utility\Mail class.
We can't make it a component because it needs Settings and references base_url global.
Remaining tasks
Fix any failing tests
Reviews
User interface changes
None
API changes
- drupal_mail() » \Drupal::service('plugin.manager.mail')->mail()
- drupal_mail_system() » \Drupal::service('plugin.manager.mail')->getInstance()
- drupal_wrap_mail() » \Drupal\Core\Utility\Mail::wrapMail()
- drupal_html_to_text() » \Drupal\Core\Utility\Mail::htmlToText()
- _drupal_wrap_mail_line() » \Drupal\Core\Utility\Mail::wrapMailLine()
- _drupal_html_to_mail_urls() » \Drupal\Core\Utility\Mail::htmlToMailUrls()
- _drupal_html_to_text_clean() » \Drupal\Core\Utility\Mail::htmlToTextClean()
- _drupal_html_to_text_pad() » \Drupal\Core\Utility\Mail::htmlToTextPad()
Comment | File | Size | Author |
---|---|---|---|
#44 | interdiff.txt | 6.38 KB | kim.pepper |
#44 | 2301393-mail-inc-44.patch | 45.58 KB | kim.pepper |
#41 | 2301393-mail-inc-41.patch | 45.56 KB | kim.pepper |
#37 | interdiff.txt | 3.45 KB | kim.pepper |
#37 | 2301393-mail-inc-37.patch | 45.54 KB | kim.pepper |
Comments
Comment #2
larowlanFixes test
Comment #3
BerdirNice.
While moving this over, can we also already convert WrapMailUnitTest to a phpunit unit test or should we do that in a follow-up? Seems like it should be trivial now.
Comment #4
andypostSuppose we need
\Drupal::mail()
wrapper fordrupal_mail()
is this needed?
Comment #5
ParisLiakos CreditAttribution: ParisLiakos commentedwhy not in
Drupal\Core\Mail
namespace?and maybe MailFormatHelper.
It is not a general Utility class, it is very focused on formatting email text
Comment #6
larowlanFixes 3,4 and 5
Comment #7
larowlanRemoves \Drupal::mail at request of @Crell and @sun on irc
Comment #9
Crell CreditAttribution: Crell commentedDrupal::mail() is unnecessary. It's very much not an "every day thing that procedural code needs" (which is the only reason to put something in Drupal::)
This should be an instantiated object, not a bunch of functions with colons in them. :-) All-static classes are a very bad code smell.
Comment #10
sunThanks for dropping the
\Drupal::mail()
method. Yes, sending mails isn't exactly >80% material in my book.A possible addition may be discussed in a separate followup issue. But normally, I'd expect consuming code to properly inject this stuff.
Comment #12
larowlanWe should create a mail value object, refactor the ::mail method to use that.
Keep array access for bc $message['subject'] and move all of the MailFormatHelper stuff to methods on the mail object.
Comment #13
larowlanFixes test, will work on #12 at weekend
Comment #14
ParisLiakos CreditAttribution: ParisLiakos commentedRe #12 Why should a value object contain logic about its formatting? Crell is right, also making it a service would allow one to easily override it..i am pretty sure there are already cases in contrib that could take advantage of it.
The value object then could contain format() methods that proxy calls to this service for DX, but i really think that this should be a nice followup
Comment #15
larowlanok so MailFormatHelper becomes a service.
So approach will be like so:
Also any objections to just killing off the four functions with _ in front of them in mail.inc _ means internal, they're mostly preg_replace and array_map callbacks, could all go the way of closures to be honest.
Just wanting to get some consensus before I start coding.
Comment #16
larowlanActually, any reason that MailFormatHelper methods can't just live on the MailManager service?
Seems like that would fit within its responsibilities.
Comment #17
sunA mail subsystem is composed of discrete components:
hook_mail_alter()
)drupal_html_to_text()
)Merging formatting into the manager would break a clean separation of concerns.
MailFormatHelper
is essentially aTextMailMessageFormatter
(Content-Type: text/plain
).FWIW, I'd recommend to not over-engineer this. If that's the goal, then we should drop our code entirely and adopt one of the real PHP mail libraries/packages instead (e.g., SwiftMailer, ZendMailer,
PHPMailer, etc). (which we should have done a decade ago already)Comment #18
larowlanso #15 sounds good and #16 is out?
Comment #19
andypostRe #17
1) There's
Message
in contact module so current stateless definition could be moved to system or Entity - needs some clean-up2) entity view modes as we have for rss and search* - done
3) Formaters and filters - done
4)
drupal_mail()
=> service - tbdComment #20
andypostprobably that means to create a kind of serialization and templating to support special view mode for content entity at least
Comment #21
Crell CreditAttribution: Crell commentedI know we're very late in the cycle, but sun has a point in #17. If we need a Robust Mail Handling System(tm), aren't there several already that we could composer install and call it a day? (That's what we did for RSS/Atom rather than trying to rewrite those parsers ourselves.)
Comment #22
BerdirCan we limit this issue to what we have? Yes, static methods are not that great but this seems to be in line with other utility classes that we added like Xss, String and Unicode.
The class is not a formatter. Just a bunch of helper methods for formatting plain-text mails. We already have format() on MailInterface and while that should be a separate plugin, it currently is not, so that is the formatter. (Mailsystem 8.x in contrib solves this problem by extending the MailManager and an adapter plugin that can forward format() and send() to different classes).
And replacing the whole mail system is really not in the scope of this issue...
Comment #23
larowlanIn which case #13 is the patch
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedI am fine with going on with #13 for now
Lets completely get rid of those. Underscore means its private and shouldnt been used directly.
Which would mean you could convert some of those to be anonymous functions like you proposed before
Since we change it lets make it Contains
Lets get rid "Unit" from its name. Maybe also rename it to
MailFormatHelperTest
and this method testWrapMail
Comment #25
dawehner.
Comment #26
kim.pepperRe-roll of #13 plus fixes for #24.
Comment #28
kim.pepperLooks like I missed the part about "Which would mean you could convert some of those to be anonymous functions like you proposed before". I just removed them. :-(
Comment #29
kim.pepperMissed a call to _drupal_html_to_mail_urls in a preg_replace_callback().
Comment #30
dawehnerIs there a reason we cannot use static::?
I guess we want an @code wrap here.
Do we have a follow up to use the request context instead?
Personally I try to avoid calling anything in the constructor , at least the config factory should be avoided as this could trigger everything, so services which potentially send mails will fetch the system.site config
Can we get rid of the silly spaces?
Let's try to use the StringTranslationTrait instead ...
Can we update this documentation to not refer to drupal_mail any longer?
nice!
Comment #31
kim.pepperThanks for the review @dawehner!
Comment #32
dawehnerBetter than nothing.
Comment #33
kim.pepperFair enough.
Comment #34
dawehnerThank you. ( we should write a change record )
Comment #35
kim.pepperAdded draft change record https://www.drupal.org/node/2309379
Comment #36
alexpottLet's make these protected. These are replacing _drupal functions and their only usages are in this class. Less public facing API to maintain. And yes this will work with the callbacks... http://3v4l.org/2XKJo
Can we add a property on the class for this or not use
$this->
Comment #37
kim.pepperFixes for #36.
Also added a property for $this->mailManager which was being set dynamically.
Also, minor code style cleanups.
Comment #40
kim.pepperComment #41
kim.pepperRe-roll
Comment #42
dawehnerIt is a little bit sad that like 95% of that functionality is generic enough to be reused for example in a CLI environment, so we maybe could work on a follow up to provide a more reusable component.
Should we refer to the new code now?
Oh wow, I thought we would have had a generic solution for that already but it doesn't seem to be the case. Would be worth as a follow up in general, as it is quite a DX improvement.
Follow up: Allow to use somehow callables here instead.
Yeah fine so far, but this line does not tell me what the main manager actually does
Just in case you have nothing to do you could wrap them with higher density, so that it is more looking like full justification
Comment #43
Berdir4. #1975136: Token::replace() should accept any type of callable, see last comments, $function() actually works for arrays in PHP 5.4+. Happy to see you didn't know this either :)
Comment #44
kim.pepperComment #45
chx CreditAttribution: chx commentedsrsly? Yes, followup, yadda, yadda, but, rly?
Comment #46
dawehnerWhat I said! berdir just pointed out that it theoretically supports callbacks if you use $foo(), we need still a proper way in the future.
On top of that it seems to be that it makes kinda sense to put a the mail manager onto ControllerBase as well as FormBase and maybe \Drupal
Comment #47
Crell CreditAttribution: Crell commentedSending mail is a very rare operation. There's no need to put that on the base classes. It's easy enough to inject when needed.
Comment #48
alexpottCommitted 6f476b8 and pushed to 8.0.x. Thanks!
Comment #50
larowlanPublished change notice
Comment #51
Sutharsan CreditAttribution: Sutharsan commentedCreated follow-up #2325955: Refactor MailFormatHelper to allow injection and testing. for the use of globals in a static method.