556731 - Make Bugzilla::Milestone, Bugzilla::Version, and Bugzilla::Component use VALIDATOR_DEPENDENCIES instead of UPDATE_VALIDATORS

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

Now that we have VALIDATOR_DEPENDENCIES, the simplest classes to convert should be milestone, version, and component, since they only have one item in UPDATE_VALIDATORS.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Here we go. This is pretty straightforward, except that Version and Milestone were using "name" as the create() argument, when the database field is actually called "value". So I switched them to using "value" as their argument.

  In Bugzilla::Component, since it already had a create() function, I got rid of the run_create_validators implementation entirely, and just modified the parameters inside of create() (since I was already doing that anyway) and added a tiny enhancement where I set the "product" value of the created object to the $product object we already had.

Comment 2

[Frédéric Buclin]

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

There is no reason to s/name/value/ in create(). We use $foo->set_name() and $foo->name(), and passing "value" instead of "name" to create() is really confusing. I disagree with this change (and is not the goal of this bug).

Comment 3

[Max Kanat-Alexander]

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

It is entirely necessary--otherwise the validators will not work properly. The unfortunate fact of the implementation of create() and the nature of our database is that create() must take the database names of the fields. The fact that it wasn't was purely a hack, and this undoes the hack.

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
> It is entirely necessary--otherwise the validators will not work properly.

Why?

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> Why?

  During Bugzilla::Object::create, values are validated using VALIDATORS, using their database field name. During set, the same thing actually happens--the *database* field name is used. "name" is not the database field name.

  We could use a special hack before, because we had custom code in run_create_validators anyway. But now that run_create_validators is no longer necessary, I switched back to using the normal Bugzilla standard, which is more consistent with the rest of Bugzilla.

  If you want them to be named "name", we should change the database field name--something that we should do for consistency at some point anyhow, but not now.

Comment 6

[Frédéric Buclin]

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

>=== modified file 'Bugzilla/Milestone.pm'

> use constant REQUIRED_CREATE_FIELDS => qw(
>-    name
>+    value

You also have to update the POD at line 257 to reflect this change:

Bugzilla/Milestone.pm:256:    my $milestone = Bugzilla::Milestone->create(
Bugzilla/Milestone.pm-257-        { name => $name, product => $product, sortkey => $sortkey });


>=== modified file 'Bugzilla/Version.pm'

>-    $product = $invocant->product if (ref $invocant);
>+    

Remove extra whitespaces.


Otherwise looks good. r=LpSolit

Comment 7

[Frédéric Buclin]

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

>=== modified file 'Bugzilla/Milestone.pm'

>-    my $params = $class->SUPER::run_create_validators(@_);
>+    my ($params) = $class->SUPER::run_create_validators(@_);

This change doesn't make sense. run_create_validators() returns an hashref.


Please remove parens on checkin.

Comment 8

[Max Kanat-Alexander]

Okay, I did all three checkin fixes.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified editmilestones.cgi
modified editversions.cgi
modified Bugzilla/Component.pm
modified Bugzilla/Migrate.pm
modified Bugzilla/Milestone.pm
modified Bugzilla/Product.pm
modified Bugzilla/Version.pm
Committed revision 7169.