302650 - Product.pm methods don't return what is expected, they are milestones, components and versions

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Gabriel Sales de Oliveira]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050608 Firefox/1.0 (Ubuntu package 1.0.4)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050608 Firefox/1.0 (Ubuntu package 1.0.4)

When i call a product method their methods ( milestones, components and
versions) don't return what is expected.

Reproducible: Always

Steps to Reproduce:
1.Instance a product object.
2.Call for methods milestones, components and versions
3.

Actual Results:  
don't return a hash.

Expected Results:  
return a hash with the method's object

Comment 1

[Max Kanat-Alexander]

I would expect them to return an array. What do the POD docs say?

Comment 2

[Frédéric Buclin]

*** Bug 302654 has been marked as a duplicate of this bug. ***

Comment 3

[Gabriel Sales de Oliveira]

Attachment: v1-change hash_refs to arrays

Yes it's supposed to return an array, this attachment may fix the problem.

Comment 4

[Frédéric Buclin]

Comment on attachment 191260 [details] [diff] [review]
v1-change hash_refs to arrays

>Index: Bugzilla/Component.pm

>-    my $hash_ref = Bugzilla::Component::get_components_by_product(1);
>+    my @array_ref = Bugzilla::Component::get_components_by_product(1);
>     my $component = $hash_ref->{1};

The "my $component = $hash_ref->{1};" line has to be removed as well.


>Index: Bugzilla/Product.pm

>-    return $self->{versions};
>+    return $self->{versions}

Oops, the semicolon has been removed by accident.


You also have to update the POD accordingly in all the concerned files; timello
forgot to update them. Replace '$hash_ref = ...' by '@array_ref = ...' as you
did in this patch. Otherwise, your patch looks good.

Comment 5

[Gabriel Sales de Oliveira]

Attachment: v2-Pod doc's updated

I update the pod doc's to the new scenario.

Comment 6

[Max Kanat-Alexander]

You know, it's easy to turn a hash into an array, but difficult to turn an array
into a hash.

Maybe we should just be returning hashes everywhere, in case we need them. I can
imagine situations where they would be useful. (For example, picking a component
out of $product->components.)

Comment 7

[Gabriel Sales de Oliveira]

Attachment: v3-changing plans

If the object has id, as components has, returns an hash else return a array of
hashes cause in the case of versions and milestones if we use hashes the ids
will be the values...

What do you think about that??

Comment 8

[Max Kanat-Alexander]

Comment on attachment 192233 [details] [diff] [review]
v3-changing plans

WHy not just modify the get_x_by_product functions to return an array_ref, if
you really want to change that?

Also, it's not an array of hashes, but an array of objects.

And don't disturb the spacing in the SYNOPSIS section. The = should be lined
up.

Comment 9

[Gabriel Sales de Oliveira]

Attachment: v4-patch

I changed the get_*_by_product and the Pod Doc's. 

Sorry about the Synopses on my last patch :(

Comment 10

[Frédéric Buclin]

Comment on attachment 192370 [details] [diff] [review]
v4-patch

>Index: Bugzilla/Version.pm

>     foreach my $value (@$values) {
>         push @versions, new Bugzilla::Version($product_id, $value);
>     }
>-    return @versions;
>+    return \@versions;
> }

editversions.cgi expects an array from get_versions_by_product() and we have a
good reason to require an array, see all the work done by timello on
editversions.cgi & co files (and by myself through reviews). Moreover, this
doesn't make sense to modify Version.pm only. As long as we have no real needs
for something else than arrays, please don't change values returned by these
routines.

Comment 11

[Frédéric Buclin]

Also, before submitting a patch, please test it first! I needed less than 10
seconds to check that editversions.cgi is now broken due to your patch.

Comment 12

[Gabriel Sales de Oliveira]

Attachment: v5-Product.pm fix

I think the better place to do the changes is Product.pm, catch versions and
milestones as arrays an pass an arrayref to the method.

If i'm wrong please give me some light... :)

Comment 13

[Frédéric Buclin]

Comment on attachment 193075 [details] [diff] [review]
v5-Product.pm fix


>- Description: Returns a hash with of all product versions.
>+ Description: Returns a array with of all product versions.


>- Description: Returns a hash with of all product milestones.
>+ Description: Returns a array with of all product milestones.

Nit: "Gets all valid versions/milestones for that product" sounds better to me.
r=LpSolit

Comment 14

[Frédéric Buclin]

Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.8; previous revision: 1.7
done