Separate the recipient (list) from the address whitelist. Add domain whitelist.

+1

@jemond, if you like it, please test it (and review) and report your results rather than "+1".

I have spent a lot of time with the patch in #9 above.

It works really well, thanks for sharing. Hope this can be included in the reroute email module proper.

Kind regards,

John

Thanks for the patch, this is just what reroute email needs! I'd love to see it committed to the module.

Thanks,
Pete

I can also confirm that the patch in #9 works well for re-routing to multiple addresses. I have not tested the whitelist options at this point though. I will try to setup another Drupal install where I can test whitelisting a bit more.

Here are some other thoughts I have on this issue:

+++ b/reroute_email.moduleundefined
@@ -5,15 +5,12 @@
+define('REROUTE_EMAIL_WHITELIST', 'reroute_email_address'); // historical reasons

"historical reasons"?

It looks like settings for variable "reroute_email_address" should be moved to "reroute_email_to". It didn't help me by having my reroute addresses moved to the email whitelist settings. I am thinking that some variable cleanup needs to be done in the .install file for variables that need data migration and variables which should be deleted from the database.

The direction here seems to be a good one. The UI side of things look okay to me so far.

@jweowu

Thanks for the quick response to my questions and comments. Okay, I think all of my confusion was because I didn't understand how whitelisting worked in the current stable release. I also get what you are saying and after more code review and I see how you are reusing the reroute_email_address variable.

I noticed that the patch in #9 will not apply to the 7.x-1.x branch. I have re-rolled your code into a new patch what now cleanly applies to 7.x-1.x. I have also updated the new variables which need to be deleted during the module's uninstall process.

I manually tested that redirection works. I also tested multiple whitelist email addresses and domains. Everything seemed to work well.

Please review and let's get this into the module. :)

#14: whitelists-1571500-14.patch queued for re-testing.

Thanks so much for the quick review. I will fix those up into a new patch!

jweowu,

You were right. I definitely missed removing the constant due to its use in the validate function. Very perceptive :)

See the attached patch which includes these changes from your original work.

Thanks!

I completed the following review and testing:

• Reviewed code using diff between patch #20 and #21. Looks right to me. The site_mail variable is used for the reroute_email_address and reroute_email_to variables as a start. sendmail_form is only used if site_mail is not set.
• Disabled and uninstalled reroute_email.
• I then installed the module with patch #21 included.
• Checked the "Enable rerouting" checkbox on the reroute_email config page.
• I confirmed that the "Email address whitelist" and "Destination email addresses" fields defaulted to the site_mail variable.
• I next repeated the uninstall and enabling of the reroute_email module to check the configuration page behavior when the site_mail variable did not exist in the database. I can confirm that ini_get('sendmail_from') is used in that case. Note that ini_get('sendmail_from') is not used as the default if site_mail

So I think this looks right based on the findings by jweowu in comment #21. I don't think I should be changing this issue to RTBC, but I figured I could at least do some review and manual testing to confirm the changes. Thanks!

Tagging for Guardr

Updated patch which merges recent commits to 7.x-1.x (5a9329 through 6bea0e).

Please review. Thanks!

Thanks for the quick review jweowu!

• See updated patch below
• That comment line change was for coding standards. I took your suggested comment and completely agree. I also went ahead and did a coding standards review of the module. Found a few minor things and fixed them in this updated patch.
• I broke out the explode and array_pop because I was getting "Only variables should be passed by reference" exceptions when running simpletests.
• Yep. That comment change was a complete mistake on my part. I restored it to the original in this patch.

Have tested patch in #26. All good!

Thanks @kidrobot! @jweowu, what's the best way to get some visibility on this one to get it committed?

Hi @shrop and @jweowu,

Thank you so much for all your work on this issue, it is surely greatly appreciated.

what's the best way to get some visibility on this one to get it committed?

I can surely understand your concerns of seeing this patch/issue open for so long without getting any feedback from any maintainer. This ticket has always had all my attention and I have actually been following it for a while.

However, since it is one of the largest patches and feature request in the bug tracker, I had assumed I could first go through other issues and come back to this one a little bit later.

In short, after a lot of work on other open issues, we managed getting down to a last one to be fixed before being able to release a new version: #1722572: What happens if it's enabled with no email added?.
I have already got @jweowu's feedback, came up with patch to review in that issue and that's pretty much where we are currently.

As soon as issue #1722572: What happens if it's enabled with no email added? will have been fixed, I assumed it might be a good idea to tag a new release and update project's page with all the latest additions (and there are a few nice ones, like the Test Email form).

Once the new release is out, it would allow us to focus on this issue and get it processed.

Therefore, I would greatly appreciate to have any of your help, feedbacks, comments, ideas, testing, reviews or concerns on the patch and the issue at: #1722572: What happens if it's enabled with no email added?.

I am certainly trying my best, like I told @jweowu before, to get these other issues out of the way as soon as possible, so that we could all focus more on the very interesting features discussed in this ticket.

Any questions, feedback, testing, changes, ideas or recommendations would be highly appreciated.
Thanks to all in advance.

@DYdave,

Thanks all the details. Makes sense to me. As I have time, I will take a look at #1722572: What happens if it's enabled with no email added? and at least test out the latest patch by @jweowu.

@shrop,

Your help would be really greatly appreciated, especially since we're going to be hitting 2 weeks inactivity from #1722572-5: What happens if it's enabled with no email added?, very soon (tomorrow, I think)...
I wanted to actually reach out some more, for some feedback and had hoped @jweowu or you could have perhaps got back to me on the patch, so that sounds like very good timing.

Thanks very much in advance for sharing with us your opinion, all your feedback, reviews and testing.
Cheers!

reroll against the current 1.x-dev code. the underlying code changed a fair amount, so this may not be the best implementation of the original intent. it is working for me in initial testing.

Reroll of #33.

we have been running this patch for a while without problems. i tried running the simpletest suite on the standard 1.2 release and hit all the same failures as i did when i ran it on the patched version. so i don't know what that means, really.

Thanks, Brad; I didn't realize that.

We've also been running this patch for quite some time so +1 RTBC for me.

So tests on the 7.x-1.x branch are succeeding, https://www.drupal.org/node/161854/qa - That means that there is a problem with tests. You can see the problems by looking at the "view" link above, https://qa.drupal.org/pifr/test/1175608

It looks to me like you have to adjust the test to handle the change you've made to the logic.

Hi guys!

Please give me a few days to get back to you on this issue.
I have applied the patch from #46 without any errors (thanks for the rerolls @jweowu).
Code seems clear with inline comments.
There are a few minor things I might change, but overall the implementation is pretty straight forward.

However, I'm very worried about the simple tests :))) ... damn, I'm gonna have to almost rewrite the entire file :)
This is going to be a problem... takes time... might have to do this in several steps, we'll see.

This is definitely coming at the top of my priorities for this module:

• It would help closing almost all current issues in the queue (a LOT of them are related either with multiple reroute recipients or larger fields...).
• So much collaborative efforts have been put into this, for such a long time.
• Ideally, this should go in before porting to D8 (especially with the growing pressure hammering the issue queue for a D8 Port)
• Certainly a very substantial new feature.

 
Thanks again very much to everyone for all the great work that has been put into this, the testing/reporting and feedback.
Cheers!

New attempt to pass tests: submitting new patch.
Let's see if this works....

Hi guys,

Good news! We're finally seeing the end of the tunnel :)

Very smart patch and approach I have to say. Not surprised at all, since it was initially submitted by @jweowu, whose contributions have always been of great quality.

Just one point of discomfort though :)

define('REROUTE_EMAIL_WHITELIST', 'reroute_email_address');

which was discussed very early at #13, and I understand there's nothing we can do about this unless we agree to break ascending compatibility (due to settings.php). In terms of standards it seems a bit "loose" and it might perhaps be a bit confusing to some readers, so I've added extra comments.
But that's nothing we can't live with, especially as we start moving on to D8. I assume this is something we will probably change and strictly align once this gets ported to D8.
 
Alright, straight to the patch:
Please find attached to this comment an updated patch against reroute_email-7.x-1.x at 65e12c2 (along with its interdiff file), which should fix the last remaining issues to successfully pass all tests on QA.
Files attached as:

