Abstract contact module mail delivery out of form into a service

I'll try to write some phpunit test for this.

+++ b/core/modules/contact/contact.services.yml
@@ -4,3 +4,7 @@ services:
+    arguments: ['@plugin.manager.mail', '@language_manager', '@logger.factory', '@string_translation']

+++ b/core/modules/contact/src/MailHandler.php
@@ -0,0 +1,135 @@
+  use StringTranslationTrait;
...
+    $this->stringTranslation = $string_translation;

This trait uses setter injection
so do not use argument.
Should be something like
calls:
- [setStringTranslation, ['@string_translation']]

I really like it!

+++ b/core/modules/contact/contact.services.yml
@@ -4,3 +4,7 @@ services:
+
+  contact.mail_handler:
+    class: Drupal\contact\MailHandler
+    arguments: ['@plugin.manager.mail', '@language_manager', '@logger.factory', '@string_translation']

+1 for using the argument, even if it uses the trait.

+++ b/core/modules/contact/src/MessageForm.php
@@ -180,77 +191,12 @@ public function preview(array $form, FormStateInterface $form_state) {
     $sender = clone $this->entityManager->getStorage('user')->load($user->id());

Does anyone know why we clone the user?

Here we go added test haven't fixed #8 or #6
This is blocked on #2030591: Expand ContactForm with methods.
Thank you very much @larowlan with all the phpunit help and fixes.

Reroll after #2030591: Expand ContactForm with methods and fixed #6 and #8.1
@dawehner I have no idea about #8.2

This patch looks really great!

1. +++ b/core/modules/contact/contact.services.yml
   @@ -4,3 +4,9 @@ services:
   +    calls:
   +      - [setStringTranslation, ['@string_translation']]
   
   +++ b/core/modules/contact/tests/src/MailHandlerTest.php
   @@ -0,0 +1,384 @@
   +    $this->contactMailHandler = new MailHandler($this->mailManager, $this->languageManager, $this->loggerFactory);
   +    $this->contactMailHandler->setStringTranslation($string_translation);
   

   Is there a reason we don't inject via constructor injection. Just wondering ...

2. +++ b/core/modules/contact/src/MailHandler.php
   @@ -0,0 +1,133 @@
   +
   +    if (!$message->isPersonal()) {
   +      $this->logger->notice('%sender-name (@sender-from) sent an email regarding %contact_form.', array(
   +        '%sender-name' => $sender->getUsername(),
   +        '@sender-from' => $sender->getEmail(),
   +        '%contact_form' => $contact_form->label(),
   +      ));
   +    }
   +    else {
   +      $this->logger->notice('%sender-name (@sender-from) sent %recipient-name an email.', array(
   +        '%sender-name' => $sender->getUsername(),
   +        '@sender-from' => $sender->getEmail(),
   +        '%recipient-name' => $message->getPersonalRecipient()->getUsername(),
   +      ));
   +    }
   

   Not for this issue but in general for example here, the logging could be moved into a decorator. This allows the actual main class to be more thin.

3. +++ b/core/modules/contact/src/MailHandlerInterface.php
   @@ -0,0 +1,36 @@
   +
   +
   

   one empty line too much :(

4. +++ b/core/modules/contact/src/MessageForm.php
   @@ -175,76 +186,12 @@ public function preview(array $form, FormStateInterface $form_state) {
   +    $this->mailHandler->sendMailMessages($message, $sender);
    
        $this->flood->register('contact', $this->config('contact.settings')->get('flood.interval'));
   

   Did you considered to use the flood control into the service as well?

5. +++ b/core/modules/contact/tests/src/MailHandlerTest.php
   @@ -0,0 +1,384 @@
   +  /**
   +   * Logger service.
   +   *
   +   * @var \Drupal\Core\Logger\LoggerChannelFactoryInterface|\PHPUnit_Framework_MockObject_MockObject
   +   */
   +  protected $loggerFactory;
   +
   +  /**
   +   * Logger service.
   +   *
   +   * @var \Drupal\Core\Logger\LoggerChannelInterface|\PHPUnit_Framework_MockObject_MockObject
   ...
   +
   

   nitpick: It can't be twice the same logger service

6. +++ b/core/modules/contact/tests/src/MailHandlerTest.php
   @@ -0,0 +1,384 @@
   +  public function testSendMailMessages(MessageInterface $message, AccountInterface $sender, $results) {
   +    $this->logger->expects($this->once())
   +      ->method('notice');
   +    $this->mailManager->expects($this->any())
   +      ->method('mail')
   +      ->willReturnCallback(
   +        function($module, $key, $to, $langcode, $params, $from) use (&$results) {
   +          $result = array_shift($results);
   +          $this->assertEquals($module, $result['module']);
   +          $this->assertEquals($key, $result['key']);
   +          $this->assertEquals($to, $result['to']);
   +          $this->assertEquals($langcode, $result['langcode']);
   +          $this->assertArrayEquals($params, $result['params']);
   +          $this->assertEquals($from, $result['from']);
   +        });
   +    $this->contactMailHandler->sendMailMessages($message, $sender);
   +  }
   

   wow!

7. +++ b/core/modules/contact/tests/src/MailHandlerTest.php
   @@ -0,0 +1,384 @@
   +  protected function getAnonymousMockMessage($recipients, $auto_reply, $copy_sender = FALSE) {
   ...
   +  protected function getAuthenticatedMockMessage($copy_sender = FALSE) {
   

   I really like this new mock methods.

8. +++ b/core/modules/contact/tests/src/MailHandlerTest.php
   @@ -0,0 +1,384 @@
   +    $message->expects($this->exactly(2))
   +      ->method('getSenderName')
   +      ->willReturn('Anonymous');
   ...
   +    $message->expects($this->exactly(4))
   +      ->method('isPersonal')
   +      ->willReturn(FALSE);
   ...
   +    $message->expects($this->exactly(4))
   +      ->method('isPersonal')
   +      ->willReturn(TRUE);
   

   I wonder whether this is too specific and would limit us from future refactoring.

For #12.1 see #8.1 :D
#8.2 Nice idea thank you.
#8.3 I'll fix it.
#8.4 Can we move it to follow up?
#8.5 I'll fix it.
#8.6 Kudos to @larowlan.
#8.7 Thanks.
#8.8 I think so to I'll change them to any.

not #8 it is #12.

1. +++ b/core/modules/contact/contact.services.yml
   @@ -4,3 +4,9 @@ services:
   +    arguments: ['@plugin.manager.mail', '@language_manager', '@logger.factory']
   
   +++ b/core/modules/contact/src/MailHandler.php
   @@ -0,0 +1,133 @@
   +    $this->logger = $logger_factory->get('contact');
   

   All regular services (except plugin.manager.mail!) define a service for their logger channel. It might worth doing here.

2. +++ b/core/modules/contact/contact.services.yml
   @@ -4,3 +4,9 @@ services:
   +    calls:
   +      - [setStringTranslation, ['@string_translation']]
   

   We don't use this for any services, usually opting for constructor injection alongside the trait.

3. +++ b/core/modules/contact/src/MailHandler.php
   @@ -0,0 +1,133 @@
   +      throw new \RuntimeException($this->t('Unable to determine message recipient.'));
   

   Do not translate exception messages, and do not use full stops in them.

   Also, is there any reason to not define a custom exception class?

4. +++ b/core/modules/contact/tests/src/MailHandlerTest.php
   @@ -0,0 +1,384 @@
   +  public function testInvalidRecipient() {
   

   What does this cover?

5. +++ b/core/modules/contact/tests/src/MailHandlerTest.php
   @@ -0,0 +1,384 @@
   +   * @covers ::sendMailMessages()
   

   Please don't use () on @covers
   EDIT: opened #2328919: Remove () from a bunch of @covers definitions in PHPUnit

That was some review thanks for the very nice review.

meh

+1 rtbc

Yep, looks good to me. Great work @jibran and @larowlan!

#8.2/#11: Could that $sender = clone ... also move to MailHandler::sendMailMessages() with the lines that override name/email?

I agree with @olli in #21 I think the clone and user load should go into the MailManager since this is where the properties on the anonymous user get set.

OTOH:

+++ b/core/modules/contact/src/MailHandler.php
@@ -0,0 +1,136 @@
+  public function sendMailMessages(MessageInterface $message, AccountInterface $sender) {

If we leave the load in MessageForm should this be UserInterface or AccountInterface?

I agree with @olli in #21 I think the clone and user load should go into the MailManager since this is where the properties on the anonymous user get set.

OTOH:

+++ b/core/modules/contact/src/MailHandler.php
@@ -0,0 +1,136 @@
+  public function sendMailMessages(MessageInterface $message, AccountInterface $sender) {

If we leave the load in MessageForm should this be UserInterface or AccountInterface?

+++ b/core/modules/contact/tests/src/Unit/MailHandlerTest.php
@@ -0,0 +1,375 @@
+    $this->contactMailHandler->setStringTranslation($string_translation);

Not needed

I made the changes but didn't fix the test yet. If the changes are good then I can fix the test.

we're limiting our options here.

It's a valid point. Service becomes useless with this change.
Please elaborate how you want to proceed here.

Looks rtbc

+++ b/core/modules/contact/src/MailHandler.php
@@ -53,37 +61,42 @@ class MailHandler implements MailHandlerInterface {
   public function sendMailMessages(MessageInterface $message, AccountInterface $sender) {
+    // Clone the sender, as we make changes to mail and name properties.
+    $sender_cloned = clone $this->userStorage->load($sender->id());

I think better to leave $account as argument name and $sender as variable

+++ b/core/modules/contact/tests/src/Unit/MailHandlerTest.php
@@ -97,6 +116,13 @@ public function testInvalidRecipient() {
+    // User IDs 1 and 0 have special implications, use 3 instead.

why not 2?

But anyways i think @alexpott points are addressed so back to RTBC

Committed e5921e7 and pushed to 8.0.x. Thanks!