304699 - Implement real objects in Flag.pm and FlagType.pm, and remove the Flag-Attachment dependency loop

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement, P1)

Description

[Frédéric Buclin]

Bugzilla::Attachment::query() should call new Bugzilla::Attachment for each
attachment belonging to a given bug and most of the code inside query() should
be moved into new().

Also, Flag.pm should not depend on Attachment.pm and we should add a new
GetAttachment() function into Flag.pm itself, avoiding the Flag - Attachment
dependency loop, as it's done already for Bugzilla::Flag::GetBug().

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

To test Attachment.pm, look at the attachments table in any bug.

Comment 2

[Frédéric Buclin]

Attachment: patch, v2

I forgot to fix filterexceptions.pl: attachid -> attach_id. No other changes.

Comment 3

[Frédéric Buclin]

Comment on attachment 192763 [details] [diff] [review]
patch, v2

per discussion with myk and justdave on IRC, it appears that the best fix is to
use Flag objects which we call from Bug.pm and Attachment.pm. This way, there
is no need to do direct call to Flag.pm from process_bug.cgi and
attachment.cgi.

(03:12:45) myk: justdave, LpSolit: i agree; passing objects around does seem
cleaner, and a function like, say, $bug->create_flag() or
$attachment->create_flag() along with $bug->flags and $attachment->flags
probably makes the most sense

Comment 4

[Frédéric Buclin]

I won't have time to play with it before the 2.22 freeze

Comment 5

[Max Kanat-Alexander]

We still want this for 2.24 though, right?

Comment 6

[Frédéric Buclin]

right

Comment 7

[Frédéric Buclin]

myk did a great job in bug 302699 when rewritting Attachment.pm. Now it's my turn with Flag.pm and FlagType.pm. This is going to be my bigger contribution for 2.24...

Comment 8

[Frédéric Buclin]

(In reply to comment #7)
> myk did a great job in bug 302699 

I meant bug 302669 :(

Comment 9

[Frédéric Buclin]

This no longer blocks bug 304599. The Attachment <-> Flag dependency loop has been removed as part of bug 339750.

Comment 10

[Frédéric Buclin]

OK, I consider this rewrite as complete. The only remaining improvement I could see is to avoid having to pass the bug and attachment objects to most Flag::* routines, but that's definitely a minor point which I won't fix till I really have a good idea on how to do that very cleanly. As most of these routines are internal, it doesn't really matter for now.