mkahn has asked for the wisdom of the Perl Monks concerning the following question:

Fine monks,
Whats wrong with this? 
my $url = param('url');

MY_PARSE($url);

sub MY_PARSE {
my $listing = shift;
open LISTING, "/usr/local/bin/lynx -source $listing|" or die "can't op
+en HANDLE_IN $!";
    my $count;
    @line = <LISTING>;
foreach $line (@line) {
              my @addrs = Mail::Address->parse($line);
    foreach $addr (@addrs) {
        print $addr->address(),  "\n";
                                 }
                      }            }

[download]
Thanks!

Replies are listed 'Best First'.
Re: parsing emails w/ MAIL::ADDRESS
by davido (Cardinal) on Feb 04, 2004 at 07:38 UTC
    You haven't told us what it's failing to do. So I can only guess given what you've shown us.

    Here are some thoughts:

    • You aren't declaring $addr with 'my'. So it won't run under strictures.
    • Your 'die' output complains about HANDLE_IN but you open a filehandle called LISTING. Not a fatal error, but an inconsistancy nevertheless.
    • You're using $line and @line. Also not an error, but definately a possible source of confusion.
    • You're passing data straight from a CGI param into a 2-argument open, which could allow tainted (or malicious data) access to the shell.
    • You declare $count, but never use it.
    • You're assuming that the script's access permissions allow it to execute lynx.
    • You're slurping into an array, which wouldn't scale well if the output from lynx were to grow large.
    • If, in fact, this is the entire script, you've forgotten to 'use' the modules you're trying to use. It looks like CGI.pm is probably on the list of modules you need to 'use'. And you should definately be using the strict and warnings pragmas as well.

    Maybe you could tell us what the problem is that you're actually trying to track down... error messages, unexpected results, etc. Otherwise, start trying to cross things off the list above.


    Dave

[reply]
      Ah, quite right.

      There's nothing wrong with the script, except that it does not parse email addresses, which is why I only included the mail::address portion of it. It parses html tags.

      try it for yourself at http://www.markkahn.com/cgi-bin/mp3.pl

      with your favorite url in the submit box.

      My vision is that I am returned a list of addys of the page, but I get a de-markup-tagged page dump as output.

      Thanks for the responses.

[reply]
        FYI For anoyone interested in this thread: I wrote to the module manager, and he described mail::address as 'a fragile piece of software, not designed to strip email from source code.' He suggested using a regular expression.
[reply]
Re: parsing emails w/ MAIL::ADDRESS
by edan (Curate) on Feb 04, 2004 at 07:25 UTC

    Hmm. Let's see.

    • You forgot to add #!/usr/bin/perl at the top?
    • You named your subroutine ALL_CAPS
    • You have poor/no indentation
    • You call Mail::Address->parse() but you forgot to use Mail::Address?

    How about this. You tell us what's wrong, i.e. what does it do? What is different about what it does from what you want it to do? What exact error messages or unexpected output does it produce?

    --
    edan (formerly known as 3dan)

[reply]

Back to Seekers of Perl Wisdom