attachment.cgi should use ->set_all

ASSIGNED
Assigned to

Status

()

--
enhancement
ASSIGNED
7 years ago
5 years ago

People

(Reporter: bugzilla-mozilla, Assigned: sjoshi)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
Created attachment 585211 [details] [diff] [review]
Patch v1

Only changed the updating of an attachment.
Attachment #585211 - Flags: review?(LpSolit)

Comment 1

7 years ago
Comment on attachment 585211 [details] [diff] [review]
Patch v1

>=== modified file 'attachment.cgi'

>-        $attachment->set_is_patch(scalar $cgi->param('ispatch'));
>-        $attachment->set_content_type(scalar $cgi->param('contenttypeentry'));

It is important to call set_is_patch() before set_content_type(), because if a file is a patch, then the MIME type is enforced to be text/plain. This is enforced in Bugzilla::Attachment::VALIDATOR_DEPENDENCIES but for some reason, removing the is_patch attribute and editing its MIME type at the same time doesn't work. The new MIME type is ignored and left to text/plain.
Attachment #585211 - Flags: review?(LpSolit) → review-

Updated

7 years ago
Target Milestone: --- → Bugzilla 4.2

Updated

7 years ago
Target Milestone: Bugzilla 4.2 → Bugzilla 5.0

Comment 2

7 years ago
OK, I found the reason of the problem: set_all() checks dependencies via VALIDATOR_DEPENDENCIES, which looks at column *names* to determine dependencies. But in Attachment.pm, we have:

sub set_content_type { $_[0]->set('mimetype', $_[1]); }
sub set_is_patch     { $_[0]->set('ispatch', $_[1]); }
sub set_is_private   { $_[0]->set('isprivate', $_[1]); }
sub set_is_obsolete  { ...; $self->set('isobsolete', $obsolete); ... }

This means that the name of the method doesn't match the name of the column. For instance, set_content_type() should be renamed set_mimetype() for VALIDATOR_DEPENDENCIES *and* set_all() to point to the same method. This also means that you must pass column names as keys in set_all(). So content_type => ... must be mimetype => ..., etc....

Comment 3

6 years ago
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.
Target Milestone: Bugzilla 4.4 → ---
(Assignee)

Comment 4

5 years ago
Created attachment 805042 [details] [diff] [review]
Patch-v2
Assignee: bugzilla-mozilla → joshi_sunil
Attachment #585211 - Attachment is obsolete: true
Attachment #805042 - Flags: review?(simon)

Comment 5

5 years ago
Comment on attachment 805042 [details] [diff] [review]
Patch-v2

This breaks the Bugzilla.update_attachment RPC calls. Please wait until bug 917483 is committed before submitting another patch (other your patch will not apply cleanly)
Attachment #805042 - Flags: review?(simon) → review-
You need to log in before you can comment on or make changes to this bug.