What happens if it's enabled with no email added?

Hi @drupalninja99,

Thank you very much for posting this issue and I certainly apologize for the delay of this answer but it actually took me a while to go through the issue queue.

This is just a quick update to log a related issue reported at #1964070-6: Add Variable module integration., which would also seem to be related with the default init of the reroute_email_address or module's handling when config field is left empty.
(@TODO: Get back to #1964070-6: Add Variable module integration. and check more in detail code changes and how they could relate to this current issue)

Feel free to let me know if you would have any suggestions, ideas, comments, issues or concerns related with this issue or a potential approach/solution, it would certainly be highly appreciated.

Thanks in advance to everyone for your help, reviews, feedbacks and comments on this issue.
Cheers!

Quick follow-up on this issue:

After checking the code a bit and this issue in more details, I think a short answer to this question might be:

Currently, even if rerouting is enabled, if the Email addresses field in the Reroute Email Settings form is saved empty (reroute destination email address is an empty string), then nothing happens (module does nothing, no rerouting takes place).
More precisely line 73 in reroute_email.module:

<?php
    if (!variable_get(REROUTE_EMAIL_ADDRESS, '')) {
      // If email address not in settings, then do nothing.
      return;
    }
?>

 
In order to assess this issue or perhaps improve the support/help, the following suggested approaches could potentially be considered:

1. Modify the Email addresses field description (or with a more prominent form help text, for example) on the Reroute Email Settings page to let users know that rerouting actually (currently) only happens if an email address is provided.
   In other words, let users know that if the reroute destination address field is saved empty, no rerouting happens (whether rerouting is enabled or not).
    
2. Enforce Email addresses field is required, which would be possible, since the field has a default value.
   It wouldn't disrupt the UI/form save operations as well if rerouting is enabled or disabled and the Email addresses field is hidden (7.x-1.x).
    
3. Provide a default fallback to variable_get('site_mail', ini_get('sendmail_from')) if reroute_email_address is empty and let users know about it in the description text.
   In which case, it might come down to removing quoted code above (reroute_email.module, line 73), and initializing REROUTE_EMAIL_ADDRESS with a default value to be passed at reroute_email.module, line 117

   <?php
       $addresslist = preg_split(REROUTE_EMAIL_EMAIL_SPLIT_RE, variable_get(REROUTE_EMAIL_ADDRESS, ini_get('sendmail_from')), -1, PREG_SPLIT_NO_EMPTY);
   ?>
   

 
I would greatly appreciate if you could please give me some feedback on what you would think would potentially be the most appropriate way this problem could be approached?
Personally, I would think perhaps the second or third options would be the most suitable, but I would certainly be glad to hear everyone else's feedback.

The first approach wouldn't alter the way the module currently behaves and handles empty reroute_email_address (equivalent to email rerouting disabled). But would we really need to keep the current logic/behavior? Couldn't the same result (as the current behavior) be achieved by disabling email rerouting?

Please let me know if I forgot, missed or overlooked any other potential solution, we would greatly appreciate to hear any other suggestions, ideas or recommendations for a proper approach.

Feel free to let me know if you would have any questions, comments or concerns on any aspects of this issue, I would surely be glad to provide more information or explain more details.

Thanks very much in advance to everyone for your questions, feedbacks, testings, reviews, ideas and comments.
Cheers!

hook_mail_alter() does allow for aborting the message entirely, so that is another option if you think it's worth spending the time to make the code changes.

Personally I'd be inclined to go with option 2 (make it a required field) as the simplest change which improves on the current situation.

Hi @jweowu,

Once again thank you so much for your great follow-up on this module.
I wanted to wait a little bit (the usual 2 weeks) to see if we could get any more feedback on this issue, but it seems we're mostly going to have to keep moving it forward together, since I wouldn't want to let it stall much longer.

hook_mail_alter() does allow for aborting the message entirely, so that is another option if you think it's worth spending the time to make the code changes.

Thanks a lot for pointing that out...
I completely missed this key option which would seem to actually be the answer to the original post:

Does it just burn the email? Ideally that is what I would like to do so that email rules never trigger off accidental email blasts on dev servers.

In this case, immediately after enabling the Reroute Email module, all emails would be aborted/intercepted ("burnt", as mentioned by original poster - not delivered) and we could for example log some information with a watchdog message.

If an empty value is saved for the Email addresses field, all outgoing emails would be aborted and a watchdog entry logged with rerouted email message.

With this solution, it would appear we could actually add a new feature to the module, that might be useful for development: Intercepting all outgoing emails to be recorded with a watchdog entry.

The major difference between this solution and other options mentioned in #2, is that none of the other methods would seem to provide this possibility/choice to disable emails delivery completely.
All the suggestions from #2 would still end up triggering outgoing emails, no matter what the default destination could be (whether it is system's or site's email addresses).
 

Based on this idea, I went ahead and came up with an initial draft for a patch against reroute_email-7.x-1.x at 6bea0ec, which aborts sending of all rerouted emails if no destination email is provided and records a watchdog entry.
File attached as: reroute_email-abort-sending-reroute-address-empty-1722572-4.patch.

The idea behind this feature would be to provide additional guarantees to developers that as soon as the module is enabled all outgoing emails would at least be intercepted/captured (not delivered).

Additionnally, it could serve as a debugging tool to check emails' contents while working on Mailing functions.
The debugging information in Recent log entries would be very different from the one obtained when email rerouting is properly configured, since it wouldn't display any email sending/delivery information that would be specific to each module (For example contact form, password recovery, etc...). But it would still leave the choice to users to properly configure a reroute email address destination, getting back to what the module already does well.

This patch has been tested and seems to work as expected, but I would greatly appreciate to have your feedback, questions, comments, reviews, suggestions, recommendations, improvements and testing of this patch.

If you would agree this initial patch would be the right direction to take to fix this issue and that it could add an interesting feature to the module, more work would still need to be done before it could get rolled in: updating README.TXT/Project page and Automated tests (reroute_email.test).
But once we would be settled on the direction/implementation, we should be able to come up with a more complete version much quicker.

Additionally, I would be glad to hear your comments, issues, questions, objections, recommendations, suggestions, testing, reporting or concerns on the idea of adding this new feature to the module, as well as the attached patch or any other aspects of this ticket in general.

Thanks in advance to all for your feedback, reviews, testing and reporting.
Cheers!

Quick follow-up on this issue:

I quickly re-rolled previous patch to clean-up a bit the call to watchdog according to its API.

Please ignore previous patch, new patch attached as: reroute_email-abort-sending-reroute-address-empty-1722572-5.patch.

Thanks in advance to all for your feedback, reviews, testing and reporting.
Cheers!

I tested this out via simplytest.me, configured an empty address, and sent email with the test form, and saw this error:

Error message
Notice: Undefined offset: 0 in reroute_email_mail_alter() (line 117 of /home/sb085c287752c3bc/www/sites/default/modules/reroute_email/reroute_email.module).

being the last line of:

    $addresslist = preg_split(REROUTE_EMAIL_EMAIL_SPLIT_RE, variable_get(REROUTE_EMAIL_ADDRESS, variable_get('site_mail', ini_get('sendmail_from'))), -1, PREG_SPLIT_NO_EMPTY);

    if (!in_array($to, $addresslist)) {
      // Not on the list, so reroute to the first address in the list.
      $message['headers']['X-Rerouted-Original-To'] = $to;
      $message['to'] = $addresslist[0];

As $addresslist is an empty array when our configured address is an empty string.

Hi @jweowu,

Thank you very much for your kind feedback and time for testing.

Indeed, this really was a very rough initial patch, probably more to see if anybody would have any objections on this particular resolution method.
I reproduced exactly the same test on my development environment and got the error message as well... really not sure how I could have missed this Notice: Undefined offset: 0...

In any case, I would assume your reply would express your tacit agreement with the suggested feature, the potential solution, direction of the patch and therefore I went a head and did another attempt to push this patch a little bit further.

Please find attached to this comment an updated patch against reroute_email-7.x-1.x at 6bea0ec, which fixes the issues from the patch at #5 and modifies other components of the module (as suggested in #4).
File attached as: reroute_email-abort-sending-reroute-address-empty-1722572-7.patch.

This patch should already be very solid and should include all the components to be modified if the feature was to be committed:

1. The feature that was fixed (from previous patch, in reroute_email.module), including updated reroute email address field description on admin settings form (reroute_email.admin.inc).
2. Updated Test Cases (reroute_email.test), in RerouteEmailDefaultAddressTestCase to test each line of the added feature as well as system default email (sendmail_from system variable), in case Reroute Email is enabled with no reroute email address configured (edge case).
3. Updated README.TXT to let users know about this particular behavior, which might as well be useful for debugging special cases related with emails (directly in the Recent Log Messages). This content would most likely be used to update project's page, when a new stable version is released.

In terms of testing, thanks to the modifications made to the Automated Test Cases, several other bugs could be identified and resolved in the reroute_email.module, from the previous patch (I'm wondering how I could even have tested successfully previous patch only once). So I'm definitely much more confident about this new patch, since it's now tested from code.

I would highly appreciate to have your testing/reporting, reviews, comments, questions, feedbacks, suggestions, concerns, objections or issues on this updated patch. It would surely be greatly helpful.

Once again, I would very much be looking forward to seeing this committed and getting this issue fixed, in order to potentially release a new stable version of the module as soon as possible, then, move on to another very interesting feature that's been pending for so long: #1571500: Separate the recipient (list) from the address whitelist. Add domain whitelist..

Feel free to let me know if you would have any questions, comments or suggestions about the code, the updated patch, suggested approach/feature or any other aspects discussed in this comment or issue, I would be very happy to explain in more details.

Thanks in advance to everyone for your testing, reporting, reviews, comments and feedbacks.
Cheers!

I would assume your reply would express your tacit agreement

Yes, I do think it's a good new feature for the module.

Thanks very much @jweowu!

I had hoped your attention could have got caught by this particular statement :-).

It seems it would take some time and experience to get to know Drupal.org Testbots, but with some work and help from the community, the patch could be updated to hopefully pass the tests this time.

In short, it would seem the sendmail_from system variable would be unset on the testbot server and therefore ini_get('sendmail_from') would return NULL.
For the story, after seeing the results of the tests with a single failed test line 462, I went straight to the #drupal-contribute IRC channel where I immediately got a reply:
Thanks a lot to @brianV, @jthorson, @rfay and @davereid, who were all very helpful and answered me immediately:
Currently, it would seem that indeed, for certain reasons (security would be one of them), the call to ini_get('sendmail_from') would return NULL.
For more information, this is tracked as a support request at #2088909: ini_get('sendmail_from') returns NULL with Testbot.

Therefore, on the Drupal.org Testbot infrastructure, it doesn't seem possible for now to test the case where the sendmail_from system variable would be used as default fallback.

So I have added an if statement in the Test Case to check if ini_get('sendmail_from') would return NULL, in which case the placeholder [Reroute email address not configured] would be used as fallback.

At least with this code change we could test locally or on other environments if this test case could be successfully passed for the sendmail_from system variable.

Please find attached to this comment an updated patch against reroute_email-7.x-1.x at 6bea0ec, with a new attempt to pass the failed test from #7, reported at line 462.
File attached as: reroute_email-abort-sending-reroute-address-empty-1722572-10.patch.

Once again, I would greatly appreciate to have your testing/reporting, reviews, feedbacks, comments, objections, questions, issues or concerns on this updated patch, which will hopefully get us one step closer to getting this committed.

Feel free to let me know if you would have any questions, comments or suggestions about the code, the updated patch, suggested approach/feature or any other aspects discussed in this comment or issue, I would be very happy to explain in more details.

Special thanks to @brianV, @jthorson, @rfay and @davereid for their great help on IRC and to @jweowu for your great reactivity!
Thanks in advance to everyone for your testing, reporting, reviews, comments and feedbacks.
Cheers!

Are you following https://drupal.org/project/reroute_email/git-instructions ?

Hi guys,

@drupalninja99, @jweowu, @shrop: Any luck testing the patch from #10?

@drupalninja99: Did you manage applying the patch and testing it a bit?

Would anyone have any feedback on this issue?
Test the patch from #10 immediately: reroute_email-abort-sending-reroute-address-empty-1722572-10.patch on simplytest.me

It has already been a little bit more than 2 weeks with no activity on this ticket and I was wondering if anybody had the chance to give the patch another round of tests.

We would greatly appreciate to have your feedbacks, comments, questions, ideas, concerns, testing and reporting on this issue, it would surely be very helpful to try to keep this feature request moving forward.

Feel free to let me know if you would have any questions, comments, further issues, or concerns with the patch from #10, I would certainly be glad to provide more information or explain in further details.

Thanks to all in advance for your great help testing, reviewing, reporting, questions, comments and feedbacks.
Cheers!

i applied this to the latest downloadable 7.x-1.x-dev release (ie not from git), no trouble. setting the email field to blank caused each attempt to send mail to abort, with an extensive log entry. i tried this with multiple Subscriptions notifications queued up, and it didn't interfere with the queue processing.

this is probably just a matter of personal test, but it'd be nice to be able to control how verbose that error is. presuming that this is something that people do on purpose, rather than accidentally, it's likely that they won't need to diagnose the event, only to log it. that's yet another thing on the admin form, though.

Hi guys,

Picking up where we left off on this issue.
Unfortunately, there doesn't seem to have been a lot of feedback in the past 8 months...

@brad.bulger,
Thanks a lot for your feedback and taking the time to test the patch.

At this point, perhaps we could already consider moving forward with this version of the feature and potentially coming back to it later on for further improvements (logging, control on verbose, etc...), if necessary.
Actually, this issue was initially meant to provide a better handling for the limit case where reroute email addresses would be empty.
When some addresses are provided, the rerouted emails should be logged by the standard Drupal mail system.

The module could maybe offer more advanced logging features, for example to distinguish between rerouted emails and the ones from the addresses list which were not rerouted.
However, it would be great if we could keep more advanced logging features in another issue, for example: #2115237: Add option to log rerouted e-mails , since this issue has already been stalling for years (!!).
The logging provided in this feature is merely an elegant way of handling a limit case, resulting in the interception of all outgoing emails and an extensive data dump intended for developers.

I hope I was able to answer your concerns, but feel free to let me know if you would have any more questions, ideas, suggestions or objections on this patch or the approach for this issue, I would be glad to provide more information.
 

I would greatly appreciate to have more testing/reporting, reviews, comments, questions, feedbacks, suggestions, concerns, objections or issues on the patch from #10. It would surely be greatly helpful.

Thanks to all in advance for your great help testing, reviewing, reporting, questions, comments and feedbacks.
Cheers!

Alright guys,

After stalling for two more months, the patch from #10 has now been available for testing/reporting for almost a year.

I hope and like to tell myself that at this point, if anybody really had any objections or major concerns with this approach and patch, they would have spoken by now. At this point it would seem reasonable to get this patch committed and this new feature in module's code.

Therefore, I went ahead and got the patch from #10 committed against reroute_email-7.x-1.x at e2bee0e.

Which pretty much wraps up this issue and that's why I allowed myself to mark it as Fixed for now, but feel free to re-open it, or post a new ticket, at any time if you have any further objections with the approach suggested in this ticket or related commit (we would surely be happy to hear your feedback).

I would recommend that we try to keep any additional related feature requests in other issues, such as options to control the information logged in watchdog or any other potential ideas.

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on the suggested feature request, this comment or this ticket in general, we would be glad to provide more information or explain in more details.

Many thanks to everyone for your great help, reviews, testing/reporting, feedback and comments on this issue.
Cheers!