Allow default_handler to be set from the Basic Settings admin page

[Ryan J Ollos]

Some allowed values for default_handler are documented at TracIni#trac-section. However, plugins may add IRequestHandlers and some of the documented values may not be valid if the Components are disabled. We might consider trying to dynamically generate the list of IRequestHandlers for the documentation. However, a simple solution would be to add a select to the Basic Settings admin page and allow the default_handler to be set through the web ui.

[Ryan J Ollos]

The IRequestHandlers in a default Trac installation includes several Components that aren't usable as a default_handler. The list of IRequestHandlers is:

['InterTracDispatcher', 'Chrome', 'LoginModule', 'AboutModule',
'AttachmentModule', 'AdminModule', 'PygmentsRenderer', 'PreferencesModule',
'SearchModule', 'BatchModifyModule', 'QueryModule', 'ReportModule',
'RoadmapModule', 'MilestoneModule', 'TicketModule', 'TimelineModule',
'BrowserModule', 'ChangesetModule', 'AnyDiffModule', 'LogModule',
'WikiRenderer', 'WikiModule']

Of those InterTracDispatcher, Chrome, LoginModule, PygmentsRenderer, BatchModifyModule and WikiRenderer aren't usable as a default_handler (and PreferencesModule needs the patch in #11531).

If BatchModifyModule and LoginModule were modified so that they would function correctly as a default_handler, then we could adopt a convention that IRequestHandlers ending in Module should function as a default_handler. However, we could probably come up with better ways of indicating that a Component was suitable for use as a `default_handler. Any good ideas?

Initial changes can be found in log:rjollos.git:t11519. Functional tests will be added later.

Is there a better way to get the list of enabled IRequestHandlers than instantiating a RequestDispatcher object?

[Peter Suter]

Replying to rjollos:

However, we could probably come up with better ways of indicating that a Component was suitable for use as a `default_handler. Any good ideas?

Similar to IWikiMacroProvider.is_inline()​ we could add (an optional) IRequestHandler.is_valid_default_handler().

Is there a better way to get the list of enabled IRequestHandlers than instantiating a RequestDispatcher object?

Could you just repeat the handlers = ExtensionPoint(IRequestHandler) declaration in BasicAdminPanel?

[Ryan J Ollos]

Replying to psuter:

Similar to IWikiMacroProvider.is_inline()​ we could add (an optional) IRequestHandler.is_valid_default_handler().

I had been thinking of adding an attribute to the Interface definition. Perhaps default to False, and implementations can set the property to True if appropriate. Are there advantages to using a function rather than an attribute for this case?

One issue with the attribute is that it may not be obvious enough to the implementer of an IRequestHandler that they should consider whether to override the default value.

Could you just repeat the handlers = ExtensionPoint(IRequestHandler) declaration in BasicAdminPanel?

Yeah, that's a good point. A similar use is in trunk/trac/config.py@12432:725#L717​.

[Ryan J Ollos]

Replying to rjollos:

Perhaps default to False, and implementations can set the property to True if appropriate.

Defaulting to True might be more sensible, since most IRequestHandlers will probably be valid default_handlers.

[Ryan J Ollos]

Replying to rjollos:

I had been thinking of adding an attribute to the Interface definition.

I guess we can't add it to the Interface since the Interfaces aren't base classes.

Like the required attribute​, we could just treat it as having a default value if it is not defined, and add it to the class when the class should have a non-default value.

I think having a method is more clear. The latest proposed changes can be found in log:rjollos.git:t11519.2.

[Peter Suter]

Right. Following the precedent of the required attribute would work as well I think.

The documentation​ sounds a bit misleading to me, given that the default is already True. A more detailed comment like for get_extra_mimetypes​ would be nice. Maybe also mention what is required of a default_handler.

[Ryan J Ollos]

Replying to psuter:

Right. Following the precedent of the required attribute would work as well I think.

I'm reconsidering. It might be bit cleaner to use an attribute, and we can document it in the IRequestHandler interface documentation: trunk/trac/web/api.py@12527:51#L50​.

I will give it some thought for another day or two, and wait to see if there is additional input on which direction to take.

I will also make sure to improve the documentation per your feedback. My understanding of the requirements for a default_handler are:

