686967 - Incoherent code in Bugzilla::BugUrl::Bugzilla::Local->should_handle()

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Frédéric Buclin]

Found in Bugzilla::BugUrl::Bugzilla::Local:

sub should_handle {
    my ($class, $uri) = @_;

    return $uri->as_string =~ m/^\w+$/ ? 1 : 0;

    my $canonical_local = URI->new($class->local_uri)->canonical;

    # Treating the domain case-insensitively and ignoring http(s)://
    return ($canonical_local->authority eq $uri->canonical->authority
            and $canonical_local->path eq $uri->canonical->path) ? 1 : 0;
}


Tell me how we are supposed to reach the code after the first return? :) Either the last part of the method is useless and should be removed, or the first return shouldn't be called unconditionally.

Comment 1

[Tiago Mello [:timello]]

Attachment: v1

It should be removed.

Comment 2

[Frédéric Buclin]

Comment on attachment 560929 [details] [diff] [review]
v1

> sub should_handle {
>     my ($class, $uri) = @_;
>-
>     return $uri->as_string =~ m/^\w+$/ ? 1 : 0;
>-
>-    my $canonical_local = URI->new($class->local_uri)->canonical;
>-
>-    # Treating the domain case-insensitively and ignoring http(s)://
>-    return ($canonical_local->authority eq $uri->canonical->authority
>-            and $canonical_local->path eq $uri->canonical->path) ? 1 : 0;
> }

As I understand it, a local bug is detected either because it matches /^\w+$/, or because it comes from the same urlbase as the local Bugzilla installation. This means we want to return early on if $uri->as_string =~ m/^\w+$/. Otherwise the 2nd check is still needed.

Comment 3

[Tiago Mello [:timello]]

Attachment: v2

Now it should handle just bug ids and local Bugzilla urls.

Comment 4

[Tiago Mello [:timello]]

s/just//

Comment 5

[Frédéric Buclin]

Comment on attachment 561027 [details] [diff] [review]
v2

Works fine. r=LpSolit

I just realized that when you add a local bug ID, the history of the other bug is not updated; only the current bug history is.

I will file a separate bug for that.

Comment 6

[Tiago Mello [:timello]]

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/                                                                                                             
modified Bugzilla/BugUrl/Bugzilla/Local.pm
Committed revision 7934.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/                                                                                                           
modified Bugzilla/BugUrl/Bugzilla/Local.pm
Committed revision 7966.