Action "Send email" not validating recipient field with multiple addresses

pameeela → credited Manibharathi E R → .

pameeela → credited pooja saraah → .

Closed 🐛 Allow multiple recipients in email actions Closed: duplicate as a duplicate, moving credit.

pameeela → credited eleonel → .

pameeela → credited lucassc → .

pameeela → credited Anchal_gupta → .

pameeela → credited Sivaji_Ganesh_Jojodae → .

pameeela → credited rishabh064 → .

pameeela → credited tanubansal → .

pameeela → credited Nathaniel → .

pameeela → credited ashishsingh27 → .

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Confirmed the issue on Drupal 10.
Was not able to add multiple emails
Applied the patch and I can

Believe it or not but "te,st"@example.com is a valid email address :(

I think we should do a more complicated fix here. I think we need to change it so textfield is a textarea and we enter them 1 one line. And then in the form submit we smoosh them together with a comma.

Good catch @alexpott. Lets try this one. Done the suggestion in #27

Oops! Patch didn't upload

+++ b/core/lib/Drupal/Core/Action/Plugin/Action/EmailAction.php
@@ -124,7 +124,9 @@ public function execute($entity = NULL) {
-    $recipient = PlainTextOutput::renderFromHtml($this->token->replace($this->configuration['recipient'], $this->configuration));
...
+
+    $recipient = PlainTextOutput::renderFromHtml($this->token->replace($recipients, $this->configuration));

@@ -190,9 +192,13 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+    $recipients = explode("\r\n", $recipients);

PHP_EOL vs \r\n - which one should we use - I think we should look at the paths in block config and do the same.

Also I think we don't need to introduce $recipients variable in EmailAction::execute() - just using a single $recipient variable is fine - even if there are multiple recipients

Added a patch to address point 1 in #31. Moved to use the same regex that the request path block visibility field uses.

> Also I think we don't need to introduce $recipients variable in EmailAction::execute() - just using a single $recipient variable is fine - even if there are multiple recipients

I didn't address this. The seperate variable is introduced, otherwise the code looks messy IMO

PlainTextOutput::renderFromHtml($this->token->replace(preg_replace("(\r\n?|\n)", ',', $this->configuration['recipient']), $this->configuration));

Happy to be overridden on this point if people disagree

did the required changes.

@Ajeet Tiwari,

did the required changes.

What changes? You've changed it back from a textarea to a textfield and deleted the test.

Sorry, i missed that change which i made actually while creating the patch. i will update it in the next patch.

Hiding patch.

@smustgrave might be having a case of the monday mornings, but why does this need a reroll?

Could of sworn when I tested #32 was failing to apply. Clearly not the case though

Wait it doesn’t. Just retested

Ah yes you're correct. When I checked this morning tests were still green, other commits since the last test run must have stopped this applying. Thanks for confirming, restoring the needs reroll tag.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

As patch #32 doesn't apply to D10 anymore adding a reroll for it.

error: while searching for:
   * {@inheritdoc}
   */
  public function validateConfigurationForm(array &$form, FormStateInterface $form_state) {
    if (!$this->emailValidator->isValid($form_state->getValue('recipient')) && strpos($form_state->getValue('recipient'), ':mail') === FALSE) {
      // We want the literal %author placeholder to be emphasized in the error message.
      $form_state->setErrorByName('recipient', $this->t('Enter a valid email address or use a token email address such as %author.', ['%author' => '[node:author:mail]']));
    }
  }


error: patch failed: core/lib/Drupal/Core/Action/Plugin/Action/EmailAction.php:190
error: core/lib/Drupal/Core/Action/Plugin/Action/EmailAction.php: patch does not apply

The last submitted patch, 43: 3307187-43.patch, failed testing. View results →

Thanks for the reroll!

Am able to add multiple recipients now.

+++ b/core/tests/Drupal/KernelTests/Core/Action/EmailActionTest.php
@@ -64,4 +63,41 @@ public function testEmailAction() {
+    // Ensure that the email sending is logged.
+    $log = \Drupal::database()
+      ->select('watchdog', 'w')
+      ->fields('w', ['message', 'variables'])
+      ->orderBy('wid', 'DESC')
+      ->range(0, 2)
+      ->execute()
+      ->fetch();
+
+    $this->assertEquals('Sent email to %recipient', $log->message);
+    $variables = unserialize($log->variables);
+    $this->assertEquals($recipientsExpected, $variables['%recipient']);

Can we use the test mail collector and assert mail trait instead of relying on watchdog entries?

Also can we get red/green patches for the new approach.

Thanks

Changing component.

Action module is approved for removal. See 🌱 [Policy] Deprecate Action (UI) module in D10 and move to contrib in D11 Fixed

I have modified the code to use test mail collector and assert mail trait instead of watchdogs entries. Can you please review if it works? Thanks

The last submitted patch, 50: 3307187-50.patch, failed testing. View results →