tags:

views:

94

answers:

5

Using Perl, I am trying to parse a bunch of XML files and trying to find any form of URL in the XML and print it. My regex does not seem to work and it is not returning any match. What am i missing?

sub findURL{
local($inputLine, $outText);
$inputLine = $_[1];
 while (length($inputLine) > 0)
 {
 if ($inputLine =~ /^(((http|https|ftp):\/\/)?([[a-zA-Z0-9]\-\.])+(\.)([[a-zA-Z0-9]]){2,4}([[a-zA-Z0-9]\/+=%&_\.~?\-]*))*$/ )

 {
 $outText .= $&;
 $inputLine = $';
 }
 else
 {
  $inputLine = "";
  $outText .= "";
 }
 }
 return $outText;
}
+11  A: 

use Regexp::Common

use Regexp::Common qw /URI/;

while (<>) {
    /$RE{URI}{HTTP}/       and  print "Contains an HTTP URI.\n";
}
Андрей Костенко
Libraries are always good. +1
Platinum Azure
A: 

I think it's what you think is a character class. For some reason that compiles, but the debug output shows something curious when I isolated the character class.

use strict;
use warnings;
use re 'debug';

my $re = qr/[[a-zA-Z0-9]\-\.]/;

And the debut output (from use re 'debug') shows this:

Compiling REx "[[a-zA-Z0-9]\-\.]"
Final program:
   1: ANYOF[0-9A-[a-z][] (12)
  12: EXACT <-.]> (14)
  14: END (0)
anchored "-.]" at 1 (checking anchored) stclass ANYOF[0-9A-[a-z][] minlen 4 

So it's looking for the literal string '-.]' as an "anchor". Thus if your hostname does not have '.-]' in it will never match. Thus it is like I said before, you're closing your character class with the first non-escaped ']'.

The best way to include a dash is to make it the last character of the class--so as to remove the possibility that it can indicate a range.

In addition, it should all just be one class. You actually close the class with the first non-escaped square-bracket close. Your character class should read:

[a-zA-Z0-9.-]

And that's all.

In addition, it's probably better practice to use named character classes as well:

[\p{IsAlnum}.-]
  • Another interesting thing I found out is that in ']' is interpreted as a literal square-close wherever a character class is not open. Thus you only need to escape it to avoid ending a character class, and thus, included it. Conversely '[[' will include '[' into the character class, thus there is no reason to escape a '[' unless outside of a character class.
Axeman
+8  A: 

Your code is seven different shades of wrong:

  • You shouldn't use a regex to parse XML (see this question)
  • local should probably not be used that way, you probably want my
  • The $&, $', and $` variables should not be used (use captures instead)
  • Your indenting is terrible
  • $inputLine = $_[1]; grabs the second argument to the function (what is the first?)
  • if you are going to use a regex, you should use the /g regex modifer, not roll your own multiple match code
  • your regex is capturing stuff it shouldn't (use (?:) for grouping, not ())

Here is how I would write your code if I didn't care that I would grab stuff I shouldn't and might miss stuff that I want (because a regex can't be smart enough to parse the XML). Note how the URL in the comment gets grabbed.

#!/usr/bin/perl

use strict;
use warnings;

use Regexp::Common qw/URI/;

sub find_urls {
    my $text = shift;
    return $text =~ /$RE{URI}{-keep}/g;
}

my $xml = do { local $/; <DATA> };

for my $url (find_urls($xml)) {
    print "$url\n";
}

__DATA__
<root>
    this is some text
    and a URL: http://foo.com/foo.html
    this isn't a URL http:notgrabbed.com
    <img src="http://example.com/img.jpg" />
    <!-- oops, shouldn't grab this one: ftp://bar.com/donotgrab -->
</root>
Chas. Owens
If he's just grepping XML for what looks like a URL, i don't think regexes are so bad. Parsing the *structure* of XML with regexes is a sin, but that doesn't seem to be what the OP wants.
Philip Potter
@Philip Potter But you will miss things that are URLs and find things that are commented out. If the XML is just a text file to him or her, then why bring up that it is XML?
Chas. Owens
@Chas then we need more domain specific knowledge. The use case will determine if a full XML parser is necessary or if that is just overkill. He probably brought up XML because normally any extra details are useful?
Philip Potter
@Philip Potter A parser is almost never overkill. I don't understand why people seem to think parsers are overkill but regexes are just fine. Parsers are generally easy to use and are often as easy or easier to use than a regex that is guaranteed to be broken. If you find your current parser hard to use, then it is time to switch. I like `XML::Twig`.
Chas. Owens
@Chas if the regex is guaranteed to be broken, use the parser, sure. But for grepping tasks, the regex is the most appropriate tool, particularly when you use Regexp::Common rather than roll your own. It's all about the most appropriate tool; and the mere presence of XML doesn't make a parser most appropriate.
Philip Potter
@Philip Potter If all you are doing is grepping, then it isn't an XML file, it is a text file whose contents happen to be XML. The distinction is important. It means the same code can be run against any text file. When you state that the file is XML, that means you expect it to be treated as such. An XML file may also have a URL broken out like: `<proto>http://</proto><dom>foo.com</dom><path>/foo.jpg</path>`; `$RE{URL}` doesn't think that is a URL, but you may actually care about it.
Chas. Owens
@Chas: I'm shocked you didn't link to [the canonical answer on why parsing XML/HTML with regexes is bad](http://stackoverflow.com/questions/1732348/regex-match-open-tags-except-xhtml-self-contained-tags/1732454#1732454) :)
Ether
@Chas: Since we're just going round and round, I'm going to say I agree with all 6 of your other bullet points and this is a fine answer.
Philip Potter
@Ether Now how am I supposed to vote/reputation-farm an answer I didn't give to a question I didn't ask? Seriously, that answer is funny, but not useful. I like my question (and its answers) better because they address the reasons why regexes shouldn't be used.
Chas. Owens
@Chas: :p` ` ` `
Ether
A: 

A few comments not directly related to your question, but to your code.

  1. I don't understand why you use local in the context you provided. My gut feeling is that you should use my instead of local.
  2. $inputLine = $_[1] actually means that you want to assign the second argument you pass to findURL to $inputline. Was it really what you intended?

About your regex:

Don't nest character classes: e.g [[a-zA-Z0-9]\-\.] should be replaced with [-a-zA-Z0-9.] (you need to put - first in order to avoid it being confused with the interval separator, and . does not need to be escaped inside a character class).

Replacing your regex with /^(((http|https|ftp):\/\/)?([-a-zA-Z0-9.])+(\.)([a-zA-Z0-9]){2,4}([-a-zA-Z0-9+=%&_.~?\/]*))*$/ works for me.

RFC3986 Appendix B provides a better regex of course.

zarkdav
+2  A: 

Use the URI::Find and URI::Find::Schemeless modules, available from the CPAN. For example

#! /usr/bin/perl

use warnings;
use strict;

use URI::Find;
use URI::Find::Schemeless;

my $xml = join "" => <DATA>;
URI::Find            ->new(sub { print "$_[1]\n" })->find(\$xml);
URI::Find::Schemeless->new(sub { print "$_[1]\n" })->find(\$xml);

__DATA__
<foo>
  <bar>http://stackoverflow.com/&lt;/bar&gt;
  <baz>www.perl.com</baz>
</foo>

Output:

http://stackoverflow.com/
www.perl.com
Greg Bacon