Closed
Bug 150153
Opened 22 years ago
Closed 22 years ago
ConnectToDatabase/quietly_check_login issues.
Categories
(Bugzilla :: Bugzilla-General, defect, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: CodeMachine, Assigned: bbaetz)
Details
Attachments
(2 files, 1 obsolete file)
2.62 KB,
patch
|
jouni
:
review+
jouni
:
review+
|
Details | Diff | Splinter Review |
13.13 KB,
patch
|
CodeMachine
:
review+
jouni
:
review+
|
Details | Diff | Splinter Review |
I did an audit of CGIs with regard to ConnectToDatabase and quietly_check_login. What I found was not good, and needs to be fixed for rc3/final. confirm_login is ok on it's own because it calls ConnectToDatabase itself. No ConnectToDatabase before quietly_check_login ----------------------------------------------- Results in the CGI dying in certain situations. xml.cgi: No CTD. colchange.cgi: CTD is done *after* qcl. No quietly_check_login ---------------------- Results in a logged out footer. quips.cgi token.cgi: Possibly not a bug, are there any situations in which it should? createaccount.cgi: Probably not a bug, given this shouldn't occur when you're logged in anyway. Stuff between ConnectToDatabase & quietly_check_login ----------------------------------------------------- Results in a logged out footer in some situations. buglist.cgi: Definitely a bug, try buglist.cgi?format=nonexistantformat. describecomponents.cgi, duplicates.cgi, sidebar.cgi: Call GetVersionTable in between, maybe not a bug at the moment, but could become one later if GetVersionTable calls Throw*Error or the like. queryhelp.cgi: Similar to above, called SendSQL, probably not a bug at the moment.
Reporter | ||
Updated•22 years ago
|
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
Reporter | ||
Comment 1•22 years ago
|
||
Also, the template initialisation code in globals.pl can fail, and throw a template error. This needs to qcl too.
Assignee | ||
Comment 2•22 years ago
|
||
OK, I looked at these independantly, and seem to have come up with the same list as mattyt. (I actually have one more; move.pl had confirm_login a bit too late for one error message) Re comment #1, we don't actually need to call quietly_check_login() in that case. What we need to do is make our code which is called when we can't create a template object not try to use templates. ThrowTemplateError is closer to what we want, but theres no 'second error' here. File a new bug on this issue, please. Patch coming soonish, which should be applied to 2.16 + trunk. I'm helping to 'test' this in conjunction to a second patch, which I don't know if we want on the branch. The second patch removes ConnectToDatabase calls from everywhere else in the code. I've mainly moved code, and have also moved GetVersionTable calls below login ones when appropriate, just for when that code starts having errors handled by templates.
Assignee: justdave → bbaetz
Assignee | ||
Comment 3•22 years ago
|
||
OK, try this. I've done basic testing, but others should look and test, too.
Assignee | ||
Comment 4•22 years ago
|
||
I'm not sure if this part should go into 2.16. Its unlikly this would break anything, but....
Comment 5•22 years ago
|
||
Comment on attachment 86929 [details] [diff] [review] v1, pt1 Nit: You're not being consistent with whether or not there is an empty line between CTD and qcl/c_l calls. Please fix this either way. I prefer CTD & qcl/c_l being close together and separated from the rest of the code by empty lines, but I'm just as fine with something else too. r=jouni otherwise :-) I think we should push part 2 to be a separate 2.18 bug, because it's somewhat different and potentially more risky. I'm ready to get part one into 2.16, but I'm not certain about part 2. It would need more testing time, and I don't think we're going to have it before release.
Attachment #86929 -
Flags: review+
Assignee | ||
Comment 6•22 years ago
|
||
OK, whitespace nits fixed. You can r= the second part, even if we're not taking it for 2.16.
Attachment #86929 -
Attachment is obsolete: true
Comment 7•22 years ago
|
||
>You can r= the second part, even if we're not taking it for 2.16.
I had a quick look at it but didn't have the time to check it out thoroughly
then. The same applies now: it still looks basically good but I don't think I'll
have the time to test it properly until Monday. If someone else gets there
before me, I'm grateful :-)
Reporter | ||
Updated•22 years ago
|
Attachment #87766 -
Flags: review+
Comment 8•22 years ago
|
||
Comment on attachment 87766 [details] [diff] [review] pt1, v2 r=jouni
Attachment #87766 -
Flags: review+
Comment 9•22 years ago
|
||
Comment on attachment 86930 [details] [diff] [review] pt2, v1 r=jouni, seems to work and code is clean. But still hesitant about the branch checkin. :-)
Attachment #86930 -
Flags: review+
Assignee | ||
Comment 10•22 years ago
|
||
Yeah, I won't check patch 2 in on the branch.
Assignee | ||
Comment 11•22 years ago
|
||
The first patch has been checked into the trunk and the branch; moving to 2.18 for the second patch, which still needs another review.
Status: NEW → ASSIGNED
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
Comment 12•22 years ago
|
||
Comment on attachment 86930 [details] [diff] [review] pt2, v1 Feh. Get this 2xr=jouni and shove it in. We'll see if it breaks, then. :-)
Attachment #86930 -
Flags: review+
Assignee | ||
Comment 13•22 years ago
|
||
Part 2 checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•