Allow multiple email recipients

duwid → changed the visibility of the branch 3489931-allow-multiple-email to hidden.

To start this issue, I just added the '#multiple' => TRUE flag to email_to field: https://git.drupalcode.org/issue/migration_notify-3489931/-/commit/434b8...

This change depends on Drupal Core #3296190 📌 Support multiple attribute for email element Active issue and is only working if you have the patch applied.

Independent solution could be to change the field type to string, but then you are lacking of proper email validation: https://git.drupalcode.org/issue/migration_notify-3489931/-/commit/3b12c...

That would then also make the release where this gets added incompatible with older Drupal versions. 🤔

Alternatively we could make that field a textfield, let people enter multiple email addresses separated with commas and do the email validation in the validateForm method.

What's also completely missing in the MR is the actual sending to multiple recipients.

Let's agree on a approach first.

If we change to "text" field and allow commas, we'll need validation before saving the configuration. Then exploding and looping at the moment of sending.

@fjgarlin
Assigning myself the ticket , if you want i can work on it as per comment #7 ✨ Allow multiple email recipients Active .
Please let me know. Thanks.

Yeah, feel free to do it. Anybody can give this a go. Thanks.

I left feedback in the MR.

Made the changes.
Please check.
Thanks.

This doesn't work for existing sites.

If a project is using this module and has email_to: email@example.com in their migration_notify.settings.yml and then updates the code to the current one in the MR, two things are wrong:

• The existing email address email@example.com is not displayed anymore on the settings form at /admin/structure/migrate/notify
• Clicking "Notify now" triggers a success message BUT no email is sent anymore to email@example.com
• When I save the form now and export config, the email is gone from migration_notify.settings.yml

We need to ensure that all of the above works for existing projects which had an existing email_to: email@example.com in their migration_notify.settings.yml. Ideally we transform the existing value in an update hook or we need to tighten the current code.

Not all of the initial feedback was addressed, and I also left some additional feedback. I think we are complicating things a little bit more than we have to and that the code can be simpler.

Also, it'd be great to accept line breaks or commas, as email separators.

I just saw #14 and I fully agree. We don't need to change the type (string) at all. It's ok to present it in a textarea, but not change from string to array.

gaurav gupta → changed the visibility of the branch 3489931-multiple-recipients-email to hidden.

gaurav gupta → changed the visibility of the branch 3489931-multiple-recipients-email to active.

@fjgarlin @leymannx Thanks for pointing out my mistake , I have revert the last commit and adressed the unresolved thread and made changes accordingly . Please check . Thanks

The feedback looks good. I added some more feedback about refactoring some minor things and also about accepting commas as delimiter for the emails.

I made some code suggestions in the MR to clarify what I mean.

As far as I understand, you want two delimiters for the multiple email addresses: commas and new lines. Am I right? However, won't it be confusing to have two delimiters? According to me, it could cause confusion.

Yeah, those are the two most common cases, and it's easy to support them both. People would use one or another.

So

email1@test.com
email2@test.com
email3@test.com

And
email1@test.com, email2@test.com, email3@test.com

would be valid. We can save them all as line-break delimited, but accepting both would be really easy.

I changed the suggestions slightly to add the exploding function to the service instead of the form, so it can be reused in more places.

@fjgarlin,
Thanks for the suggestions.
Please check.
Thanks.

The code looks good to me. @leymannx - would you be able to test this in an existing project to make sure that everything continue working as expected? Once you do it, and if you don't have any further code suggestions, we can set this to RTBC.