Updated: Comment #N
Problem/Motivation
1. Documentation in MailFactory references the old class names for devel's MailInterface implementations.
The second problem is that VariableLog is incorrectly named, it no longer uses the variable system.
Proposed resolution
1. Rename the first to Drupal\devel\DevelMailLog. The other one never existed, it was temporarly in the issue that originally added the interface but it got removed and the documentation stayed: #331180: fix pluggable smtp/mail framework. I'd suggest to use the full class name of VariableLog instead, with it's final name.
2. I'd suggest to name the class after what it does and not how. So, MailCollector. Rename class, the file and all references to it.
Remaining tasks
Write patch, review, commit. Does not need tests as there's no functional bug.
User interface changes
None.
API changes
None except someone used VariableLog themself directly, but very unlikely at this point.
Related Issues
Original report
The doxygen of MailFactory::get contains class names which are not PSR-0: DevelMailLog and DrupalDevNullMailSend. The first should be renamed to Drupal\Core\Mail\VariableLog and the second to perhaps Drupal\devel\Mail\Null
as this class doesn't exist.
A followup should be filed to rename VariableLog to MailCollector.
Comment | File | Size | Author |
---|---|---|---|
#21 | rename-mailinterface-implementations-1928440-21.patch | 6.61 KB | ar-jan |
Comments
Comment #1
alexpottThis documentation can be found in \Drupal\Core\Mail\MailFactory
Comment #2
chx CreditAttribution: chx commentedComment #3
chx CreditAttribution: chx commentedYes, but the documentation is now correct.
Comment #4
chx CreditAttribution: chx commentedComment #5
BerdirUpdated issue summary again.
Comment #6
ar-jan CreditAttribution: ar-jan commented- Rename VariableLog to MailCollector
- Update documentation in MailFactory to use PSR-0 names
Comment #7
BerdirLooks good. Can you try to configure git so that it recognices that file rename as such and not a complete remove and adding it again? Makes it easier to review. See https://drupal.org/documentation/git/configure
Comment #8
ar-jan CreditAttribution: ar-jan commentedAh yes, I was wondering why it didn't recognize that since I used git mv. Here it is.
Comment #9
BerdirThe standard of this line changed from that to "Contains \Drupal\..." (with leading backslash). Let's fix that while we touch it, that's what we usually do.
Comment #10
ar-jan CreditAttribution: ar-jan commentedSure.
Comment #11
jhodgdonThis is not really a docs issue... I've looked through the docs changes in the latest patch and they're fine.
Comment #12
BerdirAs are the functional changes. Thanks, looks good IMHO.
Comment #13
Xano10: rename-mailinterface-implementations-1928440-10.patch queued for re-testing.
Comment #14
ar-jan CreditAttribution: ar-jan commented10: rename-mailinterface-implementations-1928440-10.patch queued for re-testing.
Comment #15
webchickNot to get too pedantic about naming, but while this is indescribably a better name than "VariableLog" by virtue of having Mail in it and by virtue of it not actually using variables :), we lost the key thing that tells people what it's doing which is *not* sending the mails. I don't know that PhpMail vs. MailCollector is clear enough in that regard.
I wonder if we want to PhpMail vs. MockMail or something to make this more clear.
Or, maybe NullMail since these examples are no longer equivalent.
Comment #16
BerdirFine with changing the class name to something else.
About 2), note that the old class name *never* existed like that in core, see issue summary proposed solution 1. So I suggested to use a class name that actually does exist and this is the only alternative in core.
Comment #17
webchickYes, I know. What I'm saying is the example was "set contact auto-replies to /dev/null" (which is a realistic use case) and the new one is saying something completely different. That's why I suggested the name NullMail, because then the example's meaning doesn't change, and it's also a mostly-apt description of what's happening (the method body isn't totally empty, but it's definitely not sending mail, either).
Comment #18
ar-jan CreditAttribution: ar-jan commentedHm, some questions before I'll post a next version:
1)
Rename the class: is MockMail a clear enough name for this? How about SimulateMail, which is perhaps more explicit?(Edit: I see Mock... is a generally used term for this).2) Do we need another opinion to choose between a) an example that uses an existing class, or b) a (more) realistic example using /dev/null, for which we don't have an existing class?
If you actually wanted to do b), you wouldn't be using the Drupal\Core\Mail namespace would you? Or is it customary to use this for such examples?
3) Can I just change this to "... captures sent messages to the state system", etc.?
4) Do we also need a name change for the state? Currently it's system.test_email_collector.
Comment #19
ar-jan CreditAttribution: ar-jan commentedThis patch contains the changes suggested by webchick in #15.
Comment #20
Berdir1) Sorry for not responding earlier, but MockMail doesn't seem better to me at all. A mock doesn't do anything on it's own, instead it's something you pre-configure to act/respond accordingly. This has nothing to do with a mock, it's about logging/collecting mails that have been sent, so that a test can then later on access them again.
What about TestMailCollector, to make it clearer? We could also move it to simpletest.module, if nothing else uses it which I assume shouldn't be the case.
2) I'd use a non-existing module namespace, something like \Drupal\example\NullMail ? or my_module, which I often use in change notices.
4. I think this still makes sense, more than the class name, which is why I suggested above to name the class name after the variable and not the other way round :)
Comment #21
ar-jan CreditAttribution: ar-jan commentedThis makes sense to me. Let's see what people think.
(I also changed the state variable from system.test_email_collector to system.test_mail_collector to match the class name).
Comment #22
webchickThat looks like it addresses both of my concerns. The "DevNull" example is kept, just in slightly different format, and TestMailCollector is suitably descriptive. Thanks!
Comment #23
BerdirThanks, I hope that makes it OK to RTBC the patch, I didn't work on it but I did suggest those class names.. Feel free to set if back if you want more opinions.
Comment #24
xjm21: rename-mailinterface-implementations-1928440-21.patch queued for re-testing.
Comment #25
catchCommitted/pushed to 8.x, thanks!