Edgewall Software

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#11519 closed enhancement (fixed)

Allow default_handler to be set from the Basic Settings admin page — at Version 21

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.1.2
Component: admin/web Version:
Severity: normal Keywords: default_handler
Cc: Jun Omae Branch:
Release Notes:

The [trac] default_handler option can be set from the Basic Settings admin page. A ConfigurationError is raised when navigating to the base url if [trac] default_handler is set to a class for which is_valid_default_handler is False.

API Changes:

The IRequestHandler class attribute is_valid_default_handler determines if the class can be used as a default handler, and defaults to True when not present. The function is_valid_default_hanlder should be used to determine whether an IRequestHandler object is a valid default handler.

Description

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.

Change History (22)

comment:1 by Ryan J Ollos, 6 years ago

Milestone: next-dev-1.1.x1.1.3
Owner: set to Ryan J Ollos
Status: newassigned

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?

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

in reply to:  1 ; comment:2 by Peter Suter, 6 years ago

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?

Last edited 6 years ago by Peter Suter (previous) (diff)

in reply to:  2 ; comment:3 by Ryan J Ollos, 6 years ago

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.

in reply to:  3 comment:4 by Ryan J Ollos, 6 years ago

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.

in reply to:  3 comment:5 by Ryan J Ollos, 6 years ago

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.

comment:6 by Peter Suter, 6 years ago

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.

in reply to:  6 comment:7 by Ryan J Ollos, 6 years ago

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).
Last edited 6 years ago by Ryan J Ollos (previous) (diff)

in reply to:  2 ; comment:8 by Jun Omae, 6 years ago

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.

in reply to:  8 comment:9 by Ryan J Ollos, 6 years ago

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.

comment:10 by Ryan J Ollos, 6 years ago

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

comment:11 by Jun Omae, 6 years ago

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

in reply to:  11 comment:12 by Ryan J Ollos, 6 years ago

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.

comment:13 by Ryan J Ollos, 6 years ago

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?

in reply to:  13 ; comment:14 by Peter Suter, 6 years ago

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.)

in reply to:  14 comment:15 by Ryan J Ollos, 6 years ago

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.

Last edited 6 years ago by Ryan J Ollos (previous) (diff)

comment:16 by Ryan J Ollos, 6 years ago

Milestone: 1.1.31.1.2

comment:17 by Ryan J Ollos, 6 years ago

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.

comment:18 by Ryan J Ollos, 6 years ago

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

comment:19 by Ryan J Ollos, 6 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in [12624].

comment:20 by Ryan J Ollos, 6 years ago

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

  • trac/admin/web_ui.py

    diff --git a/trac/admin/web_ui.py b/trac/admin/web_ui.py
    index a07c7e9..6c85851 100644
    a b from trac.web import HTTPNotFound, IRequestHandler  
    4242from trac.web.chrome import add_notice, add_stylesheet, \
    4343                            add_warning, Chrome, INavigationContributor, \
    4444                            ITemplateProvider
     45from trac.web.api import is_valid_default_handler
    4546from trac.wiki.formatter import format_to_html
    4647
    4748try:
    class BasicsAdminPanel(Component):  
    212213            yield ('general', _("General"), 'basics', _("Basic Settings"))
    213214
    214215    def render_admin_panel(self, req, cat, page, path_info):
    215         valid_handlers = [h.__class__.__name__ for h in self.handlers
    216                           if getattr(h, 'is_valid_default_handler', True)]
     216        valid_handlers = [hdlr.__class__.__name__ for hdlr in self.handlers
     217                          if is_valid_default_handler(hdlr)]
    217218        if Locale:
    218219            locale_ids = get_available_locales()
    219220            locales = [Locale.parse(locale) for locale in locale_ids]
  • trac/web/api.py

    diff --git a/trac/web/api.py b/trac/web/api.py
    index 8a9851a..8ace8fe 100644
    a b class IRequestHandler(Interface):  
    8787        """
    8888
    8989
     90def is_valid_default_handler(handler):
     91    return handler and getattr(handler, 'is_valid_default_handler', True)
     92
     93
    9094class IRequestFilter(Interface):
    9195    """Enable components to interfere with the processing done by the
    9296    main handler, either before and/or after it enters in action.
  • trac/web/main.py

    diff --git a/trac/web/main.py b/trac/web/main.py
    index c546dde..bdab9f0 100644
    a b class RequestDispatcher(Component):  
    267267
    268268    def get_valid_default_handler(self, req):
    269269        handler = self.default_handler
    270         if not handler or \
    271                 not getattr(handler, 'is_valid_default_handler', True):
     270        if not is_valid_default_handler(handler):
    272271            raise ConfigurationError(
    273272                tag_("%(handler)s is not a valid default handler. Please "
    274273                     "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.

by Ryan J Ollos, 6 years ago

Attachment: t11519.diff added

comment:21 by Ryan J Ollos, 6 years ago

API Changes: modified (diff)
Release Notes: modified (diff)

Change from comment:20 committed in [12684].

Note: See TracTickets for help on using tickets.