100092 - Template process failed error prints content-type to browser window.

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P5)

Description

[Matthew Tuck [:CodeMachine]]

When the template process fails on editattachstatuses.cgi (say because of bad
permissions to the template), the content type gets written twice, and it
appears once on the page as a result.

Comment 1

[Gervase Markham [:gerv]]

This is a general problem with all the code we have that makes templates - we
are using DisplayError() to display the error, and it prints its own headers.

Surely the solution here is to move the headers into the template. I've tried
this on query.cgi and it works. We don't want to be sending rdf templates as
text/html anyway.

Gerv

Comment 2

[Matthew Tuck [:CodeMachine]]

That's one possible solution.  Another is to have a variable called
$headers_written or something that is checked by DisplayError and such.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

No, the solution is not to move the headers into the template, since most web
server interface libraries (our custom hacked one excepted) separate out header
from content generation.  DisplayError generates HTTP headers because it is
intended to be used during logic processing, during which time no content should
have been returned to the user.  We should either modify it in the way Matty
says or create another function for returning errors that occur during content
generation.

Comment 4

[Gervase Markham [:gerv]]

Or we could use functions to write the headers (WriteHeader("text/html")) and
store the header state in a global variable.

Or have two error functions. I like that better than passing a parameter. If you
want to throw an error half-way through a header, you complete it and then call
the "already done" error handler.

DisplayErrorPage() and DisplayError()?

Gerv

Comment 5

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 6

[Matthew Tuck [:CodeMachine]]

What if we are generating an XML template and it fails?

Comment 7

[Gervase Markham [:gerv]]

Having discussed this with Myk in the newsgroups, I now think we need the
following sorts of error function:

UserError("message"). Prints content type, and uses a template to format the
error message. Called anywhere in any CGIs.

CodeError("message"). Also attempts to use a template, but falls back on a
non-template error message if that fails. Optionally logs info to a file for
diagnosis of bugs.

TemplateError("content-type"). Does not print a content-type, but produces the
correct sort of "template failed" error message for the given content-type. This
should be in the || arm of all template calls.

How does that sound?

Gerv

Comment 8

[Jacob Steenhagen]

I had this whole other comment cooked up, but then I went and read the other
comments in this bug little closer...  Being that we're moving to the point
where we do all the processing before we output anything at all, I'd say comment
7 is a good solution.  We can assume that if we have an error while processing
stuff, that no header has been put out (unless that error occured while
processing the template).  That would elminiate and need to keep track of if the
http header had been closed on not (which is basically what I was gonna
suggest).  And Gerv's suggestion in comment 7 also nicely accounts for the fact
that we can have more than one output format.

Comment 9

[Gervase Markham [:gerv]]

This can now be fixed by using ThrowTemplateError($error) in all || arms of
template->process() calls, instead of DisplayError() (which is now deprecated.)

When we do the global template fixup (see bug 135707) before 2.16, this
conversion will get done.

Gerv

Comment 10

[Dave Miller [:justdave]]

setting dependencies per Gerv's comment #9

Comment 11

[Gervase Markham [:gerv]]

This bug should now be fixed. 

Gerv