• Patch: reroute_email-whitelists-1571500-51.patch
• Interdiff: interdiff-1571500-44-51.txt

 
What's changed from #44?
For complete details, I made sure an interdiff would be attached to this comment.
 
Minor changes:
I allowed myself to change the wording of a few inline comments and also add a few more (mostly for myself... it's so easy to forget about the code you've written).
Just a personal preference, I thought we could also change the uninstall process to use a wildcard. This should prevent any more changes in reroute_email_uninstall, in the future, in case any more variables are added/removed/modified.

Major changes:
Actually fixing the tests wasn't that bad. Looking at the details of the tests on QA, only 3 tests failed, which was a great sign of the compatibility of this patch with module's legacy behaviour.
The failing tests were indeed very tricky and I'm glad you left that to me, since I spent quite some time fixing this part of the code a while ago, at #1722572-10: What happens if it's enabled with no email added?.

In detail (mostly for me):
1 - REROUTE_EMAIL_DEFAULT_ADDRESS can't be a constant and has to be dynamically instantiated.
I know this is very unlikely to happen on a real site and it could seem to be only purely theoritical, but as a bottom line, nothing tells us that the system variable site_mail might not be changed during the execution flow.
Defining the constant REROUTE_EMAIL_DEFAULT_ADDRESS as site_mail:

define('REROUTE_EMAIL_DEFAULT_ADDRESS', variable_get('site_mail', ini_get('sendmail_from')));

"freezes" its value whether site_mail is altered in the execution flow or not.
Which is exactly what happens in the SimpleTests, see reroute_email.test, line 472:

variable_del('site_mail');

resulting in 2 different values for: variable_get('site_mail') (which is NULL) and REROUTE_EMAIL_DEFAULT_ADDRESS (not NULL, containing the initial site_mail testing value), returned by reroute_email_destination().

Therefore, I replaced all instances of the constant with a one line function reroute_email_default_address().

2 - Destination needs to be tested against an empty string.
Very tricky one, but according to our discussions at #1722572: What happens if it's enabled with no email added?, the "abort message" behaviour should only occur when the value of the reroute email address (REROUTE_EMAIL_TO, or, reroute_email_destination()) is set to an empty string '', which is slightly more specific than being empty.
Reverting the if statement to what it was: if ($destination === '') fixed the failing tests.

You just got to love SimpleTests!
How could these small/tricky issues have been detected without these test cases?! Additionally, it appears I didn't do too much of a bad job enforcing new behaviours in SimpleTests each time module's code was modified.

Moving forward: what's left?
0 - Your reviews/feedback on this patch.
Also, I wanted to know if it would be worth removing REROUTE_EMAIL_ADDRESS and just replacing all occurances with REROUTE_EMAIL_WHITELIST.
I'm thinking we should.

1 - Crediting and committing the patch:
Obviously many people have been working on this and I'd like to give credits, as much as possible, to those who really tried helping, keeping this feature moving and therefore deserve seeing their name on their work.
I'm thinking of @jweowu (first and foremost, who did most of the work), @shrop, @brad.bulger and @joshf. So I was thinking of perhaps breaking this into 2 commits, one credited to @jweowu, another one credited to @shrop and mentioning @brad.bulger and @joshf in commit messages.
What do you think? Would that seem fair to you guys?

2 - New test cases for the new features:
Alright, so current tests have passed, which is great. But none of these tests actually take into account some of the new and key features added with this patch, in particular: multiple recipients and whitelist/domain whitelist (or only partially tested as part of the legacy tests).
So I'm most likely going to have to follow up on this, probably in another related issue, and create one or more new cases.

3 - Documentation:
I've already updated reroute_email.info to take into account these changes, but we're most likely going to have to rewrite the project page and update the README.txt file accordingly.

Obvisouly, once points 0 and 1 are settled, this is getting committed.

Feel free to let me know at anytime if you would have any further issues, questions, objections, suggestions or concerns on any aspects of this comment or attached patch, I would certainly try answering as soon as possible and providing more information.
Thanks again very much to everyone for your testing/reporting, comments and feedback!
Almost there... Cheers!

