On Mon, Jan 28, 2002 at 09:48:32PM -0800, David Ascher wrote:
>My contribution is attached -- a patch (probably needs tweaking) to
>enable cgitb if it's been enabled.  With the patch applied, you can add

I like this patch, though I can agree with David's point about cgitb's
colour scheme.  However, the patch turns up a Quixote misdesign.
Publisher.format_internal_error() reads like this:

    def format_internal_error (self, request, error_message):
        if self.config.display_exceptions:
            # XXX note the next line!
            request.response.set_header("Content-Type", "text/plain")
            return error_message
        else:
            request.response.set_header("Content-Type", "text/html")
            admin = request.environ.get('SERVER_ADMIN',
                                   "email address unknown")
            return INTERNAL_ERROR_MESSAGE % admin

If display_exceptions is true, this always sets the content-type to
text/plain.  I submit that this is bogus; the caller is providing the
content of the message, and should therefore set the content-type to
text or HTML or whatever it sees fit.  (If you want to return a JPEG
in case of errors, go ahead!)  In the other format_*_error() methods,
the methods themselves create the message, and therefore should
specify the content-type.

A revised version of the patch is below, fixing this; it also factors
out the actual generation of error pages into separate private
methods, and adds a typo fix.  Votes?

--amk                                                  (www.amk.ca)
If I knew everything that was about to happen, where would the fun be?
    -- The Doctor, in "The Keeper of Traken"


Index: publish.py
===================================================================
RCS file: /projects/cvsroot/mems/quixote/publish.py,v
retrieving revision 1.107
diff -u -r1.107 publish.py
--- publish.py  24 Jan 2002 16:08:53 -0000      1.107
+++ publish.py  30 Jan 2002 23:00:30 -0000
@@ -7,6 +7,7 @@

 import sys, os, traceback, cStringIO
 import time, types, socket, re
+import cgitb

 from quixote import errors, fcgi
 from quixote.http_request import HTTPRequest
@@ -361,11 +362,10 @@
         Format a response after a non-PublishError exception.
         Applications may wish to subclass this method but they should do
         so carefully.  The application could be broken badly when this
-        method is called.  This probably best to keep the formatting
+        method is called.  It's probably best to keep the formatting
         simple.
         """
         if self.config.display_exceptions:
-            request.response.set_header("Content-Type", "text/plain")
             return error_message
         else:
             request.response.set_header("Content-Type", "text/html")
@@ -390,7 +390,32 @@
         error_summary = traceback.format_exception_only(exc_type, exc_value)
         error_summary = error_summary[0][0:-1] # de-listify and strip newline
         error_file = cStringIO.StringIO()
+
+        if isinstance(sys.excepthook, cgitb.Hook):
+            self._generate_cgitb_error(error_file,
+                                       request, original_response,
+                                       exc_type, exc_value, tb)
+        else:
+            self._generate_plaintext_error(error_file,
+                                           request, original_response,
+                                           exc_type, exc_value, tb)
+
+        error_msg = error_file.getvalue()
+        self.error_log.write(error_msg)
+        if request.session is not None:
+            request.session.dump(file=self.error_log)
+
+        if self.config.error_email:
+            self.mail_error(error_msg, error_summary)
+
+        request.response.set_status(500)
+        return self.format_internal_error(request, error_msg)
+

+    def _generate_plaintext_error (self,
+                                   error_file, request, original_response,
+                                   exc_type, exc_value, tb):
+        request.response.set_header("Content-Type", "text/plain")
         #self.debug("report_error: exception = %s: %s" % (exc_type, exc_value))

         # record the time of the error
@@ -409,17 +434,17 @@
                            '=================\n')
         original_response.write(error_file)
         error_file.write('='*65 + '\n')
-
-        error_msg = error_file.getvalue()
-        self.error_log.write(error_msg)
-        if request.session is not None:
-            request.session.dump(file=self.error_log)
-
-        if self.config.error_email:
-            self.mail_error(error_msg, error_summary)
-
-        request.response.set_status(500)
-        return self.format_internal_error(request, error_msg)
+
+    def _generate_cgitb_error (self, error_file, request, original_response,
+                               exc_type, exc_value, tb):
+        request.response.set_header("Content-Type", "text/html")
+        hook = cgitb.Hook(file=error_file)
+        hook(exc_type, exc_value, tb)
+        error_file.write('
Original Request
')
+        error_file.write(request.dump_html())
+        error_file.write('
Original Response
')
+        original_response.write(error_file)
+        error_file.write('
')

     def mail_error (self, msg, error_summary):
         """Send an email notifying someone of a traceback."""