Please add support for adding github issues in the 'See Also' field. Those take URL in the form: https://github.com/<user>/<project>/issues/<n> where <user> & <project> would be \S+?, and <n> would be \d+.

Comment on attachment 578698 [details] [diff] [review] Add support for github issues, v1 Just a side comment: > + # While GitHub URLs can be either HTTP or HTTPS, > + # always go with the HTTPS scheme, as that's the default. That's not exactly true, I'd say. Github URLs are always https; http always redirects.

How's this?

Comment on attachment 578772 [details] [diff] [review] Add support for github issues, v2 Review of attachment 578772 [details] [diff] [review]: ----------------------------------------------------------------- ::: Bugzilla/BugUrl/GitHub.pm @@ +50,5 @@ > + $username = $1; > + $repository_name = $2; > + $issue_id = $3; > + } else { > + ThrowUserError('bug_url_invalid', { url => $value }); Same as GetSatisfaction, should_handle could look for the right pattern and then we let class_for throw the error.

I was following the examples of Debian.pm, Google.pm, Launchpad.pm, and Sourceforge.pm Should those also have a more complete pattern in should_handle?

CC'ing timello so that he can reply to Matt's question in comment 5.

(In reply to Matt Selsky [:selsky] from comment #5) > I was following the examples of Debian.pm, Google.pm, Launchpad.pm, and > Sourceforge.pm > > Should those also have a more complete pattern in should_handle? You know what? If class_for already does that why we are repeating code in the subclasses? Also, I think the reason => 'show_bug' only applies to Bugzillas... and we can't say if the 'id' is whether valid or not, unless per the pattern... So I think we could: 1. Move the content of [% IF reason == 'show_bug' %] in the user-error template to an [% ELSE %] and then no need to pass 'reason' in the class_for. 2. Completely remove the reason 'id' and 'show_bug' because we are going to catch that in the pattern of the should_handle method. I think it's enough to say that the url must be a valid url from any of the listed bug trackers.

So, the _check_value method would only be in charge of returning the URI object with the shortest url form for the bug tracker...

+1 this would be fantastic for b2g and pdf.js, which span across github and bugzilla.

Updated should_handle as per comment 7. Should _check_value be renamed to _canonify_url, or similar, since that's the purpose as per comment 8?

Don't use Bugzilla::{Util,Error} since those are only called in the parent class.

(In reply to Matt Selsky [:selsky] from comment #10) > Created attachment 588800 [details] [diff] [review] > Add support for github issues, v3 > > Updated should_handle as per comment 7. Should _check_value be renamed to > _canonify_url, or similar, since that's the purpose as per comment 8? No, _check_[something] is the default name for every validator in Bugzilla even though it is not actually 'validating' anything.

Comment on attachment 588805 [details] [diff] [review] Add support for github issues, v4 >=== added file 'Bugzilla/BugUrl/GitHub.pm' + >+sub should_handle { >+ my ($class, $uri) = @_; >+ >+ # GitHub issue URLs have only one form: >+ # https://github.com/USER_OR_TEAM_OR_ORGANIZATION_NAME/REPOSITORY_NAME/issues/111 >+ return ($uri->authority =~ /^github.com$/i >+ and $uri->path =~ m|^/[^/]+/[^/]+/issues/\d+$|) ? 1 : 0; Ah! This is where that GetSatisfaction code came from :) >+} >+ >+sub _check_value { >+ my $class = shift; >+ >+ my $uri = $class->SUPER::_check_value(@_); >+ >+ # GitHub HTTP URLs simply redirect to HTTPS, so just use >+ # the HTTPS scheme. >+ if ($uri->scheme eq 'http') { >+ $uri->scheme('https'); >+ } I agree, this is doing the exactly the same as GetSatisfaction does. Lets create a common method and use it here. Maybe as you suggested _canonify_url and call it here. Otherwise, it looks good.

Matt, are you still available to update the patch? We'd really like this on b.m.o.

Simplify the scheme forcing.

Comment on attachment 609614 [details] [diff] [review] Add support for github issues, v5 The patch should be changed to add GitHub in the MoreBugUrl extension.

I think GitHub is popular enough to be in the core code.

Comment on attachment 609614 [details] [diff] [review] Add support for github issues, v5 So, otherwise the patch looks good.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/ modified Bugzilla/BugUrl.pm added Bugzilla/BugUrl/GitHub.pm modified template/en/default/global/user-error.html.tmpl Committed revision 8202.

Added to relnotes for 4.4.