307688 - Move the OS and platform detection routines out of enter_bug.cgi

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Frédéric Buclin]

pickplatform and pickos should really be in their own .pm module instead of
being in enter_bug.cgi.

Comment 1

[:glob ✱]

Attachment: patch v1

this extracts the user agent detection methods out of enter_bug into their own module.  i also refactored it to make it sane.

Comment 2

[:glob ✱]

Attachment: patch v2

this revision uses List::MoreUtils instead of Tie::IxHash to avoid introducing a new dependency.

Comment 3

[:glob ✱]

Comment on attachment 547425 [details] [diff] [review]
patch v2

oops; this patch has a copy+paste problem.

Comment 4

[:glob ✱]

Attachment: patch v3

Comment 5

[Max Kanat-Alexander]

Comment on attachment 547427 [details] [diff] [review]
patch v3

Review of attachment 547427 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you so much for doing this; this is a huge improvement.

::: Bugzilla/UserAgent.pm
@@ +27,5 @@
> +
> +package Bugzilla::UserAgent;
> +
> +use strict;
> +use base 'Exporter';

It'd be nice to do qw(Exporter) for consistency with other modules.

@@ +28,5 @@
> +package Bugzilla::UserAgent;
> +
> +use strict;
> +use base 'Exporter';
> +@Bugzilla::UserAgent::EXPORT = qw(detect_platform detect_op_sys);

That should be "our @EXPORT"

@@ +32,5 @@
> +@Bugzilla::UserAgent::EXPORT = qw(detect_platform detect_op_sys);
> +
> +use List::MoreUtils qw(natatime);
> +
> +# Using Tie::IxHash to keep the keys in order

I actually don't see a Tie::IxHash usage here.

@@ +33,5 @@
> +
> +use List::MoreUtils qw(natatime);
> +
> +# Using Tie::IxHash to keep the keys in order
> +my @platforms_map = (

You can't use "my" on a package level in mod_perl; the variable will disappear. (You have to use "our.") Anyhow, shouldn't this be a constant?

@@ +203,5 @@
> +    my $dbh = Bugzilla->dbh;
> +    foreach my $value (@values) {
> +        return $value if $dbh->selectrow_array(
> +            "SELECT 1 FROM $field WHERE value = ?", undef, $value); 
> +    }

I think you're going to be better off, performance-wise, doing Bugzilla->fields and asking for legal_values (which will probably get pulled in at some point anyway, and so saves us extra DB trips).

@@ +204,5 @@
> +    foreach my $value (@values) {
> +        return $value if $dbh->selectrow_array(
> +            "SELECT 1 FROM $field WHERE value = ?", undef, $value); 
> +    }
> +    return 'Other';

That should be a constant.

Comment 6

[:glob ✱]

Attachment: patch v4

Comment 7

[Max Kanat-Alexander]

Comment on attachment 547621 [details] [diff] [review]
patch v4

Review of attachment 547621 [details] [diff] [review]:
-----------------------------------------------------------------

This is so awesome. :-)

::: Bugzilla/UserAgent.pm
@@ +22,5 @@
> +#   Joe Robins <[e-mail]>
> +#   Gervase Markham <[e-mail]>
> +#   Shane H. W. Travis <[e-mail]>
> +#   Nitish Bezzala <[e-mail]>
> +#   Byron Jones <[e-mail]>

timeless actually wrote quite a bit of this code; he should probably be credited.

Comment 8

[:glob ✱]

(In reply to comment #7)
> timeless actually wrote quite a bit of this code; he should probably be
> credited.

agreed, however timeless hasn't been credited in any file in bugzilla, so i suspect he's made a conscious decision to be excluded.  of course i'm happy to include him if he desires.

Comment 9

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified enter_bug.cgi
added Bugzilla/UserAgent.pm
Committed revision 7901.