309749 - Remove get_x_by_y functions from the new .pm files, in favor of object methods

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P2)

Description

[Max Kanat-Alexander]

I originally wanted all SQL relating to Products to be in Bugzilla::Product, all
SQL relating to components to be in Bugzilla::Component, etc.

However, that was before I realized that perl has problems with dependency
loops. Now that I know that, I realize I had a bad goal.

So, I don't care where SQL goes. What I care about is that people use *object
methods* and not *subroutines*.

So, all functions of the type Bugzilla::Component::get_components_by_product
should be removed entirely. The code for that function should go entirely into
the "components" method of Bugzilla::Product.

The same for all other get_x_by_y functions. They encourage bad programming
practices, and confuse developers who don't know which choice to make between
the object methods and the functions.

Comment 1

[Frédéric Buclin]

You cannot avoid all subroutines, such as check_*. About methods versus
subroutines, this won't fix all loops, as $component->product and
$product->components would create loops too (and the situation is even worse
when considering classifications).

These subroutines allowed us to do a great work so far about updating the code
of editcomponents.cgi & co, so we didn't waste our time. But I agree with you
and we could think about removing all these get_x_by_y functions (assuming they
have a method equivalent to them), but that's not critical so far. ;)

Comment 2

[Max Kanat-Alexander]

Yeah. I'm not suggesting removing any subroutines other than the get_x_by_y ones
-- check_product and new() and so on, we need those. :-)

But yeah, we need to decide whether people get to know about their children, or
they get to know about their parents. Because of silly dependency loops, they
can't know about both. :-( (Unless we do things like Bugzilla::Product->new and
require the caller to have already said "use Bugzilla::Product" before "use
Bugzilla::Component" and so on.)

I think that usually, the best thing to do is to have parents know about their
children, but I guess we'll have to see what works best for each object.

Comment 3

[Frédéric Buclin]

(In reply to comment #2)
> But yeah, we need to decide whether people get to know about their children, or
> they get to know about their parents.
> I think that usually, the best thing to do is to have parents know about their
> children

Yes, I agree. Moreover, parents usually have several children, so
$classification->products is much more useful than $product->classification
(especially when passing the object to templates).

I was thinking about this problem too, and so far, what I note is that products
are the central place for now (I mean the object from which you can know
everything, either about childrens or parents), but maybe we should shift the
central role to classifications? It's hard to say.

Milestone's, version's and component's names are not unique (or in other words,
are unique *per* product only), which makes products the logical central place
to start from. Moreover, tables in our DB do not mention classifications at all
(for the three tables mentioned above), which would make the classification a
bad central place. So maybe what we did so far is fine? But if that's true, it
looks like classification objects have no reason to exist, as we already know
everything from $product->classification?! Again, it's hard to say.

Comment 4

[Max Kanat-Alexander]

Yeah, I think it might be safe to go with $product knowing about both parents
and children, for the moment, and not having Bugzilla::Classification know about
Products at all. In the future, if our needs change, it should perhaps be
possible to switch them back... I'm not totally certain.

I think we need to go over all the code that currently uses Bugzilla::Product
and Bugzilla::Classification, and see if we could get away with $product not
having an ->classification sub. That is, if we can always just get the
$classification if we need it, some other way.

Comment 5

[Frédéric Buclin]

Note bug 306601

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> Note bug 306601

  OK, then. Let's just go with $classification->products and
$product->components. I think that's a better design, anyway.

Comment 7

[André Batosti]

Attachment: batosti_v1

Comment 8

[Frédéric Buclin]

Comment on attachment 197774 [details] [diff] [review]
batosti_v1

>Index: editcomponents.cgi

>+    my $components = $product->components;
>     $vars->{'product'} = $product->name;
>+    $vars->{'components'} = $components;

Better is to pass $product directly and use it in templates.



>Index: editmilestones.cgi

>+    my $milestones = $product->milestones;
>     $vars->{'product'} = $product->name;
>+    $vars->{'milestones'} = $milestones;
>     $vars->{'default_milestone'} = $product->default_milestone;

Same remark here.



>Index: editversions.cgi

>+    my $versions = $product->versions;
>     $vars->{'product'} = $product->name;
>+    $vars->{'versions'} = $versions;

Same here.



>Index: Bugzilla/Product.pm

> sub group_controls {

Nit: Isn't there a better way of doing this instead of enumerating all columns
of the 'groups' table?

Comment 9

[André Batosti]

Attachment: batosti_v2: some changes in templates to use objects and his methods

Comment 10

[Frédéric Buclin]

Comment on attachment 197774 [details] [diff] [review]
batosti_v1

Per discussion with batosti on IRC, it appears that passing objects to
templates would require to do it in all files, making the patch much bigger and
is out of the scope of this bug.

>Index: editcomponents.cgi

> unless ($action) {
> 
>+    my $components = $product->components;
> 
>+    $vars->{'components'} = $components;

Nit: you could write: $vars->{'components'} = $product->components;

Same remark in other two .cgi scripts.


The patch works fine. r=LpSolit

Comment 11

[André Batosti]

Attachment: batosti_v1_fix

Comment 12

[Frédéric Buclin]

Comment on attachment 198629 [details] [diff] [review]
batosti_v1_fix

great! thanks! r=LpSolit

Comment 13

[Max Kanat-Alexander]

Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <-- 
editcomponents.cginew revision: 1.62; previous revision: 1.61
done
Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <-- 
editmilestones.cginew revision: 1.45; previous revision: 1.44
done
Checking in editversions.cgi;
/cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v  <--  editversions.cgi
new revision: 1.39; previous revision: 1.38
done
Checking in Bugzilla/Component.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Component.pm,v  <--  Component.pm
new revision: 1.8; previous revision: 1.7
done
Checking in Bugzilla/Group.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Group.pm,v  <--  Group.pm
new revision: 1.9; previous revision: 1.8
done
Checking in Bugzilla/Milestone.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Milestone.pm,v  <--  Milestone.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.10; previous revision: 1.9
done
Checking in Bugzilla/Version.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Version.pm,v  <--  Version.pm
new revision: 1.7; previous revision: 1.6
done