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

load report_store modules using Module::Load instead of relying on 'eval' #237

Merged
merged 5 commits into from
Jun 1, 2024

Conversation

bigio
Copy link
Contributor

@bigio bigio commented May 31, 2024

This commit is a fix for issue #234

Copy link
Owner

@msimerson msimerson left a comment

Choose a reason for hiding this comment

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

Thanks! Can you also add Module::Load to Build.PL?

@msimerson
Copy link
Owner

Since this PR adds the Module::Load dependency, might as well also do Report::Receive:

index 7ba1561..7385b40 100644
--- a/lib/Mail/DMARC/Report/Receive.pm
+++ b/lib/Mail/DMARC/Report/Receive.pm
@@ -11,6 +11,7 @@ use Email::Simple;
 use Encode;
 use IO::Uncompress::Unzip;
 use IO::Uncompress::Gunzip;
+use Module::Load;
 use XML::LibXML;
 
 use parent 'Mail::DMARC::Base';
@@ -20,7 +21,7 @@ require Mail::DMARC::Report::Aggregate::Record;
 
 sub from_imap {
     my $self = shift;
-    eval "require Net::IMAP::Simple";    ## no critic (Eval)
+    load "Net::IMAP::Simple";
     croak "Net::IMAP::Simple seems to not work, is it installed?" if $@;
 
     my $server = $self->config->{imap}{server} or croak "no imap server conf";
@@ -30,7 +31,7 @@ sub from_imap {
     my $port   = $self->config->{imap}{port} // 993;
 
     if ($port != 143) {
-        eval "use IO::Socket::SSL";  ## no critic (Eval)
+        load "use IO::Socket::SSL";
         if ( $@ ) {
             croak "Can't load IO::Socket::SSL: $!\n";
         };

Copy link
Owner

@msimerson msimerson left a comment

Choose a reason for hiding this comment

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

LGTM

@msimerson msimerson merged commit 9588cc8 into msimerson:master Jun 1, 2024
5 checks passed
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.

2 participants