• Returns a template representing an HTML document and the data dictionary needed to render the template (i.e. must satisfy the minimum requirements for an HTML document: html, head and body tags).
• Does not require match_request to be called before process_request (as an example, PreferencesModule failed to meet this requirement prior to the fix in #11531: branches/1.0-stable/trac/web/main.py@12460:180-182#L171​).

[Jun Omae]

Similar to IWikiMacroProvider.is_inline()​ we could add (an optional) IRequestHandler.is_valid_default_handler().

I like property rather than such a method, e.g. jquery_noconflict property of IRequestHandler class at tags/trac-1.0.1/trac/web/chrome.py@:674-677#L652​.

[Ryan J Ollos]

Replying to jomae:

I like property rather than such a method, e.g. jquery_noconflict property of IRequestHandler class at tags/trac-1.0.1/trac/web/chrome.py@:674-677#L652​.

That is an interesting case. It seems like a good time to document jquery_noconflict in the IRequestHandler interface definition as well.

[Ryan J Ollos]

Latest changes can be found in log:rjollos.git:t11519.3. is_valid_default_handler is now a property.

[Jun Omae]

Refactored in jomae.git@t11519.4, especially, accessing option properties is not cheap.

[Ryan J Ollos]

Replying to jomae:

Refactored in jomae.git@t11519.4, especially, accessing option properties is not cheap.

I was confused and had meant to use lazy there rather than property, but your changes are better. Thanks! I'll add functional tests in the coming days.

[Ryan J Ollos]

I tried some different ways of isolating the code that checks whether default_handler is a valid value.

One was to define the class attribute as _default_handler, and have a class property named default_handler: [13144c03/rjollos.git]. Maybe that would be okay, but it doesn't look quite right. It would be nice if we could set a getter when the ExtensionOption is defined, but I haven't sorted out whether there is a way to make that work.

The other simple way to isolate the code would be to have a get_default_handler method: [9b8b200b/rjollos.git].

Is either of these two better than previous changes?

[Peter Suter]

Both look ok to me.

Another alternative would be to leave default_handler as it is, and add the class property named valid_default_handler. (But if that's really better is debatable.)

[Ryan J Ollos]

Replying to psuter:

Another alternative would be to leave default_handler as it is, and add the class property named valid_default_handler. (But if that's really better is debatable.)

I like valid_default_handler. I'll go ahead and make that change.

[Ryan J Ollos]

If I had tested the changes I proposed in comment:13, I would have realized that using a property is problematic because a Request object is needed for constructing the href to the Basic Settings admin page.

I tried instead using self.env.href. That seems to work okay for the case that [trac] base_url is not set, because of this assignment​. However, if [trac] base_url is set incorrectly, we land on the wrong page. I don't see any other cases of self.env.href used to generate an href, so I'm suspicious as to whether this is a good idea. Just the problem seen when [trac] base_url is set incorrectly might push us to instead prefer: [7903703f/rjollos.git].

get_valid_default_handler might be better anyway because in the future I'd like to propose adding default_handler as a session preference, and when get_valid_default_handler is modified to check the user's default_handler preference, it will need access to the Request object.

Proposed changes can be found in log:rjollos.git:t11519.5. I'm working on adding tests now.

[Ryan J Ollos]

Functional test added in [b4735636/rjollos.git].

[Ryan J Ollos]

Committed to trunk in [12624].

[Ryan J Ollos]

Olemis and I came up with the following refactoring after reviewing [12624] today:

trac/admin/web_ui.py

@@ -42 +42 @@
 from trac.web.chrome import add_notice, add_stylesheet, \
                             add_warning, Chrome, INavigationContributor, \
                             ITemplateProvider
+from trac.web.api import is_valid_default_handler
 from trac.wiki.formatter import format_to_html
 
 try:
@@ -212 +213 @@
             yield ('general', _("General"), 'basics', _("Basic Settings"))
 
     def render_admin_panel(self, req, cat, page, path_info):
-        valid_handlers = [h.__class__.__name__ for h in self.handlers
-                          if getattr(h, 'is_valid_default_handler', True)]
+        valid_handlers = [hdlr.__class__.__name__ for hdlr in self.handlers
+                          if is_valid_default_handler(hdlr)]
         if Locale:
             locale_ids = get_available_locales()
             locales = [Locale.parse(locale) for locale in locale_ids]

trac/web/api.py

@@ -87 +87 @@
         """
 
 
+def is_valid_default_handler(handler):
+    return handler and getattr(handler, 'is_valid_default_handler', True)
+
+
 class IRequestFilter(Interface):
     """Enable components to interfere with the processing done by the
     main handler, either before and/or after it enters in action.

trac/web/main.py

@@ -267 +267 @@
 
     def get_valid_default_handler(self, req):
         handler = self.default_handler
-        if not handler or \
-                not getattr(handler, 'is_valid_default_handler', True):
+        if not is_valid_default_handler(handler):
             raise ConfigurationError(
                 tag_("%(handler)s is not a valid default handler. Please "
                      "update %(option)s through the %(page)s page or by "

The aim is to isolate to a single location in the codebase the definition of the default value for the is_valid_default_handler attribute.

[Ryan J Ollos]

Change from comment:20 committed in [12684].