364254 - Add hook to Bugzilla::Template::quoteUrls

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Colin Ogilvie [:cso]]

As description - add a hook to Bugzilla::Template::quoteUrls

Comment 1

[Colin Ogilvie [:cso]]

Attachment: Patch v1

Comment 2

[Colin Ogilvie [:cso]]

Attachment: make_talkback_id_links extension

Implements Talkback IDs as an extension using this hook.

Comment 3

[Colin Ogilvie [:cso]]

Requesting Blocking 3.0 as this could be useful for the BMO upgrade as per Gerv's mail from earlier today.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 249021 [details] [diff] [review]
Patch v1

"things" and "count" are really unclear--what would I be using those for? Imagine I've never seen the quoteUrls code.

Also, isn't "count" just "scalar @things"?

Comment 5

[Max Kanat-Alexander]

It's an enhancement, so it's not a blocker. But it's just a hook, so we'll still take it for 3.0.

Comment 6

[Max Kanat-Alexander]

Attachment: v1

Okay, here we go. This should be simpler.

I may be adding that coderefs bit myself, fairly soon.

Comment 7

[Max Kanat-Alexander]

Attachment: v2

This one's slightly more useful--allows you to actually return some HTML and avoid various forms of confusion. :-)

Comment 8

[Max Kanat-Alexander]

Attachment: v3

And...one further fix to make it actually possible to return HTML.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 392428 [details] [diff] [review]
v3

Hold on, I'm updating this to be more flexible and useful, after more experience trying to use it.

Comment 10

[Max Kanat-Alexander]

Attachment: v4

