log_level and log_type should be ChoiceOptions

[Ryan J Ollos]

If we change log_level and log_type to ChoiceOptions, a TracError will be raised when an invalid value is used.

[logging]
log_level = DEBUG
log_type = stderror

Error

TracError: [logging] log_type: expected one of ("file", "none", "stderr", "syslog", "winlog"), got u'stderror'

This could be useful, but will be challenged by the presence of aliases:

• ALL​ is an alias for DEBUG, and winlog and syslog have aliases​.
• WARN​ is documented as an alias for WARNING, and it appears that the default is WARNING​ for any unrecognized value.

Possible resolutions are:

• Eliminate some aliases and replace them in trac.ini in a upgrade step.
• Enhance ChoiceOption to support aliases.

I probably won't take any action on this for a while, but prepared some proposed changes in log:rjollos.git:t11573 before noticing these issues, and wanted to capture them in a ticket.

[Jun Omae]

I think using ChoiceOption is a good idea. I like to enhance ChoiceOption rather than add a upgrade step for the minor issue.

Also, in [2e6e9c1c/rjollos.git], documents of the log_level and log_type options wouldn't be extracted for tracini.pot.

BTW, I think a ConfigurationError will be raised for such a misconfiguration, not a TracError.

[Ryan J Ollos]

Replying to jomae:

BTW, I think a ConfigurationError will be raised for such a misconfiguration, not a TracError.

ConfigurationError is raised in the call to trac.env.open_environment and the exception is trapped in trac.web.main.dispatch_request. The exception message is passed as env_error to trac.web.main._dispatch_request and an HTTPInternalError is raised: tags/trac-1.0.1/trac/web/main.py@:494#L485​. That exception is trapped (tags/trac-1.0.1/trac/web/main.py@:502#L485​) and sent to the user in trac.web.main._send_user_error. The result is that the user sees:

Error

TracError: [logging] log_type: expected one of ("file", "none", "stderr", "syslog", "winlog"), got u'stderror'

So yes, a ConfigurationError is raised. I should have been more precise and said "the user sees a TracError", rather than "a TracError will be raised".

[Ryan J Ollos]

[trac] default_date_format and [trac] default_dateinfo__format can also be ChoiceOptions: log:rjollos.git:t11573.1.

This results in a significant change in behavior in that a ConfigurationError is raised when an invalid option is specified in trac.ini (similar to the change in #10285).

It was suggested in comment:2:ticket:10285 to add a new class rather than having OrderedExtensionOption raise an exception. Reconsidering this behavior, it might make sense to have a strict parameter for the ChoiceOption, ExtensionOption and OrderedExtensionOption. The strict option would ultimately determine whether the error is only logged, or also presented to the UI. The mechanism could be to have two exception classes that get handled differently.

[Ryan J Ollos]

The changes in log:rjollos.git:t11573.1 could be pushed to the trunk rather than 1.0-stable so that we have time to get feedback on whether it's too extreme of a reaction to show a ConfigurationError on the web interface in response to an invalid configuration value.

[Ryan J Ollos]

Changes from comment:4 committed to trunk in [13298].

[Ryan J Ollos]

LOG_LEVELS extracted to trac.log in [13357]. We might want to make this a dictionary or other type that allows aliases can be specified.

[Ryan J Ollos]

Also, making log_type a ChoiceOption would restrict the ability to add new loggers unless we also provide a plugable interface for new logger types.

[Ryan J Ollos]

Replying to rjollos:

Also, making log_type a ChoiceOption would restrict the ability to add new loggers unless we also provide a plugable interface for new logger types.

I'll propose a new interface in #11874.

For now, the proposed changes that convert the two Options to ChoiceOptions can be found in: log:rjollos.git:t11573.2. I'll add some tests before committing.

[Ryan J Ollos]

Committed to trunk in [13502].

[Ryan J Ollos]

In r15116, fixed string interpolation error in r13502. Merged to trunk in r15117.

[Ryan J Ollos]

Regression with these changes reported in #12779.