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.

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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Component: system.module » documentation
Issue tags: -Configuration system +Novice

This documentation can be found in \Drupal\Core\Mail\MailFactory

chx’s picture

Issue summary: View changes
chx’s picture

Status: Active » Fixed

Yes, but the documentation is now correct.

chx’s picture

Issue summary: View changes
Status: Fixed » Active
Berdir’s picture

Title: Update mail interfaces » Rename incorrectly named MailInterface implementations
Issue summary: View changes

Updated issue summary again.

ar-jan’s picture

Status: Active » Needs review
FileSize
5.09 KB

- Rename VariableLog to MailCollector
- Update documentation in MailFactory to use PSR-0 names

Berdir’s picture

Looks 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

ar-jan’s picture

Ah yes, I was wondering why it didn't recognize that since I used git mv. Here it is.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Mail/MailCollector.php
@@ -2,7 +2,7 @@
 /**
  * @file
- * Definition of Drupal\Core\Mail\VariableLog.
+ * Definition of Drupal\Core\Mail\MailCollector.
  */

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

ar-jan’s picture

jhodgdon’s picture

Component: documentation » mail system

This is not really a docs issue... I've looked through the docs changes in the latest patch and they're fine.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

As are the functional changes. Thanks, looks good IMHO.

Xano’s picture

ar-jan’s picture

webchick’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Mail/MailCollector.php
    @@ -2,7 +2,7 @@
    - * Definition of Drupal\Core\Mail\VariableLog.
    + * Contains \Drupal\Core\Mail\MailCollector.
    

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

  2. +++ b/core/lib/Drupal/Core/Mail/MailFactory.php
    @@ -72,8 +72,8 @@ public function __construct(ConfigFactory $configFactory) {
    -   *   'contact_page_autoreply' => 'DrupalDevNullMailSend',
    ...
    +   *   'contact_page_autoreply' => 'Drupal\Core\Mail\MailCollector',
    

    Or, maybe NullMail since these examples are no longer equivalent.

  3. Also, if we're changing the class, we ought to update the docs to be consistent with it (e.g. the PHPDoc above the class and the mail() function still mention variables).
Berdir’s picture

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

webchick’s picture

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

ar-jan’s picture

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

ar-jan’s picture

This patch contains the changes suggested by webchick in #15.

Berdir’s picture

1) 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 :)

ar-jan’s picture

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

webchick’s picture

That looks like it addresses both of my concerns. The "DevNull" example is kept, just in slightly different format, and TestMailCollector is suitably descriptive. Thanks!

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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

xjm’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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