Okay, this is far more flexible (and can actually do what I need for the extension that I'm writing) and is probably a nice improvement for Bugzilla's code anyway.

Comment 11

[David Lawrence [:dkl]]

(In reply to comment #10)
> Created an attachment (id=392670) [details]
> v4
> 
> Okay, this is far more flexible (and can actually do what I need for the
> extension that I'm writing) and is probably a nice improvement for Bugzilla's
> code anyway.

Hey. In reviewing this patch I decided to port over some of our customizations we use currently in quoteUrls into the new extension format and am having issue with getting it to work. 

Here is the exact code from Bugzilla::Template::quoteUrls:

$text =~ s~\b(issue\s*\#?\s*(\d+))
    ~($things[$count++] = "<a href=\"https://enterprise.redhat.com/issue-tracker/?module=issues&action=view&tid=$2\">$1</a>") &&
    ("\0\0" . ($count-1) . "\0\0")
    ~egmxi;

So I add it to a bug-format_comment.pl extension in the following way:

push(@$regexes, { match   => qr/\b(issue\s*#?\s*(\d+))/, 
                  replace => "<a href=\"https:\/\/enterprise.redhat.com\/issue-tracker\/\?module=issues&action=view&tid=\$2\">\$1<\/a>" });

Then I add the following comment:

This is a test bug for issue 45442 blah

When it uses the extension I added the comment becomes:

This is a test bug for $1 blah

where $1 is a hyperlink to specified URL except the tid is $2 so it is substituting the $1 and $2 exactly. I have escaped them as you described in your POD doc but it still doen't do it right. Any suggestions?

Dave

Comment 12

[Max Kanat-Alexander]

(In reply to comment #11)
> where $1 is a hyperlink to specified URL except the tid is $2 so it is
> substituting the $1 and $2 exactly. I have escaped them as you described in
> your POD doc but it still doen't do it right. Any suggestions?

  Hmm, strange. We might have to do some research on how to get Perl to parse $1, $2, etc. inside of a string in s///. Maybe we have to do eval(quotemeta($replace)) or something.

Comment 13

[Max Kanat-Alexander]

(In reply to comment #11)
>                   replace => "<a
> href=\"https:\/\/enterprise.redhat.com\/issue-tracker\/\?module=issues&action=view&tid=\$2\">\$1<\/a>"

  FWIW, this would be simpler as:

  replace => '<a href="https://enterprise.redhat.com/issue-tracker/?module=issues&amp;action=view&amp;tid=$2">$1</a>'

  Does it work like that, by any chance?

Comment 14

[Max Kanat-Alexander]

Hmm, I can't seem to quite figure out a way to get this to work, and neither can #perl, yet. So I think the solution is to add coderef support. (We just pass $1, $2, $3, $4, etc. to it.)

Comment 15

[David Lawrence [:dkl]]

(In reply to comment #13)
> (In reply to comment #11)
> >                   replace => "<a
> > href=\"https:\/\/enterprise.redhat.com\/issue-tracker\/\?module=issues&action=view&tid=\$2\">\$1<\/a>"
> 
>   FWIW, this would be simpler as:
> 
>   replace => '<a
> href="https://enterprise.redhat.com/issue-tracker/?module=issues&amp;action=view&amp;tid=$2">$1</a>'
> 
>   Does it work like that, by any chance?

No this does the same thing.

Dave

Comment 16

[Max Kanat-Alexander]

Yeah, there is a way to make this work, using /ee. We might be able to do it by doing something like $replace = 'qq{' . $replace . '}' and escaping any } that exists in $replace.

Comment 17

[Max Kanat-Alexander]

Attachment: v5

Okay, here we go, this allows you to return a coderef, which will be passed in the matches. Do you think there's anything else that we should pass in, or should we perhaps use named arguments so that we can expand the coderef in the future?

Comment 18

[David Lawrence [:dkl]]

Comment on attachment 401135 [details] [diff] [review]
v5

>Index: Bugzilla/Template.pm

>-    my $current_bugurl = $curr_bugid ? "show_bug.cgi?id=$curr_bugid" : "";
>+    my $current_bugurl = $bug ? ("show_bug.cgi?id=" . $bug->id) : "";

I do not see why you needed to put ("show_bug.cgi?id=" . $bug->id) in place of "show_bug.cgi?id=" . $bug->id
here as both will give you the same when assigning to a scalar.

As for the coderefs, I also think that it may be best for the future if you use a named param now instead of just passing the matched values in to the subroutine as a list. Maybe just call the param 'matches' and have it as a reference to a list representing the matched values.

Here is the code I used to test with in extensions/redhat/code/bug-format_comment.pl

 # Linkify CAN or CVE links IE: CAN-XXXX-XXXX CVE-XXXX-XXXX
my $cve_match = qr/\b(C(?:VE|AN)-\d{4}-\d{4})/;
my $cve_replace = sub {
    my ($cve) = @_;
    return qq{<a href="http://cve.mitre.org/cgi-bin/cvename.cgi?name=$cve">$cve</a>};
};
push(@$regexes, { match => $cve_match, replace => $cve_replace });

# This handles issue tracker links, must be "issue <id>" format.
my $it_match = qr/\b(issue\s*\#?\s*(\d+))/;
my $it_replace = sub {
    my ($text, $id) = @_;
    return qq{<a href="https://enterprise.redhat.com/issue-tracker/?module=issues&action=view&tid=$id">$text</a>};
};
push(@$regexes, { match => $it_match, replace => $it_replace });

So with the names param for example the $cve_replace would be:

my $cve_replace = sub {
    my ($params) = @_;
    my $cve = $params->{matches}[0];
    return qq{<a href="http://cve.mitre.org/cgi-bin/cvename.cgi?name=$cve">$cve</a>};
};

and $it_replace would be:

my $it_replace = sub {
    my ($params) = @_;
    my $text = $params->{matches}[0];
    my $id   = $params->{matches}[0];
    return qq{<a href="https://enterprise.redhat.com/issue-tracker/?module=issues&action=view&tid=$id">$text</a>};
};

Aside from that everything worked as expected and is a good solution. If you decide not to the named params then r=dkl.

Comment 19

[Max Kanat-Alexander]

(In reply to comment #18)
> I do not see why you needed to put ("show_bug.cgi?id=" . $bug->id) in place of
> "show_bug.cgi?id=" . $bug->id
> here as both will give you the same when assigning to a scalar.

  To make the precedence clear with the ternary (?:) operator.

> As for the coderefs, I also think that it may be best for the future if you use
> a named param now instead of just passing the matched values in to the
> subroutine as a list. Maybe just call the param 'matches' and have it as a
> reference to a list representing the matched values.

  Okay. I think that's a good idea, too.

Comment 20

[Max Kanat-Alexander]

(In reply to comment #18)
>  # Linkify CAN or CVE links IE: CAN-XXXX-XXXX CVE-XXXX-XXXX

  Mozilla does this too. Maybe you could publish this part as a public extension?

Comment 21

[Max Kanat-Alexander]

Attachment: v6

Okay, here we go! This uses a named argument, and I also made the architecture of the example hook a little better--I put the subroutine into a package, and I put an html_quote line in the subroutine, so that people will remember to html_quote their returned data.

Comment 22

[David Lawrence [:dkl]]

(In reply to comment #20)
> (In reply to comment #18)
> >  # Linkify CAN or CVE links IE: CAN-XXXX-XXXX CVE-XXXX-XXXX
> 
>   Mozilla does this too. Maybe you could publish this part as a public
> extension?

I would be glad to. Where would these be published to? I do not see a section for server side extensions at https://wiki.mozilla.org/Bugzilla:Addons yet. Also what format should it be distributed? tarball of the single pl file?

Dave

Comment 23

[Max Kanat-Alexander]

(In reply to comment #22)
> I do not see a section
> for server side extensions at https://wiki.mozilla.org/Bugzilla:Addons yet.

  Yeah, probably best to just create a section. I mean Testopia is a server-side extension, and it has a listing. Maybe just a Misc Server-Side Extensions section?

> Also what format should it be distributed? tarball of the single pl file?

  Tarball of a directory that can be untarred directly into the extensions/ directory.

Comment 24

[David Lawrence [:dkl]]

Comment on attachment 401945 [details] [diff] [review]
v6

Ok, finished testing the latest version and works perfectly for me. Will work on publishing some public extensions to the wiki for the CVE replacement. r=dkl

Comment 25

[Frédéric Buclin]

Did someone make sure there is no perf issue with this patch? It looks like quoteUrls() is going to look for extensions again and again.

Comment 26

[Max Kanat-Alexander]

I haven't proved any performance issue with extensions at all, actually.

Comment 27

[Max Kanat-Alexander]

Checking in Bugzilla/Hook.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Hook.pm,v  <--  Hook.pm
new revision: 1.31; previous revision: 1.30
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.110; previous revision: 1.109
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/extensions/example/code/bug-format_comment.pl,v
done
Checking in extensions/example/code/bug-format_comment.pl;
/cvsroot/mozilla/webtools/bugzilla/extensions/example/code/bug-format_comment.pl,v  <--  bug-format_comment.pl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/extensions/example/lib/Bugzilla/ExampleHook.pm,v
done
Checking in extensions/example/lib/Bugzilla/ExampleHook.pm;
/cvsroot/mozilla/webtools/bugzilla/extensions/example/lib/Bugzilla/ExampleHook.pm,v  <--  ExampleHook.pm
initial revision: 1.1
done
Checking in template/en/default/bug/comments.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/comments.html.tmpl,v  <--  comments.html.tmpl
new revision: 1.43; previous revision: 1.42
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.165; previous revision: 1.164
done

Comment 28

[David Lawrence [:dkl]]

(In reply to comment #25)
> Did someone make sure there is no perf issue with this patch? It looks like
> quoteUrls() is going to look for extensions again and again.

Given that the amount of times quoteUrls is called is not really changing, the extensions should in theory be no more of a performance hit than just putting the code snippetes in Bugzilla::Template::quoteUrls directly. Of course there is the overhead of hitting the file system to search through the all of the extensions for bug-format_comment.pl each time but that should be it. Maybe in the future a way to cache the extension code in memory could be possible if it proves to be a performance issue.

Dave