TracError messages with markup are not enclosed in a p.message when rendered

[Ryan J Ollos]

While trying to raise an exception in which the message contains a link, I found that the message was is not wrapped in a p.message. There's at least one existing case in the codebase where the message contains markup: tags/trac-1.0.1/trac/web/auth.py@:145-153#L130​. By forcing the conditional to True, it can be seen to render as:

The behavior is readily explained by the rendering logic​, and [5388#file4] indicates it was implemented that way intentionally to avoid nested paragraphs. I propose a minor change to incrementally improve on this: log:rjollos.git:t11368.

[Ryan J Ollos]

With the proposed change we get the expected:

[Jun Omae]

The changes break error message from timeline view. See ​http://twitpic.com/dln1ed/full.

At least, TracError or subclasses with Genshi elements is raised from the following.

$ egrep -r '(TracError|AdminCommandError|InvalidAttachment|ConfigurationError|ResourceNotFound|QuerySyntaxError|QueryValueError|InvalidTicket|ProcessorError|InvalidWikiPage)\( *tag' trac tracopt
trac/versioncontrol/web_ui/util.py:        raise ResourceNotFound(tag(
trac/notification.py:            raise TracError(tag(
trac/timeline/web_ui.py:        raise TracError(tag(
trac/web/auth.py:            raise TracError(tag_("Authentication information not available. "

We should fix only trac/web/auth.py, I think.

branches/1.0-stable/trac/web/auth.py

@@ -148 +148 @@
                              title=_("Configuring Authentication"),
                              href=req.href.wiki('TracInstall')
                                   + "#ConfiguringAuthentication")
-            raise TracError(tag_("Authentication information not available. "
-                                 "Please refer to the %(inst_doc)s.",
-                                 inst_doc=inst_doc))
+            raise TracError(tag.p(tag_("Authentication information not "
+                                       "available. Please refer to the "
+                                       "%(inst_doc)s.", inst_doc=inst_doc),
+                                  class_='message'))
         remote_user = req.remote_user
         if self.ignore_case:
             remote_user = remote_user.lower()

[Ryan J Ollos]

It doesn't seem like very good behavior to make the user wrap every error message with containing in a paragraph in order to get consistent display of error messages. I will see if I can modify my patch so that it works correctly with error messages from the timeline as well.

[Ryan J Ollos]

Oh, I am dumb. I tested find_element, but I didn't test the usage of find_element for the case that the message contains a p element, and I wasn't calling it correctly. After [4a61f679/rjollos.git], the TracError from the timeline event that contains a paragraph will be rendered correctly. Do you see any problem with the changes now that the issue is fixed?

[Jun Omae]

Before the changes, if want to show directly a message on error view, TracError's message must have Genshi fragments or elements. After the changes, TracError's message must have Genshi p element.

No problem. Verified the following. Please commit.

• trac/versioncontrol/web_ui/util.py
• trac/timeline/web_ui.py
• trac/web/auth.py

TracError from trac/notification.py would be shown with add_warning, not on error view. Also it has another issue, #11369.

[Ryan J Ollos]

Replying to jomae:

Before the changes, if want to show directly a message on error view, TracError's message must have Genshi fragments or elements. After the changes, TracError's message must have Genshi p element.

Maybe I'm misunderstanding, but as far as I can see the only difference after the change is that if the error message doesn't contain a p element, the error message will be wrapped in a p.message: [dffc5587/rjollos.git#file0].

I did some testing this evening with the following plugin:

• /throwerror
• /throwerror?markup=true
• /throwerror?markup=true&p=true

from genshi.builder import tag
from trac.core import Component, TracError, implements
from trac.web.api import IRequestHandler

class ThrowErrorPlugin(Component):
    implements(IRequestHandler)

    def match_request(self, req):
        if req.path_info.startswith('/throwerror'):
            return True

    def process_request(self, req):
        if 'markup' in req.args and req.args['markup'] == 'true':
            if 'p' in req.args and req.args['p'] == 'true':
                # Markup with p element should be rendered as is
                raise TracError(tag(tag.p("Error ", tag.strong("with markup"), " and it is already enclosed in a ", tag.em("p.message"), class_="message"), tag.p("There is also some ", tag.em("markup outside the p.message"), "including a link to ", tag.a("the Trac site", href='http://trac.edgewall.org'))))
            else:
                # Markup with no p element should be wrapped in a p.message
                raise TracError(tag("Error ", tag.strong("with markup."), " It should be wrapped in a ", tag.code("p.message")))
        else:
            raise TracError("Error in plain text.")

Maybe a functional test could be added to check these cases.

TracError from trac/notification.py would be shown with add_warning, not on error view. Also it has another issue, #11369.

Oh, that's great, thank you. I also encountered that issue while working on #11189, and I was planning to investigate after this ticket is resolved.

[Ryan J Ollos]

I added a small refactoring in [22ab25dc/rjollos.git].

[Ryan J Ollos]

Committed to 1.0-stable in [12260] and merged to trunk in [12261].

[Ryan J Ollos]

#12916 is related.