Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
After submitting a contact form the message "Your message has been sent." is shown to the user even in the case that the emails were failed to send. The correct error message about the failure of sending the mail itself appears separately.
Proposed resolution
Make the message conditional so that it displays an error in case the mail is not sent.
In case the user checks the "send yourself a copy" checkbox:
In case the user doesn't check the "send yourself a copy" checkbox:
Remaining tasks
Everything.
User interface changes
Message string will change.
Comment | File | Size | Author |
---|---|---|---|
#90 | 2348119-90.patch | 4.15 KB | casey |
#88 | 2348119-88.patch | 4.17 KB | casey |
#71 | wrong_message_displayed-2348119-71.patch | 7.11 KB | googletorp |
Comments
Comment #1
kugta CreditAttribution: kugta commentedComment #2
batigolixComment #3
batigolixComment #4
batigolixComment #5
batigolixComment #6
rpayanmtrying...
Comment #8
hexblot CreditAttribution: hexblot commentedThis 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.
Comment #9
rpayanm@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.
Comment #10
rpayanmRTBC +1
Comment #11
larowlanAlso #2348783: drupal_set_message inherits previous style if set to 'status' is why it is red (reviews needed)
Comment #12
larowlanI 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.
Comment #13
larowlanComment #14
rpayanmLet me try... :)
Comment #15
rpayanmI 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:
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.
Comment #16
larowlanI think we should return all of them, it make the mail-handler class more useful for other consumers
which means we'd need to store and check the response here of course.
Still needs tests
Comment #17
hexblot CreditAttribution: hexblot commentedI 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 :
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).
Comment #18
rpayanmComment #19
Nishruu CreditAttribution: Nishruu commentedCurrently testing the patch.
Comment #20
Nishruu CreditAttribution: Nishruu commentedI 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).
Comment #21
yannisc CreditAttribution: yannisc commented#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.
Comment #22
kugta CreditAttribution: kugta commented@yannisc, the success message not being green is a different issue here: drupal_set_message inherits previous style if set to 'status'
Comment #23
hexblot CreditAttribution: hexblot commentedThis 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.
Comment #24
larowlanwe lost auto-reply here
I don't think this is needed
still needs tests
Comment #25
yannisc CreditAttribution: yannisc commentedComment #26
yannisc CreditAttribution: yannisc commentedSorry for the last change.
Comment #27
yannisc CreditAttribution: yannisc commentedComment #28
hexblot CreditAttribution: hexblot commented@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.
Comment #29
hexblot CreditAttribution: hexblot commentedRe-uploading patch file, seems to not have been sent for testing.
Comment #30
kulcsi CreditAttribution: kulcsi commentedI'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.
Comment #31
Nishruu CreditAttribution: Nishruu commentedIn 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.
Comment #32
hexblot CreditAttribution: hexblot commented@Nishruu - already posted an issue here
Comment #33
hexblot CreditAttribution: hexblot commented@Nishruu - already posted an issue here
Comment #34
batigolixComment #35
larowlanNeeds tests
Also the check for copy shouldn't happen unless they tick that box.
Comment #36
yannisc CreditAttribution: yannisc commented@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.
Comment #37
hexblot CreditAttribution: hexblot commented@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.
Comment #38
subins2000 CreditAttribution: subins2000 commentedHere is a patch of the test file that reproduces this bug
Comment #39
larowlanComment #40
disasm CreditAttribution: disasm at AppliedTrust commentedLets 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.
Comment #41
googletorp CreditAttribution: googletorp as a volunteer commentedFixed the test, so that it now fails on the behavior we want to avoid.
Comment #42
googletorp CreditAttribution: googletorp as a volunteer commentedCombined 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.
Comment #46
adriancotter CreditAttribution: adriancotter commentedTesting 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).
Comment #47
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedPatch 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.
It should be:
Comment #48
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedComment #49
googletorp CreditAttribution: googletorp as a volunteer commentedI Removed the custom error message and left a comment, since at some point maybe the mailhandler wont create the error message anymore.
Comment #50
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedAwesome work @googletorp.
Patch added in #49 works perfactly. Moving it to RTBC with latest output.
Comment #51
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedComment #52
anavarreNit: s/displyed/displayed
Needs rewording.
Wouldn't that be an int?
Comment #53
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commented@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)),
Comment #54
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedComment #55
googletorp CreditAttribution: googletorp as a volunteer commentedThings from #52 has been addressed, think this is RTBC now.
Comment #56
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedNice coordination, Credit goes to all.
Comment #57
larowlanWhat if this fails? Do we care?
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.
We don't reference issues in comments (except for todos). Please describe the test instead
why did we add this, its the default?
Comment doesn't seem relevant to next line of code?
I think we use drupal.org or example.com for test mail domains
We aren't testing the case where the copy isn't sent.
This looks copied from another test, can we add a new trait instead
Comment #58
larowlanBTW, great work on this - looking awesome - thanks
Comment #59
googletorp CreditAttribution: googletorp as a volunteer commented1. 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.
Comment #60
googletorp CreditAttribution: googletorp as a volunteer commentedSo 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.
Comment #61
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedUpdated code looks good. Moving this to RTBC after getting the right output as mentioned in #50
So not uploading the screens again and again.
Comment #62
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedComment #63
larowlan+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?
Comment #64
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedI am not sure about
But if you guide us, perhaps we can do that.
Comment #65
xjmExcellent work. Thanks for uploading a test-only patch above, and for the clear screenshots illustrating the bug.
If we're adding a return value, we probably need to add docs for it?
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.) :)
Several of these use statements are unused; let's clean that up.
Minor: The class name is kind of awkward. Maybe NonexistentEmailContactTest?
Nit: This is not quite grammatical. I'd suggest: "...when a contact e-mail cannot be 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()
andassertNoText()
have sufficient information in their default messages. (Unlike things likeassertEqual()
orassertTrue()
which give you no clue.)Comment #66
googletorp CreditAttribution: googletorp as a volunteer commentedI 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.
Comment #67
googletorp CreditAttribution: googletorp as a volunteer commentedI 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.
Comment #69
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedComment #70
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedPassed from test patch added in #66 Could you please remove
Code throwing error:
Comment #71
googletorp CreditAttribution: googletorp as a volunteer commentedComment #72
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedAwesome, @googletorp great work. moving this to RTBC
Comment #73
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedComment #76
googletorp CreditAttribution: googletorp as a volunteer commentedlooks like test bot failed one of the daily runs, all should still be good.
Comment #77
alexpottIt'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.
Comment #78
googletorp CreditAttribution: googletorp as a volunteer commentedHmm.
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:
and
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.
Comment #79
andypostI 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
Comment #81
googletorp CreditAttribution: googletorp as a volunteer commentedSo 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.
Comment #82
alexpottThis should be postponed on #2349725: Core Mail class prints non-overridable errors on the page
Comment #88
casey CreditAttribution: casey at SWIS commentedComment #90
casey CreditAttribution: casey at SWIS commentedComment #95
quietone CreditAttribution: quietone at PreviousNext commentedI have not triaged this issue but it is no longer postponed.
Is this still a problem?
Comment #97
brad.bulger CreditAttribution: brad.bulger commentedYes, 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.