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: