282632 - Eliminate AUTOLOAD section from BugMail.pm

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement, P3)

Description

[Max Kanat-Alexander]

There's an AUTOLOAD section in BugMail.pm. The comment above it says this:

# I got sick of adding &:: to everything.
# However, 'Yuck!'
# I can't require, cause that pulls it in only once, so it won't then be
# in the global package, and these aren't modules, so I can't use globals.pl
# Remove this evilness once our stuff uses real packages.

The AUTOLOAD statement really messes with the namespace -- I keep getting
inexplicable errors while I'm working on patches to BugMail.pm.

Enough stuff is in packages now that we don't need this thing.

Comment 1

[Max Kanat-Alexander]

Attachment: Fix up BugMail.pm to not use the AUTOLOAD

This makes life soo much better.

Comment 2

[Max Kanat-Alexander]

Attachment: Fix bitrot

Comment 3

[Jacob Steenhagen]

Comment on attachment 177551 [details] [diff] [review]
Fix bitrot

I haven't even come close to doing a complete review, but upon trying to add a
comment to a bug after applying this patch I got the following error:

undef error - Undefined subroutine &Bugzilla::BugMail::login_to_id called at
Bugzilla/BugMail.pm line 612.

Comment 4

[Max Kanat-Alexander]

Oh right, that's a bitrot fix I forgot to fix. :-) It's simple, I just need to
add a use Bugzilla::Bug.

Comment 5

[Max Kanat-Alexander]

Attachment: v1.2

OK, that should do it.

Comment 6

[Max Kanat-Alexander]

Attachment: v1.3

OK, yeah, I forgot Bugzilla::User, not Bugzilla::Bug. :-)

Comment 7

[Max Kanat-Alexander]

Comment on attachment 178768 [details] [diff] [review]
v1.3

I suspect this has possibly experienced a bit of bitrot, but it might still be
OK...

Comment 8

[Max Kanat-Alexander]

Attachment: v2

OK, here's the AUTOLOAD removal, but it creates a circular dependency between
Bugzilla::BugMail and Bugzilla::User. Normally circular dependencies aren't a
problem; I'm not sure why this one is. It causes a bunch of messages to be
printed like this when you try to compile BugMail:

Subroutine FormatTriple redefined at Bugzilla/BugMail.pm line 79.
Subroutine FormatDouble redefined at Bugzilla/BugMail.pm line 89.
Subroutine Send redefined at Bugzilla/BugMail.pm line 110.
Subroutine ProcessOneBug redefined at Bugzilla/BugMail.pm line 120.
Subroutine sendMail redefined at Bugzilla/BugMail.pm line 472.
Subroutine MessageToMTA redefined at Bugzilla/BugMail.pm line 606.
Subroutine PerformSubsts redefined at Bugzilla/BugMail.pm line 647.
Subroutine MailPassword redefined at Bugzilla/BugMail.pm line 654.

Comment 9

[Max Kanat-Alexander]

OK, the fix in bug 300334 seems to have fixed the errors I was getting, so
everything should be good, now. :-)

Comment 10

[Teemu Mannermaa (:wicked)]

Comment on attachment 188898 [details] [diff] [review]
v2

Yhh, those &:: make this code look bad. :( I don't think this is a big problem
because all lines  seem to refer to a sub in globals.pl and these are going to
go away RSN. Maybe this change even gives us nice incentive to kill globals.pl
sooner than later..

I tested both bugmail during bug changes and creation as well as flagmail. All
mailings seem to still work.

Hmm, looks like this patch will slightly bitrot the patch in the blocked bug.

>+use Bugzilla::User;
>+use Bugzilla;

Not even a nit: Any reason Bugzilla couldn't be first one, if it's needed at
all?

Comment 11

[Frédéric Buclin]

wicked, don't forget to request approval when you grant review.

Comment 12

[Max Kanat-Alexander]

Oh yeah, Bugzilla could be the first one. It would look cleaner, and be more
consistent with our other files. I can fix that on checkin.

Comment 13

[Teemu Mannermaa (:wicked)]

(In reply to comment #11)
> wicked, don't forget to request approval when you grant review.

I have usually let the assignee do that so that he/she can catch up on my review
comments if any. This was an option before but looking at reviewer guide now
indicates that process might have been changed since then. Now if only r+ on
attachment screen could do that automatically.. :)

Comment 14

[Dave Miller [:justdave]]

don't forget comment 12.

Comment 15

[Max Kanat-Alexander]

Did the checkin fix.

Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.41; previous revision: 1.40
done