Thanks again so much @jweowu for your very constructive feedback and great guidance.
Sorry for this slow reply.
Overall, I'm very happy to see that we were able to make a significant step in the right direction.

Alright, straight to the patch and your comments:

#52.2. and #53:
Thank you very much for putting down these very interesting arguments.
However, I believe now I have perhaps been overeaching (a bit) on this :).
If possible, I'd like to maybe roll back on this to your version of the patch (#44), which would be the closest to the actual version, mostly because I'd like to keep my focus on some of the more essential aspects of this feature.
I personally liked very much your suggested approach in #53, but I am afraid this might end up in dragging us into a different discussion... Maybe we could create a different issue for that? I personally think it wouldn't really be necessary since keeping the current approach works just fine. Additionally, we should keep in mind that many of these variables management issues will be gone with Drupal 8.

3.

My personal feeling is that the module's own code is ok as-is

Me too :) (as said previously: nothing we can't live with).
Although I like the approach you suggested, I think we might all benefit from keeping things a bit more simple in the end, even if coding is a bit "loose".
Thanks again very much for providing more details and clarifying this issue (especially the reason why we couldn't use an update hook).
I totally agree with your concerns on documentation and we'll have to get that changed as well.

One thought though, we could potentially create a new branch for this patch, given the importance of the change and features: reroute_email-7.x-2.x and mark 7.x-1.x as bug fix only/deprecated.
In this case, we could perhaps break the compatibility with 7.x-1.x variables and let users know this is a branch update which may require manual changes. A new branch would also be consistent with the extensive documentation changes as well as the ones to be made to the project page.
I guess it would be completely acceptable to ask from users to manually update their settings.php if they really wanted to update to the new version.
They would still have the choice not to update and stick to the 7.x-1.x branch.
I'm personally thinking this would be the right way to go... what do you think?

4. Funny you're asking because I was thinking exactly the same while working on this particular test, hahaha.
But I didn't want to dig into this as I was afraid I would start drifting away.
I went back quickly to the test file to check the comments and found in reroute_email.test, line 437:

  /**
   * Test reroute email address is set to site_mail, sendmail_from or empty.
   *
   * When reroute email addresses field is not configured and settings haven't
   * been configured yet, check if the site email address or the sendmail_from
   * system variable are properly used as fallbacks. Additionally, check that
   * emails are aborted and a watchdog entry logged if reroute email address is
   * set to an empty string.
   */

I think the NULL value was used to fallback and the empty string used to trigger the particular abort behaviour.
Ultimately, if there is no fallback, like on the QA server, we might want to still abort the message... again, something to be discussed in a different ticket, #1722572: What happens if it's enabled with no email added? I believe.
 
5. Great! Glad we're settled on this. With your constant presence and after all the work everyone has been putting into this, it seems to be the very least I could do :). If others, mentioned in the credits, have any objections, suggestions of concerns, please let us know.
 
6. Not sure why your PHP syntax/code highlight didn't go through in your comment.... strange...
Agreed! Your updated version is much better! I think it's much clearer and probably a bit more optimized.
 
7. Got it! All occurrences of REROUTE_EMAIL_ADDRESS to be replaced with REROUTE_EMAIL_WHITELIST.
 
8. REROUTE_EMAIL_ENABLE_DSM is not registered in reroute_email_variable_info()

I notice that REROUTE_EMAIL_ENABLE_DSM is not registered in reroute_email_variable_info().

Nice catch... Not sure how we could have missed that in #2473245-8: Add simple option to display message when mail was rerouted.

I have committed the necessary changes at 8efc15f and updated corresponding ticket, see #2473245-12: Add simple option to display message when mail was rerouted.

Summary of the changes: For complete details, see interdiff attached to this comment

1. Rolled back the install file.
2. Basic update of the code snippet in the README.txt file.
3. Replaced function as suggested in point 6.
4. Removed all instances of REROUTE_EMAIL_ADDRESS

 
Please find attached to this comment an updated patch against reroute_email-7.x-1.x at 8efc15f (along with its interdiff file), which implements the changes detailed in the list above.
Files attached as:

• Patch: reroute_email-whitelists-1571500-55.patch
• Interdiff: interdiff-1571500-44-55.txt

 
Feel free to let me know at anytime if you would have any further issues, questions, objections, suggestions or concerns on any aspects of this comment or attached patch, I would certainly try answering as soon as possible and providing more information.
Thanks again very much to everyone for your testing/reporting, comments and feedback!
Cheers!

Wow @jweowu!

This really looks great! You're definitely amazing and an incredible Drupal coder! :D

I haven't tested your code yet, but I read it carefully and understood your approach very well (thanks to the great inline comments).
The way you managed getting the deprecated address is so ingenious hahaha.

Perhaps it would need a bit of testing/debugging, like with every fresh code, but ultimately, I think your idea and this code could totally work.

The trade-off is in having this sort of logic in the code-base at all, and it's your call as to whether or not you're happy to do that.

I guess it all comes down to me, hey?! :D

What do you think about creating a 7.x-2.x branch?

Personally, I'm still leaning towards creating a new branch, for many reasons other than code. I just believe it would allow us to have a smoother/softer updating process for such an important change and improvement.

I keep on trying to play out the different scenarios that could be resulting in problematic situations if we were to create another branch and I can't seem to find anything significant enough that it would prevent us from doing so.
It would allow us to keep both versions of the module co-existing (marking the former as obsolete/deprecated/bug fix only, when the latter is ready). We would only need to add an update hook, other than that, module's 7.x-1.x code would essentially stay exactly as it is right now (and we know it works fine with very few outstanding bug reports).

As for my real life personal experience, in terms of the projects using the module in our organization: I wouldn't see any problem at all.

Projects currently using the 7.x-1.x branch could keep doing so, as if no other alternative existed (which has always been the case until the new version) and has always been satisfying (otherwise why would the module have been used in the first place?!).
New projects to start could use the new branch as the recommended release (whatever change would be needed in makefiles or any other automatic project checkout process).
Development teams that would like to update the module (miraculously discovered the new features) would have to go through the project update notes, among which running update.php and manually updating their settings.php would be clearly specified.
If the notes were to be skipped (which would be rather naive for a developer, at least for the first update) and just the update run (at the very least run update.php), the only negative impact I could see would be that the line:

$conf['reroute_email_address'] = "example@example.com";

in project's settings.php would simply become useless (and harmless).
 
However, through the update process, we could still set the config based on the deprecated variable, maintaining the rerouting behavior (nothing to worry about: emails would still be intercepted - whether on shared DB/different settings.php files setups, or more complicated ones, the variables would still get initalized).
Speaking for myself, I would be very surprised if any of my colleagues actually updated the module in one of their projects ... hahaha :D
Once again, we're talking about a development module here, which works very well in its current state and seems to be satisfying for a lot of people and projects.
 
Now, since I took over the maintenance of the module, you and I have been working together at numerous occasions, for a long time, on many issues (it's almost like you have been my only real interlocutor in the project :D and you know the module extremely well).
If I recall properly, in many cases (if not all of them) I chose to listen to your opinion/advice (even reverted a commit haha #2241857: Revert module weight = 99 and implement reroute_email_module_implements_alter()).
So before stepping forward with a new branch, it would still be very important for me to have your green light (your blessing haha).
 
If we were to move forward with a new branch, we would still need to do the following tasks before committing the patch:

1. Make a few adjustments to the patch from #55, among which reroute_email_destination() and REROUTE_EMAIL_WHITELIST.
2. Add an update hook for the 7.x-1.x branch.
3. Add update notes (I've never done that for any module before, but I guess it wouldn't be hard to find a few examples around).

 
Overall, what do you think?
 
Thanks again so much for sharing your great code and going one step further into something that we thought wouldn't actually be possible...
Looking forward to your feedback.
Cheers!

Hi @jweowu,

Thank you very much once again for your great and very helpful feedback.

I'm very happy we are able to move forward with the new branch :D

Then, straight back to the patch:

Please find attached to this comment an updated patch against reroute_email-7.x-1.x at 8efc15f (along with its interdiff file), which implements the necessary changes for the new branch.
Files attached as:

• Patch: reroute_email-whitelists-1571500-55.patch
• Interdiff: interdiff-1571500-44-55.txt

Since, we are now clear about the distinction of the code in two major versions, we don't necessarily need to worry that much about deprecated variables or behavior.
We don't need to fallback on the legacy behavior anymore, relieving the function reroute_email_destination from its initial duties (removed).

Most of the work needed in this updated patch was for the implementation of hook_update_N.
I have already tested this patch and the update multiple times, with 5 different cases (whether reroute_email_address was set in settings.php, db, both, or empty string). So I have already done quite a bit of debugging and clean up, giving me enough confidence to get this patch submitted for your reviews.
(I have to say testing update scripts is very annoying :D hahaha, having to install/uninstall and run the update script all the time, manually...)

One thing I found out though:
In fact there is no real way to tell whether a variable has been set in settings.php or in the database.
If the variable pulled from the db and the one pulled from the settings.php have the same value, how do we actually differentiate them?
This could correspond to the same result as if the variable had been set in the db and not in the settings.php...
It is impossible to tell users precisely in which case potential code snippets should be cleaned up from their settings.php.
Therefore, we display the message in all cases:
Important warning: It is strongly advised that you update module's $conf variables in site's settings.php file. In particular, you may have to remove the line of code with $conf['reroute_email_address'].
The variable reroute_email_address is deprecated and has been removed.

Additionally, I seized the opportunity to revamp almost entirely the README.txt file to be consistent with the new version (note the section about "key contributors").

Personally, I think this patch could be committed.
So I'm very much looking forward to your reviews and comments (especially @jweowu) on the update hook, the README file and any change that have been done since #44 (see interdiff).

Lastly, feel free to let me know at anytime if you would have any further issues, questions, objections, suggestions or concerns on any aspects of this comment or attached patch, I would certainly try answering as soon as possible and providing more information.
Thanks again very much to everyone for your testing/reporting, comments and feedback!
Cheers!

Blessed be these simple tests!
Caught a couple of poor logical find/replace mistakes... replaced with the wrong constant variables.
New attempt for this updated patch, reroute_email.test was the only file changed from #59.

ah... too bad, won't be able to fix that today unfortunately.

But please start reviewing and testing if you have time.
I'll get the Tests fixed whenever possible.

Thanks!

Following up on the failing test cases for the patch at #62:

It appears the problem actually came from the call to ini_get('sendmail_from'), resolving on the QA server to '' an empty string, and not NULL.
Therefore, in order to keep the existing logic enforced by #1722572: What happens if it's enabled with no email added?, and its related test case RerouteEmailDefaultAddressTestCase, the value of the field REROUTE_EMAIL_TO has to be tested against an empty string and not $destination (see patch at #62, reroute_email.module, line 193).

<?php
    // Abort sending of the email only if the address is an empty string.
    if (variable_get(REROUTE_EMAIL_TO, NULL) === '') {
?>

Please find attached to this comment an updated patch against reroute_email-7.x-1.x at 8efc15f, which restores the compatibility with the existing logic and tests.
File attached as: reroute_email-whitelists-1-1571500-66.patch
 
Back to Needs review and marking helper issue #2695825: Make patch-1571500 pass existing SimpleTests as fixed.
 
Feel free to let me know at anytime if you would have any further issues, questions, objections, suggestions or concerns on any aspects of this comment or attached patch, I would certainly try answering as soon as possible and providing more information.
Thanks again very much to everyone for your testing/reporting, comments and feedback!
Cheers!

No feedback on this for 2 months, for such a substantial and interesting feature....
I would have expected a little bit more replies.

Would be fantastic if anyone could please give this patch a shot?

Thanks in advance to everyone for your replies, testing/reporting, feedback and participation in this module's issue tracker.
Cheers!

#66 works well for me. Thanks @DYdave!

Tested basic rerouting and the domain whitelist.

This patch adds 90% of what I need for a ticket I'm working on right now :). Thx for the great work!

Before implementing the missing 10% (rerouting only specific mail keys), I first needed to tidy up the code a bit. So, I'm attaching a patch + interdiff with my improvements:

• Refactored business logic to live in a separate function: reroute_email_choose_action($message).
• Refactored hook_mail_alter() to just call the new function to get the action and perform it (with a switch statement).
• Changed the domain whitelist input field (config form) to be a textarea; this is more consistent, IMO.
• Added a new header: X-Rerouted-Action; used to signal which action was chosen for the message. Only case where it's not used is when a message is blocked for delivery (aborted).
• Print different Drupal messages depending on the action, even address/domain whitelisting bypass action have a separate message.
• Updated testing suite to accomodate some small changes.
• Improved wording on description for various fields in config form.
• Fixed various coding standards.
• Updated entries in .variable.inc to match what is defined in the form constructor.

Tests are running fine in a local test site (vanilla Drupal 7.50). Let's see what the testbot has to say.

I just did some manual testing of #69 (which should be almost functionally equivalent to #66) and found this:

- Leaving "Destination email addresses" empty causes otherwise-rerouted messages to be aborted; whitelisting still works in this case; this is different from #66, but if this doesn't break existing sites I think the new behavior is better.
- Whitelisting a single e-mail address works fine.
- Whitelisting a single domain or multiple domains works fine. I even tried with mixed delimiters (comma and newline) and it's resillient enough to handle that scenario correctly!.

Minor update: adjusted description text to account for whitelisting being active even if "Destination email addresses" is empty. Interdiff is still against #66.

And this is a screenshot of the config form as it renders with #71:

Hi @jweowu!. Patch in #71 is almost functionally equivalent to #66, only change is the one I mentioned on whitelisting still being followed even if "Destination email addresses" is empty; the testing suite from #66 required only very minor tweaks for #71 to pass. Additionally, getting the blocking behavior to override whitelisting is *very* easy to do.

Please have a closer look at the list of improvements I posted in #69, there you'll see I identified some cases where #66 is definitely needing polish and I did just that :).

As for the additional scope I mentioned, my plan is to generate clearly separated patches for the refactor I did and for the new functionality. No work has been done yet on the extra 10% I mentioned, but now that you mention it I think your idea on opening a separate ticket for that is better to not disrupt the patch being worked in here :).

Adding useful comment about RFC spec sections for setting multiple addresses in To: header.

@DYdave: this info came from viktar_pryshchepa (#2627274: Ability to reroute email to several recipients), so I think it may be good to credit that person here too.

Updating patch with very minor update for X-Rerouted-Action header, it now uses the raw action selected for the message.

New patch with minor update to make coder happy when parsing .install file.

I apologize for posting so many patches, I'm just in a rush trying to get this to a working state ASAP.

Hi @jedihe,

Sorry for the slow reply...

Gonna have to catch back up on this issue and your patch.
Please give me a few days to go over the patch and the latest comments.

Thanks again for your contribution.

Hi @DYDave! great to see you back on this issue :). I'll keep an eye on this, to provide adjustments/feedback as needed.

#77 works for me (applied to 8efc15fe557fd6c88ef0067d6d4a1626ec5e1b88).

Hi, @all,

It was a kind of challenge to read and investigate all thread :)

1) Rerouting by a domain and multiple recipients features were done in:
#2924078: Update settings form to have 2 emails fields.
Additional tests were done and update paths via hook_update_N are handled as well.
Committed to both 7.x and 8.х branches.

2) There was a discussion about supporting of variables module.
It's a part of 7.x branch already.

3) Readme file was updated according to the proposed changes.

4) Rerouting by mail key mode will be discussed further in #2790143: Add an ability to filter by mail keys.

All changes were committed to both 7.x and 8.х branches.
One commit authored to @jweowu, another one to @jedihe, all others are credited.

I’m going to mark this one is fixed.
Please reopen otherwise or create separate (smaller) issues.

The age of this issue 5.5 years. Some children born on creation date are going to school already :)

Thanks everyone for yours effort!

I guess we will have almost clear issue queue in 2 weeks,
and we will be able to create new releases for both 7.x and 8.х.

Cheers!