Wrong message displayed after contact form mail(s) cannot be sent

trying...

This is a different approach than the one used in the patch provided by rpayanm.

I think there's a bit of clarification required as to the way to proceed with this - to my understanding, at least the issue should be solved in the \Drupal\contact\MessageForm class (the one that also prints out the success message), which cannot get the error message since \Drupal\contact\MailHandler->sendMailMessages() does not return the message (or any other indication that something went wrong).

In addition, it seems that MailHandler->sendMailMessages() can potentially send up to 3 emails ( requested, copySender and auto reply as documented in the interface). Since an error here would indicate that our mailing system does not accept emails to send, should the process be aborted after the first failure ?

Finally, there's a bit of a naming conflict here - $message, in the scope of \Drupal\contact\MessageForm is an instance of \Drupal\contact\MessageInterface , whereas in scope of \Drupal\contact\MailHandler it is an array of the actual message data.

I've rolled a patch that does the following (in a way that I'm not proud of, please suggest a better way) :
* assigns the return value of MailManager->mail() in MailHandler->sendMailMessages()
* checks the result value on every step of the way, and does not make an attempt to send more emails if the first failed
* passes the final value on to the MessageForm, which then uses it to decide whether to tell the user his mail has been sent.

@rpayanm: in addition to the intended result, you seem to have an unintended change in your patch for the language module.

Hope I'm making some sense, please advise.

@hexblot Nice comment and thank you for say me my error, I mix two patches hehe ;)

I like you patch, you miss change the status to "needs review", for the bots make the dirty work! :D

Greetings.

RTBC +1

Also #2348783: drupal_set_message inherits previous style if set to 'status' is why it is red (reviews needed)

+++ b/core/modules/contact/src/MailHandler.php
@@ -116,18 +116,18 @@ public function sendMailMessages(MessageInterface $message, AccountInterface $se
+    $message_data = $this->mailManager->mail('contact', $key_prefix . '_mail', $to, $recipient_langcode, $params, $sender_cloned->getEmail());
...
+      $message_data = $this->mailManager->mail('contact', $key_prefix . '_copy', $sender_cloned->getEmail(), $current_langcode, $params, $sender_cloned->getEmail());
...
+      $message_data = $this->mailManager->mail('contact', 'page_autoreply', $sender_cloned->getEmail(), $current_langcode, $params);

@@ -144,6 +144,8 @@ public function sendMailMessages(MessageInterface $message, AccountInterface $se
+    return $message_data['result'];

I think we should keep sending even if an earlier message failed and that the return should be keyed by the type. ie. we add three constants to the class for each of the types MESSAGE_RECIPIENT, MESSAGE_COPY, MESSAGE_REPLY and then the return is and array of those results. If two were called, then we return an array with two results, three mails = array with three results and so on.
Then the code in the form just checks over the array to see that $message_results[MailHandler::MESSAGE_RECIPIENT] is TRUE and shows the user the success message in that case.

Also we need tests here to prevent a regression - good news is we have PHPUnit for the MailHandler class so can adapt it. Happy to work on tests - let me know.

Let me try... :)

I got a dude, for example:
I use the contact form and I check "Send yourself a copy", but when Drupal send the message occur this:

• Send email to the recipient(s).....ok
• Send a copy to the user..............fail
• Send an auto-reply......................fail

Drupal show a successful message to the user, but the user never get a copy of this.

Anyway, here the patch you tell me what you think.

Grettings.

1. +++ b/core/modules/contact/src/MailHandler.php
   @@ -144,6 +159,8 @@ public function sendMailMessages(MessageInterface $message, AccountInterface $se
   +    return $message_results[MailHandler::MESSAGE_RECIPIENT]['result'];
   

   I think we should return all of them, it make the mail-handler class more useful for other consumers

2. +++ b/core/modules/contact/src/MessageForm.php
   @@ -188,10 +188,11 @@ public function preview(array $form, FormStateInterface $form_state) {
   +    if($this->mailHandler->sendMailMessages($message, $user)) {
   

   which means we'd need to store and check the response here of course.

Still needs tests

I don't think there's a point in checking individual cases -- this is why:
if you get an error, it is not because the email address was wrong, or something trivial or user dependent - it is because your mail server was unreachable, not accepting mail, or otherwise wrongly configured.

This status is basically inherited from PHP's mail() (in the default case) which reports that the return value is :

Returns TRUE if the mail was successfully accepted for delivery, FALSE otherwise.

That, to my mind, means that the first mail could not be passed on to the mail server for whatever reason, and cannot be a reason that will change in the span of this request. Feel free to correct me if I'm wrong, but even in every implementation I've encountered in D7 (SMTP etc) failure at this level is not recoverable, and state will not change simply because it means that you could not properly talk to the mail server (not that something was wrong with the message).

Currently testing the patch.

I think the #8 hexblot's patch solves the issue in a better way. It seems very unlikely that the mail will fail in a case and not the others, and by using this patch we only have one error message (the #18 create multiple messages for what we think is the same error).

#18 solves the problem when I don't select "send yourself a copy", as I get the error: Unable to send email. Contact the site administrator if the problem persists.
Screenshot: http://cl.ly/image/2f392Z3x0317

When I check the "send yourself a copy" option, I get two errors:

Unable to send email. Contact the site administrator if the problem persists.
Your message copy has been sent.

Screenshot: http://cl.ly/image/2X1O373V3p35

I think the second message should be green, as it's not an error, it's the success message.

So, I mark the issue as needs work.

This is actually two different problems -
* one is with the core Mail class, which prints to the screen messages without being asked for
* the other is with what the contact modules does in ways of error handling.

This issue should focus on the contact module side of things, and here we should print a message about an error that occurred. Ideally, we should have a way to tell the core Mail module to not output on the screen, but this issue is not the place to address it.

In the patch I'm attaching:
* extra constant is removed ( auto reply is never checked )
* try to only have one error / success message (if normal message has issues, stop)
* if the copy was selected and sent, check it and print it individually. Only when the edge case of the normal message being sent and the copy not being sent will you get two error messages, which should be less than one in a million chance.
* fixed the fatal error thrown when an error occurred.

1. +++ b/core/modules/contact/src/MailHandler.php
   @@ -23,6 +23,16 @@ class MailHandler implements MailHandlerInterface {
   +   * Index to identify the result of the message recipient.
   +   */
   +  const MESSAGE_RECIPIENT = 'recipient';
   +
   +  /**
   +   * Index to identify the result of the message copy.
   +   */
   +  const MESSAGE_COPY = 'copy';
   +
   

   we lost auto-reply here

2. +++ b/core/modules/contact/src/MessageForm.php
   @@ -188,10 +188,24 @@ public function preview(array $form, FormStateInterface $form_state) {
   +    if (isset($message_results[MailHandler::MESSAGE_COPY])) {
   +      if (!$message_results[MailHandler::MESSAGE_COPY]['result']) {
   +        drupal_set_message($this->t('Your message copy has been sent.'));
   +      }
   +      else {
   +        drupal_set_message($this->t('Your message copy has not been sent.'), 'error');
   +      }
   +    }
   

   I don't think this is needed

still needs tests

Sorry for the last change.

@larowlan: we lost auto reply because is neither checked or there is much sense in checking it (the current user should not be notified about some other users setting failing to work as expected). If you think it's best to have it there for completeness sake, let me know.

In addition, the second bit of code about the copy is there to catch the case where the normal case was sent but the copy was not (VERY edge case, but still).

@yannisc : As for the multiple error messages - as written in #23, the first message is written by Mail in core, whereas the second is written by Contact module, for which this issue is about. Mentors here are investigating on whether core mail should actually print its own messages on screen, but on either case it's best to have a friendly message related to our specific case for users, not just the system error above ( in my opinion ).

Contact should have a message about failing, regardless of core. It should find a way to tell core Mail to NOT print a message, but that's an issue for core Mail, not the Contact module.

Let me know if we need additional changes to the code.

Re-uploading patch file, seems to not have been sent for testing.

I've tested the patch #29
The result is the same in case the user checks the "send yourself a copy" checkbox, and in case the user don't checks it.
Here is the screenshoot:

I think the initial problem is fixed.

In both case the mail sent to the user failed, so there is and should be an error because when you install Drupal, the recipient for the mail isn't filled by default.
In case both mails (original and copy) failed, there's only one message (no repetition), which I think is a good idea.

For me the #30 is RTBC.

On another subject, we should create an issue about the first message (see #30 "Unable to send email...") which is generated by core (not contact module). @hexblot and me agreed that Core shouldn't print directly this message. I will post another issue about that if needed.

@Nishruu - already posted an issue here

@Nishruu - already posted an issue here

Needs tests
Also the check for copy shouldn't happen unless they tick that box.

@Nishruu, regarding the issue that after Drupal installation finishes, the contact form misses the recipient email, I opened an issue here: #2349651: Default contact form does not send email as email recipient is not set during the installation.

@larowlan: Copy is checked in MailHandler ( uses $message->copySender() which is a boolean ) , and the result value is only set there if it was used . That's what the isset() in MessageForm is about.

Will add tests for all the above cases as well.

Here is a patch of the test file that reproduces this bug

Lets combine #29 and #38. Lets do some manual testing to determine if this issue is reproducable still in 8.0.x. If it is, lets fix the test in #38 so it fails properly without the code in #29 and passes with it.

Fixed the test, so that it now fails on the behavior we want to avoid.

Combined patch from #41 with patch from #29.

The test should now pass. The patch from #41 proves that the this is reproducible, so removed manual test tag, along with other outdated tags.

Testing today here at DrupalConLA
Using an invalid email for the sender this is what I got (screenshot below)

The checkbox for emailing a copy was checked.

This is a confusing user experience. I'm not sure if this relates to having two messages from different systems, or if this is one message for the email being sent, and the other for the message copy being sent to the sender. If the latter, one of the messages should reflect which email did or did not get sent (we were unable to email you a copy of the email).

Patch added in #42 has a lots of improvement. but still there are few thing which I think needs to be fix in case of failure.
Before :

After :

In after screen you see there are two messages which says same statement.

" Unable to send email. Contact the site administrator if the problem persists.
  Your message has not been sent.
"

It should be:

We are unable to send email. Please contact the site administrator if the problem persists.
 

I Removed the custom error message and left a comment, since at some point maybe the mailhandler wont create the error message anymore.

Awesome work @googletorp.
Patch added in #49 works perfactly. Moving it to RTBC with latest output.

1. +++ b/core/modules/contact/src/Tests/NotExistEmailContactTest.php
   @@ -0,0 +1,128 @@
   +   * Tests messages displyed when can't send contact mail.
   

   Nit: s/displyed/displayed

2. +++ b/core/modules/contact/src/Tests/NotExistEmailContactTest.php
   @@ -0,0 +1,128 @@
   +    // Unable to send email message should be displayed, but not the your
   

   Needs rewording.

3. +++ b/core/modules/contact/src/Tests/NotExistEmailContactTest.php
   @@ -0,0 +1,128 @@
   +   * @param string $id
   

   Wouldn't that be an int?

@anavarre, I have fixed issues in updated patch. and Not adding interdiff because just 2 comments line changed.

1. Fixed
2. Removed the lost part of the statement which was not making sence
3. It should be same as it is because $id contains alpha numeric. eg
$this->addContactForm($id = Unicode::strtolower($this->randomMachineName(16)),

Things from #52 has been addressed, think this is RTBC now.

Nice coordination, Credit goes to all.

1. +++ b/core/modules/contact/src/MailHandler.php
   @@ -116,15 +126,15 @@ public function sendMailMessages(MessageInterface $message, AccountInterface $se
          $this->mailManager->mail('contact', 'page_autoreply', $sender_cloned->getEmail(), $current_langcode, $params);
   

   What if this fails? Do we care?

2. +++ b/core/modules/contact/src/MessageForm.php
   @@ -212,10 +212,24 @@ public function validate(array $form, FormStateInterface $form_state) {
   +        drupal_set_message($this->t('Your message copy has not been sent.'), 'error');
   

   Should this be 'Your message copy could not be sent, please contact the administrator if the problem persists.' or similar - for consistency with the standard error message set by MailManager? Can you add a screenshot of this message and tag the issue as 'Needs usability review' so the UX team can sign off - thanks.

3. +++ b/core/modules/contact/src/Tests/NotExistEmailContactTest.php
   @@ -0,0 +1,127 @@
   +/**
   + * Reproduces Contact Form Bug https://www.drupal.org/node/2348119
   

   We don't reference issues in comments (except for todos). Please describe the test instead

4. +++ b/core/modules/contact/src/Tests/NotExistEmailContactTest.php
   @@ -0,0 +1,127 @@
   +  protected $strictConfigSchema = TRUE;
   

   why did we add this, its the default?

5. +++ b/core/modules/contact/src/Tests/NotExistEmailContactTest.php
   @@ -0,0 +1,127 @@
   +    // Create a valid form.
   

   Comment doesn't seem relevant to next line of code?

6. +++ b/core/modules/contact/src/Tests/NotExistEmailContactTest.php
   @@ -0,0 +1,127 @@
   +    $this->addContactForm($id = Unicode::strtolower($this->randomMachineName(16)), $label = $this->randomMachineName(16), "mailnoexist@done1ec.com", '', TRUE);
   

   I think we use drupal.org or example.com for test mail domains

7. +++ b/core/modules/contact/src/Tests/NotExistEmailContactTest.php
   @@ -0,0 +1,127 @@
   +    $this->assertNoText(t('Your message has been sent.'), 'Did not display "Message sent"');
   

   We aren't testing the case where the copy isn't sent.

8. +++ b/core/modules/contact/src/Tests/NotExistEmailContactTest.php
   @@ -0,0 +1,127 @@
   +  function addContactForm($id, $label, $recipients, $reply, $selected, $third_party_settings = []) {
   ...
   +  function submitContact($name, $mail, $subject, $id, $message) {
   

   This looks copied from another test, can we add a new trait instead

BTW, great work on this - looking awesome - thanks

1. We are only catching if PHP can send mail, so both should be the same. To really make a better behavior, we would need to have complete control over the error message when messaging fails, which there is a separate issue for. So I vote for leaving it as it is for now.

7. I don't think we need a test for that. One test for handling message could not be sent, should be enough.

So I addressed the issues form #57 except #1 + #7.

So what I did was

• Improve actual code a bit, to not display error message if copy message fails. Also less if nesting.
• Simplify the test case removing cruft.
• Don't use methods for creating a contact form and sending a mail since we only do it once.
• New array syntax.

So all should be good.

Updated code looks good. Moving this to RTBC after getting the right output as mentioned in #50

So not uploading the screens again and again.

+1 RTBC
Thanks again for your work @RavindraSingh and @googletorp

If you're interested, perhaps we could get a followup to create a ContactTestTrait with convenience methods for adding contact forms and sending messages?

I am not sure about

"create a ContactTestTrait with convenience methods for adding contact forms and sending messages?"

But if you guide us, perhaps we can do that.

Excellent work. Thanks for uploading a test-only patch above, and for the clear screenshots illustrating the bug.

1. +++ b/core/modules/contact/src/MailHandler.php
   @@ -144,6 +154,8 @@ public function sendMailMessages(MessageInterface $message, AccountInterface $se
   +    return $message_results;
   

   If we're adding a return value, we probably need to add docs for it?
    

2. +++ b/core/modules/contact/src/MessageForm.php
   @@ -212,10 +212,19 @@ public function validate(array $form, FormStateInterface $form_state) {
   +    // \Drupal\Core\Mail\MailManager will display an error message, if the
   

   Super minor nit: There is an exta comma before the word "if". (I infer that the comma would be appropriate there in at least German and Dutch based on the contributors who tend to add one, but it's not used there in English.) :)
    

3. +++ b/core/modules/contact/src/Tests/NotExistEmailContactTest.php
   @@ -0,0 +1,76 @@
   +use Drupal\Component\Utility\Unicode;
   +use Drupal\contact\Entity\ContactForm;
   +use Drupal\Core\Mail\MailFormatHelper;
   +use Drupal\field_ui\Tests\FieldUiTestTrait;
   +use Drupal\simpletest\WebTestBase;
   +use Drupal\Core\Entity\EntityTypeInterface;
   

   Several of these use statements are unused; let's clean that up.
    

4. +++ b/core/modules/contact/src/Tests/NotExistEmailContactTest.php
   @@ -0,0 +1,76 @@
   +class NotExistEmailContactTest extends WebTestBase {
   

   Minor: The class name is kind of awkward. Maybe NonexistentEmailContactTest?
    

5. +++ b/core/modules/contact/src/Tests/NotExistEmailContactTest.php
   @@ -0,0 +1,76 @@
   +   * Tests messages displayed when can't send contact mail.
   

   Nit: This is not quite grammatical. I'd suggest: "...when a contact e-mail cannot be sent."
    

6. +++ b/core/modules/contact/src/Tests/NotExistEmailContactTest.php
   @@ -0,0 +1,76 @@
   +    $this->assertText(t('Unable to send email. Contact the site administrator if the problem persists.'), 'Displayed "Unable to send email..."');
   +    $this->assertNoText(t('Your message has been sent.'), 'Did not display "Message sent"');
   

   So, the second assertion is fragile (well assertNoText() in general is). If the text of the message is later changed elsewhere (and after all the summary shows two variants), then the assertion will pass even if there is a regression.

   I'd suggest writing assertions to make sure the first message is the only message displayed. (And then uploading an additional test-only patch to expose the new failures when posting it.)

   Also, minor, but we can remove the optional assertion message text arguments from these as it's redundant. Usually assertText() and assertNoText() have sufficient information in their default messages. (Unlike things like assertEqual() or assertTrue() which give you no clue.)
    

I made a special interdiff for the changes for the test class (since it was renamed, the normal interdiff isn't that useful).

The -TEST patch, is the patch with the test alone.

I should note that I fixed all the comments from @xjm. I changed the test to detect a status message with xpath and ignore the actual content of the message, which should make the test more robust.

Passed from test patch added in #66 Could you please remove

diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
old mode 100644
new mode 100755
diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
old mode 100644
new mode 100755

Code throwing error:

warning: unable to unlink sites/default/default.services.yml: Permission denied
warning: unable to unlink sites/default/default.settings.php: Permission denied
fatal: unable to write file 'sites/default/default.services.yml' mode 100755: Permission denied

Awesome, @googletorp great work. moving this to RTBC

looks like test bot failed one of the daily runs, all should still be good.

+++ b/core/modules/contact/src/MessageForm.php
@@ -212,10 +212,19 @@ public function validate(array $form, FormStateInterface $form_state) {
+    // \Drupal\Core\Mail\MailManager will display an error message if the
+    // message couldn't be sent, so only set a message if it was sent.
+    if (!empty($message_results[MailHandler::MESSAGE_RECIPIENT]['result'])) {
+      drupal_set_message($this->t('Your message has been sent.'));
+    }
+
+    if (!empty($message_results[MailHandler::MESSAGE_COPY]['result'])) {
+      drupal_set_message($this->t('Your message copy has been sent.'));
+    }

It's a bit odd to have two messages as a result of one action. What happens if the second one fails but the first is successful. You'll get message saying "Your message has been sent" - most people would assume that means the copy too.

Maybe I have these reservations because the texts are so similar - the second text could be "A copy has been sent to the sender". But all of this language is really odd... since on the form the label is "Send copy to sender" which really means send a copy to yourself. Apart from the time when you use someone else's email address... so it's not your address... so is it your message?!?! Who knows?

Setting back to needs review to get more feedback.

Hmm.

Part of out problems here is out of scope for this. Right now when a message failed to be sent, we get a standard message error message. Ideally we would like something like this to be possible:

• Failed to send the message
• The copy message has been sent to the sender

and

• Your message has been sent
• Failed to send the copy message to the sender

But we can't really do that right now, so your first case about the copy message failing is hopeless bad right now, since we would first display the generic failed to send email, followed by the "your message has been sent" message, both being correct. We can't really improve this until #2349725: Core Mail class prints non-overridable errors on the page has been resolved.

I'll leave it to the native English speaking people to improve the message about the copy in it needs improving, but I'm in favor of committing this as is instead of wasting more time bike-shedding. Texts can be improved later on if needed.

I think better to split the problems
1) fix mailing system to return status of mail sending/queueing - #2349725: Core Mail class prints non-overridable errors on the page
2) this issue should be about usability of text messages

So for now, this is a good as we can get, until #2349725: Core Mail class prints non-overridable errors on the page lands, putting back to RTBC unless Alex Pott disagrees with this.

This should be postponed on #2349725: Core Mail class prints non-overridable errors on the page

I have not triaged this issue but it is no longer postponed.

Is this still a problem?

Yes, at least in the sense that the Contact module is not checking if the result of sending the mail is successful or not in sendMailMessages() - it just assumes it worked.

This came up for me because I am using hook_mail_alter() to prevent sending mail to the addresses of blocked users.