Closed Bug 367077 Opened 17 years ago Closed 17 years ago

Big attachments which should be stored locally are not

Categories

(Bugzilla :: Attachments & Requests, defect)

2.23.3
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Bugzilla 3.0

People

(Reporter: LpSolit, Assigned: LpSolit)

References

Details

(Keywords: dataloss, regression)

Attachments

(1 file)

If you turn on the "BigFile" attribute, _validate_data() returns 0 as we don't want to fill the DB with the content of the attachment:

$data = _validate_data($throw_error, $hr_vars) || return 0;

But this means that |return 0| is called and we leave Attachment::insert_attachment_for_bug() before we had a chance to save the attachment in data/attachments/.

This is a regression due to bug 5179.
Flags: blocking3.0?
There is something wrong in this code. How do you distinguish |$data = 0| because an error has been thrown in _validate_data() and |$data = 0| because "0" is the content of the attachment?
Flags: blocking3.0? → blocking3.0+
Attached patch patch, v1Splinter Review
If the attachment is considered as a "big file", $data eq ''. Else the attachment cannot be empty nor contain the single character "0", so we know that if $data eq '0', then an error has been thrown.

So instead of the current |$data || return|, we have to check
|($data ne '0') || return|.
Assignee: attach-and-request → LpSolit
Status: NEW → ASSIGNED
Attachment #251622 - Flags: review?(wurblzap)
Attachment #251622 - Flags: review?(wicked+bz)
Comment on attachment 251622 [details] [diff] [review]
patch, v1

Yup. Good catch. r=Wurblzap by inspection.

Nit: for clarity, I think you should make it |$data eq '0' && return 0;|.
I wonder whether we might consider changing the logic (maybe in another bug) so that $data is undef (instead of 0) if an error is thrown. The check here would be |defined($data) || return 0;| then (or even |[...] return undef;| if surrounding code is modified along with it).
Attachment #251622 - Flags: review?(wurblzap) → review+
Flags: approval?
Attachment #251622 - Flags: review?(wicked+bz)
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.44; previous revision: 1.43
done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: approval? → approval+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: