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

Applied patch #2 on 9.4.x.Multiple email addresses are accepted in the recipient field after applying this patch.

Before patch:

After patch:

Verified and tested patch #2 on the drupal 9.4.x-dev version and the patch applied successfully and looks good to me.

Testing Steps:

1. Install drupal 9.4.x-dev version.
2. Install the Action module.
3. Goto admin/config/system/actions/add/action_send_email_action
4. Enter the multiple email ids in the recipient field and send.
5. An error message will be displayed.
6. Apply the patch and clear the cache.
7. Again enter the multiple email ids in the recipient field and send.

Testing Results:
After applying the patch multiple email addresses are accepted in the recipient field
We can move this ticket to RTBC.

D10 version needed
At this time we would need a D10.1.x patch or MR for this issue.

Hi i will review this patch.

Applied patch #9 for Drupal 10.1.x version and working fine. adding screenshot for the reference.

#9 patch applied cleanly on 10.1.x-dev , looks good adding screenshot

@gaurav-mathur and @himanshu_jhaloya, thank you for your interest in this issue. However, uploading screenshots of a patch applying is never helpful. The testbot tells us if the patch applies. You can find instructions for Review a patch or merge request task in the Contributor guide. The contributor guide also has details for other ways to help.

Reading the issue I see that this has not passed the core gates.

This issue is tagged for tests and that still needs to be done. There is also no code review.

Setting back to NW.

Done a first review of the patch, it looks good. I made some minimal changes and added a test.

There is a bit of code duplication in the test with creating the plugin and reading the logs, but it hasn't broken the rule of three so I've left it rather than creating private functions.

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

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.

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

Hiding patch.

@smustgrave I 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

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 #3266458: [Policy] Deprecate Action (UI) module in D10 and move to contrib in D11

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