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)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.0
People
(Reporter: LpSolit, Assigned: LpSolit)
References
Details
(Keywords: dataloss, regression)
Attachments
(1 file)
912 bytes,
patch
|
Wurblzap
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•17 years ago
|
||
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?
Updated•17 years ago
|
Flags: blocking3.0? → blocking3.0+
Assignee | ||
Comment 2•17 years ago
|
||
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 3•17 years ago
|
||
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+
Updated•17 years ago
|
Flags: approval?
Assignee | ||
Updated•17 years ago
|
Attachment #251622 -
Flags: review?(wicked+bz)
Assignee | ||
Comment 4•17 years ago
|
||
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.
Description
•