Problem/Motivation

1. Create a contact form, hide the default e-mail field. Configure an auto-reply response.
2. Send a message as anonymous user.
3. you get an error message about failed to send e-mail.

I had to debug this *twice* in the last two weeks on different client projects ;)

Proposed resolution

Check that there is an e-mail to send the auto-reply. If not, skip mail and log a *slient* error that tells you what wrong.

Remaining tasks

User interface changes

API changes

Data model changes

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#30 auto-reply.png4.09 KBsunil.yadav
#22 interdiff-2814141-20-22.txt180 bytestbonomelli
#22 auto_reply_mail-2814141-22.patch3.9 KBtbonomelli
#20 interdiff-2814141-16-20.txt1.63 KBtbonomelli
#20 auto_reply_mail-2814141-20.patch4.13 KBtbonomelli
#16 interdiff-2814141-14-16.txt1.65 KBtbonomelli
#16 auto_reply_mail-2814141-16.patch3.89 KBtbonomelli
#14 interdiff-2814141-12-14.txt2.09 KBtbonomelli
#14 auto_reply_mail-2814141-14.patch3.39 KBtbonomelli
#12 interdiff-2814141-9-12.txt3.45 KBtbonomelli
#12 auto_reply_mail-2814141-12.patch3.84 KBtbonomelli
#9 auto_reply_mail-2814141-9.patch2.98 KBtbonomelli
#9 interdiff-2814141-7-9.txt3.35 KBtbonomelli
#7 auto_reply_mail-2814141-7.patch2.4 KBtbonomelli
#7 interdiff-2814141-6-7.txt1.39 KBtbonomelli
#6 interdiff-2814141-4-6.txt941 bytestbonomelli
#6 auto_reply_mail-2814141-6.patch1.02 KBtbonomelli
#4 auto_reply_mail-2814141-4.patch1.04 KBtbonomelli

Comments

Berdir created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Needs tests
tbonomelli’s picture

tbonomelli commented
Assigned: Unassigned » tbonomelli
tbonomelli’s picture

tbonomelli commented
Status: Active » Needs review
StatusFileSize
new auto_reply_mail-2814141-4.patch1.04 KB

Checks if there is an E-Mail to send, if not it skips sending the mail.
Still need to work on it.

ginovski’s picture

