Bug #64626 for Email-Valid: [PATCH] silence an error in an eval

Sun Jan 09 06:49:14 2011 andrew [...] slando.com - Ticket created

Subject:	[PATCH] silence an error in an eval
Date:	Sun, 9 Jan 2011 11:48:58 +0000
To:	bug-Email-Valid [...] rt.cpan.org
From:	Andrew Sayers <andrew [...] slando.com>

We find it useful to trap all errors in our program, as errors within 'eval' blocks occasionally contain vital debugging clues. Email::Valid sometimes throws an error inside an eval, confusing our logs. Please consider the following patch, that would squash the error: $ diff -Naur /opt/slando/perl/lib/site_perl/5.12.2/Email/Valid.pm Valid.pm --- /opt/slando/perl/lib/site_perl/5.12.2/Email/Valid.pm 2010-06-11 03:05:55.000000000 +0100 +++ Valid.pm 2011-01-09 11:23:13.000000000 +0000 @@ -311,7 +311,7 @@ local_rules )], \@_); my $addr = $args{address} or return $self->details('rfc822'); - $addr = $addr->address if eval { $addr->isa('Mail::Address') }; + $addr = $addr->address if eval { ref($addr) && $addr->isa('Mail::Address') }; $addr = $self->_fudge( $addr ) if $args{fudge}; $self->rfc822( -address => $addr ) or return undef;

Message body is not shown because sender requested not to inline it.

Sun Jan 09 07:54:44 2011 rjbs [...] cpan.org - Correspondence added

"trap all errors"? Do you mean with a SIG{__DIE__}? Your proposed patch won't catch errors where, for example, $addr is an unblessed reference The eval already handles every error the library cares about. Adding more code to it to handle bizarre cases like a SIG{__DIE__} seems redundant. -- rjbs

Sun Jan 09 07:54:47 2011 The RT System itself - Status changed from 'new' to 'open'

Mon Jan 10 06:18:37 2011 andrew [...] slando.com - Correspondence added

Subject:	Re: [rt.cpan.org #64626] [PATCH] silence an error in an eval
Date:	Mon, 10 Jan 2011 11:17:49 +0000
To:	bug-Email-Valid [...] rt.cpan.org
From:	Andrew Sayers <andrew [...] slando.com>

Yeah, we generally use a SIG{__DIE__} to catch silly mistakes - agile web development is great for many things, but typo-avoidance is not one of them ;) $addr being an unblessed reference is a good example actually - it's quite possible that we'd accidentally pass an array through some bizarre code path, miss it during testing, and get weird live bugs. I know it's a bit of an edge case, but a SIG{__DIE__} that lets us spot such daftness can be a great debugging aid. I've attached a test that shows what I mean. While creating the test, I noticed that for some reason, "foo123"->isa(...) doesn't cause the issue, but "123foo"->isa(...) does. So eval { $addr->isa('Mail::Address') } will only die for people whose e-mail address doesn't begin with a letter.

Message body is not shown because sender requested not to inline it.

Thu Jan 19 11:45:40 2012 fbergo [...] gmail.com - Correspondence added

From:

fbergo [...] gmail.com

On Sun Jan 09 07:54:44 2011, RJBS wrote: Show quoted text

> "trap all errors"? Do you mean with a SIG{__DIE__}? > > Your proposed patch won't catch errors where, for example, $addr is an > unblessed reference > > The eval already handles every error the library cares about. Adding > more code to it to handle > bizarre cases like a SIG{__DIE__} seems redundant.

I have submitted another patch, here: https://rt.cpan.org/Ticket/Display.html?id=74055 that fixes this issue by eliminating the eval block. I ran into this bug as we use Email::Valid in a CGI script that checks emails with SIG{__DIE__} caught, and we had the unusual bug of valid emails starting with digits causing the CGI to bail out. Only after submitting that patch I saw this ticket, but I believe the patch I submitted there is cleaner.

Sun Jan 22 11:39:06 2012 rjbs [...] cpan.org - Correspondence added

I accepted the above change to use Scalar::Util. -- rjbs

Sun Jan 22 11:39:07 2012 rjbs [...] cpan.org - Status changed from 'open' to 'resolved'

Bug #64626 for Email-Valid: [PATCH] silence an error in an eval

Preferred bug tracker