Right now Bugzilla::Bug has a lot of subroutines, and there are a lot of modules and scripts that only use Bugzilla::Bug in order to get those subroutines. Instead, I'd like people to only have to directly use Bugzilla::Bug if they want to actually create a Bug object. My proposed solution is to create a Bugzilla::Bug::Util module that has all the static subroutines from Bugzilla::Bug, and then people can just use that if they want. Bugzilla::Bug::Util should itself have a very limited set of dependencies.

This patch creates Bugzilla::Bug::Util, but doesn't fix up the scripts to actually use it. I made POD for Bugzilla::Bug::Util.

Comment on attachment 213961 [details] [diff] [review] Changes to create Bugzilla::Bug::Util Kevin, could you look at the POD for me?

Comment on attachment 213961 [details] [diff] [review] Changes to create Bugzilla::Bug::Util >+ Returns: nothing >+ >+=cut >+sub AppendComment { >+ my ($bugid, $whoid, $comment, $isprivate, $timestamp, $work_time) = @_; This patch alone is not very useful as I cannot test it without .cgi being fixed too. Anyway, your patch doesn't pass tests, as expected: *** WARNING: line containing nothing but whitespace in paragraph at line 57 in file Bugzilla/Bug/Util.pm *** ERROR: Spurious text after =cut at line 119 in file Bugzilla/Bug/Util.pm *** ERROR: Spurious text after =cut at line 189 in file Bugzilla/Bug/Util.pm *** ERROR: Spurious text after =cut at line 249 in file Bugzilla/Bug/Util.pm *** ERROR: Spurious text after =cut at line 296 in file Bugzilla/Bug/Util.pm *** ERROR: Spurious text after =cut at line 636 in file Bugzilla/Bug/Util.pm *** ERROR: Spurious text after =cut at line 703 in file Bugzilla/Bug/Util.pm *** ERROR: Spurious text after =cut at line 768 in file Bugzilla/Bug/Util.pm *** ERROR: Spurious text after =cut at line 850 in file Bugzilla/Bug/Util.pm *** ERROR: Spurious text after =cut at line 878 in file Bugzilla/Bug/Util.pm not ok 116 - Bugzilla/Bug/Util.pm has incorrect POD syntax --ERROR This is because you must have a newline after each =cut. And you also have a line which only contains whitespaces. FYI, I won't review a patch which doesn't fix dependencies too, i.e. .cgi and other .pm files, as you cannot check in this one alone.

Comment on attachment 213961 [details] [diff] [review] Changes to create Bugzilla::Bug::Util I haven't finished reading through the POD, however, I can see right away that there's a problem with the =cut lines. They must be followed with a blank line. I'll get back to you with more later today.

Okay, this version contains all the CGI changes and passes runtests.

Hrm... the goal of this bug is to remove dependency loops between Perl modules, but I don't see any dependency being removed in this patch. Could you show me where the benefit is? Moreover, you should keep GetComments() in Bug.pm itself so that Bug.pm doesn't need to use Bug/Util.pm; Bug::longdescs() uses it (and you forgot to update this method anyway).

(In reply to comment #6) > Hrm... the goal of this bug is to remove dependency loops between Perl > modules, but I don't see any dependency being removed in this patch. Could you > show me where the benefit is? In the bug that we block, which can't go in without this patch. > Moreover, you should keep GetComments() in Bug.pm itself so that Bug.pm > doesn't need to use Bug/Util.pm; I think Bug.pm Would have to use Bug::Util anyhow. Bug::Util has things like ValidateBugID in it.

(In reply to comment #7) > I think Bug.pm Would have to use Bug::Util anyhow. Bug::Util has things like > ValidateBugID in it. > No, Bug.pm doesn't use ValidateBugID() itself.

Comment on attachment 214043 [details] [diff] [review] v1 Thinking about this bug a bit more, I think we should: 1) Keep GetComments() in Bug.pm, and even merge its code with longdescs(). The only place where GetComments() is used is in process_bug.cgi: $vars->{'comments'} = Bugzilla::Bug::GetComments($id, "oldest_to_newest"); IMO, we should now write: my $bug = new Bugzilla::Bug($id); $vars->{'comments'} = $bug->longdescs; or even pass the bug object to the template. 2) Keep GetBugActivity() in Bug.pm too, and convert it to a bug method. This function is used in show_activity.cgi and process_bug.cgi only. We should now write: my $bug = new Bugzilla::Bug($bug_id); my $activity = $bug->history(); 3) Keep AppendComment() in Bug.pm and make it a method of the bug object: $bug->add_comment(). I think the remaining functions can be moved into Bug/Util.pm. I think this would even remove the Bug - Bug::Util dependency.

(In reply to comment #9) > 1) Keep GetComments() in Bug.pm, and even merge its code with longdescs(). The > only place where GetComments() is used is in process_bug.cgi: > > $vars->{'comments'} = Bugzilla::Bug::GetComments($id, "oldest_to_newest"); > > IMO, we should now write: > > my $bug = new Bugzilla::Bug($id); > $vars->{'comments'} = $bug->longdescs; > > or even pass the bug object to the template. Yeah. We just need to modify the longdescs accessor so that it can do what we need, I think. > 2) Keep GetBugActivity() in Bug.pm too, and convert it to a bug method. This > function is used in show_activity.cgi and process_bug.cgi only. We should now > write: > > my $bug = new Bugzilla::Bug($bug_id); > my $activity = $bug->history(); That's a good idea, but GetBugActivity is used in a lot of places. That would be a pretty significant modification. I need this Bugzilla::Bug::Util in order to eliminate versioncache. I don't want to do more rearchitecture that I have to. I've already had to rewrite Bugzilla::Auth. > 3) Keep AppendComment() in Bug.pm and make it a method of the bug object: > $bug->add_comment(). Yeah, we have a bug for that already: bug 283926

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set "blocking3.2" tp "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved. I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

The creation of Bugzilla::Bug::Util might not happen, but I think we could steal the POD from this patch and put it into Bugzilla::Bug. It would need checking for accuracy, though. Gerv