ginovski commented
Status: Needs review » Needs work 
+++ b/core/modules/contact/src/MailHandler.php
@@ -122,7 +122,14 @@ public function sendMailMessages(MessageInterface $message, AccountInterface $se
+        $this->logger->error('sending autoreply failed because of missing mailadress from sender in %contact_form', array(

1. Try something like: 'Error sending autoreply, missing sender mail address in %contact_form'
2. Use [] instead of array()

tbonomelli’s picture

tbonomelli commented
Status: Needs work » Needs review
StatusFileSize
new auto_reply_mail-2814141-6.patch1.02 KB
new interdiff-2814141-4-6.txt941 bytes

Changed the errormessage and used [] instead of array().

tbonomelli’s picture

tbonomelli commented
StatusFileSize
new interdiff-2814141-6-7.txt1.39 KB
new auto_reply_mail-2814141-7.patch2.4 KB

Started writing a test-case for this, still need to figure out how to assert the error log.

Status: Needs review » Needs work

The last submitted patch, 7: auto_reply_mail-2814141-7.patch, failed testing.

tbonomelli’s picture

tbonomelli commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2814141-7-9.txt3.35 KB
new auto_reply_mail-2814141-9.patch2.98 KB

Added the test, still fails though because the error doesn't seem to get logged.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/contact/src/MailHandler.php
@@ -122,7 +122,14 @@ public function sendMailMessages(MessageInterface $message, AccountInterface $se
+      if(!$sender_cloned->getEmail() || $sender_cloned->getEmail() == '') {

    the first check covers the second as well, so just ! is enough.

    also, coding standards. 

  2. +++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
@@ -384,6 +391,17 @@ function testAutoReply() {
+    $this->assertErrorLogged($this->expectedExceptionMessage);

    this is about php fatal errors, not related to what we do.

    You need to enable the watchdog module and then check on admin/reports/dblog for example.

    And you also need to check that you do not see the current error message with assert no text.

The last submitted patch, 9: auto_reply_mail-2814141-9.patch, failed testing.

tbonomelli’s picture

tbonomelli commented
Status: Needs work » Needs review
StatusFileSize
new auto_reply_mail-2814141-12.patch3.84 KB
new interdiff-2814141-9-12.txt3.45 KB

Enabled dblog and added an assertion of the current error text.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
@@ -40,6 +40,13 @@ protected function setUp() {
   /**
+   * Exceptions thrown by site under test that contain this text are ignored.
+   *
+   * @var string
+   */
+  protected $expectedExceptionMessage;
+
+  /**

    this is not an exception message. jut directly look for that text with $this->assertNoText('...')

    and remove the property. 

  2. +++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
@@ -384,6 +397,21 @@ function testAutoReply() {
+    // Verify that the current error message doesn't show, that the auto-reply doesn't get sent and the correct silent error gets logged.

    comments must not be > 80 characters per line. 

  3. +++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
@@ -384,6 +397,21 @@ function testAutoReply() {
+    $this->expectedExceptionMessage = 'Error sending autoreply, missing sender mail-address in foo';
+    $this->assertRaw($this->expectedExceptionMessage);

    same here.

tbonomelli’s picture

tbonomelli commented
Status: Needs work » Needs review
StatusFileSize
new auto_reply_mail-2814141-14.patch3.39 KB
new interdiff-2814141-12-14.txt2.09 KB

Removed the property and adjusted the comment.

tduong’s picture

tduong commented
Status: Needs review » Needs work
Issue tags: -Needs tests 
  1. +++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
@@ -27,7 +27,7 @@ class ContactSitewideTest extends WebTestBase {
+  public static $modules = array('text', 'contact', 'field_ui', 'contact_test', 'block', 'error_service_test', 'dblog');

@@ -346,7 +346,13 @@ function testSiteWideContact() {
+    $admin_user = $this->drupalCreateUser(array(

    Since you are touching these lines, change the array syntax as well.

  2. +++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
@@ -384,6 +390,20 @@ function testAutoReply() {
+    entity_get_form_display('contact_message', 'foo', 'default')

    Let's use non deprecated functions, use instead something like:

    $form_display = \Drupal::entityTypeManager()->getStorage('entity_form_display')->load('contact_message.foo.default');
tbonomelli’s picture

tbonomelli commented
StatusFileSize
new auto_reply_mail-2814141-16.patch3.89 KB
new interdiff-2814141-14-16.txt1.65 KB

Changed the array syntax
Didn't apply $form_display = \Drupal::entityTypeManager()->getStorage('entity_form_display')->load('contact_message.foo.default'); because it returns NULL.

tbonomelli’s picture

tbonomelli commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: auto_reply_mail-2814141-16.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I think mail-address isn't how that's written. Use e-mail address instead. However, the UI uses Auto-reply, so lets use auto-reply in the error message too.

About the deprecated function, just opened #2818227: Undeprecate entity_get_(form_)display() or offer a real replacement. Keep it like it is for now.

tbonomelli’s picture

tbonomelli commented
Status: Needs work » Needs review
StatusFileSize
new auto_reply_mail-2814141-20.patch4.13 KB
new interdiff-2814141-16-20.txt1.63 KB

Applied changes to the error messages.

Status: Needs review » Needs work

The last submitted patch, 20: auto_reply_mail-2814141-20.patch, failed testing.

tbonomelli’s picture

tbonomelli commented
Status: Needs work » Needs review
StatusFileSize
new auto_reply_mail-2814141-22.patch3.9 KB
new interdiff-2814141-20-22.txt180 bytes

Removed an unrelated change from the patch.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Assigned: tbonomelli » Unassigned
Status: Needs review » Reviewed & tested by the community

This looks good to me. We no longer show an error to the user and instead log it, with pretty clear information on why we can't send the auto-reply.

I think that's the best we can do and should help users who have this problem in the future.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

+1 RTBC, great work folks

  • catch committed aae2969 on 8.3.x 
    Issue #2814141 by tbonomelli, Berdir, Ginovski, tduong: Auto-reply mail...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

  • catch committed c9f0b9c on 8.2.x 
    Issue #2814141 by tbonomelli, Berdir, Ginovski, tduong: Auto-reply mail...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

sunil.yadav’s picture

sunil.yadav commented
StatusFileSize
new auto-reply.png4.09 KB

Hi I have update drupal version from 8.3 to drupal-8.8.5.
For contact for as anonymous user i am getting followig error

Error sending auto-reply, missing sender e-mail address…

Please suggest me what i am doing wrong.

Auto reply email is not working