Closed Bug 65164 Opened 24 years ago Closed 23 years ago

not sending </html>

Categories

(Bugzilla :: User Interface, defect, P3)

Product:
Bugzilla
Component:
User Interface
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.16

People

(Reporter: darin.moz, Assigned: kiko)

References

(
URL
)

Details

Attachments

(8 files)

Proposed patch
795 bytes, patch
Details | Diff | Splinter Review
new with non-closure option
863 bytes, patch
Details | Diff | Splinter Review
patch #3
511 bytes, patch
Details | Diff | Splinter Review
patch (array based)
887 bytes, patch
Details | Diff | Splinter Review
kiko_v1: simple patch, one optional parameter to avoid closing page
691 bytes, patch
Details | Diff | Splinter Review
kiko_v2: this is not my day
615 bytes, patch
Details | Diff | Splinter Review
kiko_v3: fixed per Jake's comments
692 bytes, patch
Details | Diff | Splinter Review
kiko_v3: for real this time
694 bytes, patch
gerv
: review+
jacob
: review+
Details | Diff | Splinter Review
i noticed the other day that bugzilla/buglist.cgi is not sending </html> at the end of the document. mozilla has a bug that causes it to throb on this (even though the socket connection has closed), so it is very noticeable.
Attached patch Proposed patch — — Splinter Review
Noticed that bugzilla is also not sending </BODY>. Fixed both of them in sub PutFooter. The only way I could see this breaking anything is if any non-serverpush text is being sent after PutFooter. I looked, but didn't see any.
Keywords: patch
This looks good to me. I am not sure why this was happening. I'll review it if someone can check it in. cc'ing gerv on this to see if he can commit. r= zach@zachlipton.com
It happens because the <HTML> and <BODY> is sent manually. The placement of closure blocks was merely neglected. I believe placing the fix in PutFooter should solve the problem in a more lasting manner.
Just a comment... I have one instance on my site where PutFooter is not the last thing on the page. Putting the closure tags in PutFooter would likely break those situations. (I have my index page rigged to display the PutFooter at the bottom of the left side - inside a table cell so it's to the left of the big ant picture :)
Okay, in that case, I guess I should split PutFooter into two functions: PutFooter, and PutPageClose (unless anyone's got a better idea?). Slap the "real" stuff in one, and put the page closer in the other. I'd really like to keep the closing in a function, as it keeps the CGI scripts closer to SCRIPTS than HTML pages. I'll get this done in a sec or two.
how about a compromise? I'd hate to see a separate function for it, then again, maybe that is the better way. How about an optional additional parameter for PutFooter that will suppress the closure tags if it's present?
I like that a whole bunch myself. Anyone have any objections?
Sorry about the delay. Damn real life. These people just pay me. Dunno why I actually do the work. <g>. The option to not close the HTML file is "--donotclosepage". Yay kludge!
Under Perl 5.6 and newer with 'use strict' the test for $close in there will fail with an undefined variable in the comparison if the close parameter is not passed. You have this: print "</BODY></HTML>\n" unless ($close eq "--donotclosepage"); What I would do is this: print "</BODY></HTML>\n" if (!$close); That way, if anything is passed as a parameter (other than a 0 or an empty string), it supresses the close, and if the caller doesn't pass anything, it doesn't throw an exception on the compare (since undef returns false if you look at it by itself). Might change the variable name to suit its purpose, too. (i.e. $dontclose)
Cool. My only problem with that approach is that it doesn't do so hot with future expansion (God forbid we have such). What do you think of print "</BODY></HTML>\n" if (!$close && $close eq "--donotclosepage"); At least that way we show that there IS a specific parameter, and this won't cause total goofiness if PutFooter is used in a list context by some unwitting person.
*** Bug 62505 has been marked as a duplicate of this bug. ***
Thank you! And will this fix also work for query.cgi ??
This bug is very highly visible when using Mozilla (see bug 53956), it has a patch attached but no target milestone set yet. Nominating for 2.12. Alan's last suggestion looks like the best way to me (although the logic of the if statement was slighly wrong). Adding another patch containing it.
URL: http://bugzilla.mozilla.org/buglist.cgi → http://bugzilla.mozilla.org/show_bug....
Keywords: review
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.12
Attached patch patch #3 — — Splinter Review
I'm with Dave in that I really don't think it's neccessary to pass the sub the exact text of --donotclosepage. Having said that... Alan, when you say that it doesn't leave room for future expansion, I assume you mean that at some poin we may wanna do something along the lines of: PutFooter("--donotclosepage --someotheroption"); If that's the case, the current if statement really doesn't leave room for that. Perhaps something like (I don't know for sure that this works or how it effects -w and perl 5.6): print "</BODY></HTML>\n" if ($close =~ /--donotclosepage/i); Also, if the variable is supposed to be a listing of options, it probably shouldn't be called $close ;) At this point the 2.12 list is pretty much closed. Moving to 2.16 (the next non-security release).
Target Milestone: Bugzilla 2.12 → Bugzilla 2.16
Ok, I admit that I did that patch a bit too late at night. Heh. Here's an updated patch using Stephen's code. This permits a bit more extensibility. Let me know if I'm being a moron again. Wouldn't be the first time.
Please note, that this is not a bug at all, if bugzilla would only declare to use 'HTML 4 transitional' correctly by including a doctype-declaration, because it is perfectly legal to drop the end tags for the 'body' and the 'html' element. See <http://www.w3.org/TR/html4/intro/sgmltut.html#h-3.2.1>! If the Mozilla browser has problems with this, this bug should be changed to one of the browser in 'standard-complience'.
While that is true, XHTML 1 sez that it's no longer optional (at least, that's my reading of the DTD). Not that that's a good reason to do this as opposed to include a DOCTYPE (which I think we should, but that's the subject of another bug entirely), but it does show which direction the W3Cs moving towards, and I'd rather go with the future than the current. Also, I didn't look up whether the strict dtd declared </head> as optional.
Priority: -- → P3
-> Bugzilla product, User Interface component, reassigning.
Assignee: tara → myk
Component: Bugzilla → User Interface
Product: Webtools → Bugzilla
Version: other → unspecified
Hmm. Can I say I'm very opposed to this --donotclosepage thing? This isn't done _anywhere_ in Bugzilla, and, well, what do we need it for? I'm all in favour of a complete API, but when it's necessary. We didn't even have a parameter to start off with :-) Okay, let's get this in, this is annoying and confusing.
Assignee: myk → kiko
Status: NEW → ASSIGNED
Christian, excuse me for grinching, but I fail to see how your patch to userprefs will affect CGI.pl and thus the closing of the page. ;-) As for whether or not it is used in Bugzilla anywhere...don't confuse Bugzilla the product with b.m.o the implementation. See Dave Miller's comments below. However, I completely agree with you in regards to "let's get this patch in".
How can you possibly agree with a moron like me? I've attached the wrong patch. Will fix. As per Justdave's comment, my patch does address his problem. I know bmo != bugzilla. :-)
Blocks: 47251
Works fine for me on my test installation. r=dkl@redhat.com
No longer blocks: 47251
Blocks: 47251
Comment on attachment 52456 [details] [diff] [review] kiko_v2: this is not my day >Index: CGI.pl ... >+# Putfooter echoes footerhtml and closes page. >+# PutFooter [ boolean dontclosepage ] >+# pass dontclosepage to avoid sending closing tags. These comments seem a bit confusing... >+ my @options = @_; I'd personally prefer somthing like: my ($dontclosepage) = @_; > print PerformSubsts(Param("footerhtml")); > SyncAnyPendingShadowChanges(); >+ >+ print "</BODY></HTML>\n" if ( ! $options[0] ) The </BODY></HTML> stuff really should appear above the SyncAnyPendingShadowChanges() call (wow, not that's quite a subroutine name ;) as Sync...() doesn't output anything we want to seen on the page (and shouldn't output anything at all). Also, should that be </body></html> (lower case)? Or am I smoking crack?
> These comments seem a bit confusing... Fixed. Better? > >+ my @options = @_; > > I'd personally prefer somthing like: > my ($dontclosepage) = @_; Fixed. > > print PerformSubsts(Param("footerhtml")); > > SyncAnyPendingShadowChanges(); > >+ > >+ print "</BODY></HTML>\n" if ( ! $options[0] ) > > The </BODY></HTML> stuff really should appear above the > SyncAnyPendingShadowChanges() call (wow, not that's quite a subroutine > name ;) as Sync...() doesn't output anything we want to seen on the > page (and shouldn't output anything at all). Fixed. > > Also, should that be </body></html> (lower case)? Or am I smoking > crack? Don't know about the crack, but that's fixed too.
Comment on attachment 52641 [details] [diff] [review] kiko_v3: for real this time r=gerv. Gerv
Attachment #52641 - Flags: review+
Comment on attachment 52641 [details] [diff] [review] kiko_v3: for real this time Works for me :) r=jake
Attachment #52641 - Flags: review+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: