Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Deprecated Mail::Sender by Email::Sender #61

Merged
merged 11 commits into from
Aug 14, 2017

Conversation

tkempf
Copy link

@tkempf tkempf commented Jul 18, 2017

Replaces the deprecated Mail::Sender with Email::Sender without the need of changing master.cf.
Works with TLS on Debian Stretch
Adds a regexp novacation_pattern to define adresses where vacation E-mails never should be sent

@DavidGoodwin
Copy link
Member

Thank you.

Can you include a comment near where $novacation_pattern is specified which illustrates what it could look like?

# Replacing deprecated Mail::Sender by Email::Sender
# Add configuration parameter $novacation_pattern in order to exlude specific alias-recipients from
# sending vacation mails, even if one or multiple of the recipients the alias points to has vacation
# currently active.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a mix of tabs and spaces here. Please use only spaces.

# libemail-valid-perl
# libtry-tiny-perl
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another tab that should be a space ;-)

# default = ''
# preventing vacation notifications for recipient info@example.org would look like this:
# our $novacation_pattern = 'info\@example\.org';
our $novacation_pattern = '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea!

Nitpicking: could you please use a slightly more readable variable name, maybe $no_vacation_pattern?

'Content-Type' => "text/plain; charset=utf-8",
'X-Loop' => 'Postfix Admin Virtual Vacation',
],
body => $body,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section contains a wild mix of tabs and spaces. Please change it to spaces only.

Besides that, the new code looks good (disclaimer: I only read it, but didn't test it)

$logger->debug("Vacation response sent to $to from $from subject $subject sent\n");
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more tabs ;-)

@@ -695,6 +722,12 @@ sub check_and_clean_from_address {
exit(0);
}
$logger->debug("Email headers have to: '$to' and From: '$from'");

if ($to =~ /^.*($novacation_pattern).*/i) {
Copy link
Contributor

@Callidior Callidior Jul 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the empty default value for $novacation_pattern, this will match everything, preventing vacation mails from ever being sent.

header => [
To => $to,
From => $from,
Subject => $subject,
Copy link
Contributor

@Callidior Callidior Jul 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have you removed encode_mimewords($subject, 'Charset', 'UTF-8')?
This way, UTF-8 characters in vacation subjects are not encoded as quoted printable and will be rendered incorrectly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because i didn't read the RFCs and tests with non ASCII-chars in the Subject worked with my client. I'll revert that change.

@DavidGoodwin DavidGoodwin merged commit d98e83e into postfixadmin:master Aug 14, 2017
@DavidGoodwin
Copy link
Member

I tested this locally; test.sh didn't explode. I'll move to using it on a real server and see how that fares. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants