[PATCH] Allow plugins to provide configuration information

[Joachim Hoessler <hoessler@…>]

Many plugins rely on proper configuration of other components, e.g., they might require that a specific custom ticket field has been configured. Currently, all that configuration must be manually added to the "trac.ini" file, which can be error-prone. Furthermore, if the plugin is going to be removed again, all changes to the "trac.ini" must be removed manually as well.

To solve this problem, the attached patch against source:trunk@8199​ introduces a new extension point IConfigurationProvider that allows components to provide additional configuration options.

A similar request might have been raised in #4295, though I'm not sure :-)

[Christian Boos]

It seems that when saving, the sections and their values coming from providers would be saved as well, which defeats one of the purpose you stated.

Otherwise, the patch looks good: the providers only provide extra configuration values and the regular trac.ini and inherited .ini have precedence.

Now if this is the best way to provide custom ticket fields programmatically, that's an other question (see e.g. WorkFlow and source:sandbox/workflow/trac/ticket/api.py?rev=3378#L71).

[Jeff Hammel <jhammel@…>]

This has historically been done for many plugins as an IEnvironmentSetupParticipant. Is this not the way to go?

[osimons]

Why not just provide 'ticket-custom' values as Option on the component? No patches needed, and works today. A plugin can provide any Option it likes for any section - including custom fields and workflow.

I really don't see the need for adding an API to do the exact same thing.

[Jeff Hammel <jhammel@…>]

Replying to osimons:

Why not just provide 'ticket-custom' values as Option on the component? No patches needed, and works today. A plugin can provide any Option it likes for any section - including custom fields and workflow.

I really don't see the need for adding an API to do the exact same thing.

Hah, i didn't even think about that! Makes my ​th:CustomFieldProviderPlugin look pretty silly

[Joachim Hoessler <hoessler@…>]

Replying to osimons:

Why not just provide 'ticket-custom' values as Option on the component? No patches needed, and works today. A plugin can provide any Option it likes for any section - including custom fields and workflow.

I really don't see the need for adding an API to do the exact same thing.

You're right, if the option descriptors can provide the same functionality, then there is really no need for that extension. However, in the current implementation (0.11-stable as well as trunk), I'm not able to add an additional custom field via an option.

It seems that the options added via descriptors can be accessed only directly via Section.get(), but they are missing if you iterate over the options in a section. I guess there's some code missing in Section.__iterate__().

[osimons]

It sure smells like a bug in Trac. I made this plugin:

from trac.core import *
from trac.config import Option

class MyCustomField(Component):
    """ Example of providing a custom field in a module. """

    implements()

    myfield = Option('ticket-custom', 'myfield', 'text')
    myfield_label = Option('ticket-custom', 'myfield.label', 'My field')
    myfield_value = Option('ticket-custom', 'myfield.value', 'Hello')

Then some interactive testing:

>>> from trac.env import Environment
>>> env = Environment('/path/to/project')
>>> env.config.has_option('ticket-custom', 'myfield')
True
>>> env.config.get('ticket-custom', 'myfield')
u'text'
>>> env.config.options('ticket-custom')
[]

So, that verified your finding and it sure looks like Section does not iterate correctly. Hmm. A quick look at that code and I see 2 issues:

1. __iter__() only considers options found in environment or parent, not in options registry.
2. get() first checks if option is found in local file, and if not checks to see if there is a parent file and if so tries to locate it there. The way the clause is written, it means that if the project has a parent file that elif will evaluate to True regardless of the wether the option is there or not - meaning the else clause that looks in options registry will never kick in.

Better have a look at this. It should work.

[Joachim Hoessler <hoessler@…>]

Fix for wrong reading from of options registry

[Joachim Hoessler <hoessler@…>]

Replying to osimons:

So, that verified your finding and it sure looks like Section does not iterate correctly. Hmm. A quick look at that code and I see 2 issues:

[…]

I've tried to address those two issues in attachment:bug8290.patch​, as well as a similar issue in __contains__(). Now I'm able to add custom fields via an Option in my plugin!

[osimons]

Patch for lookup and check of Option supplied options.

[osimons]

Heh. Let's compare patches :-)

I think we have solved the same thing, but need to look at it more closely. I was going to write a test-case or two as well, but my Subversion 1.6.1 seems to crap out the test suite in no minor way and I got a bit side-tracked… I really need to get the test suite working before being able to finish this…

[osimons]

Further improvements - including multiple inheritance…

[osimons]

Ah, the joy of Subversion 1.6.2 - my tests are now working again.

Reading the code again, I see that my (2.) above was not a correct observation. The parent is in itself a Configuration, so that the top-level (parent if inherited file is used, or the project trac.ini if no parent is configured) will be the one to report the component-supplied options.

So, instead of adding code this patch was actually a matter of cleaning what should be working correctly in the first place. That I may say, also includes removing errors made by me earlier…

I've also added tests for has_option(), sections() and options() into existing tests. All tests pass.

Finally, a side-effect of the code working as it should is actually that it supports multiple inheritence… I've tested a few levels deep and all works like a charm. There is however nothing in the code or documentation that makes this an offical or supported feature, and no guards in code to prevent people from introducing circular references that will keep reading until Trac blows up. On the other hand, the administrators that work with trac.ini at this level should really know what they are doing anyway.

Testing and feedback welcome.

[osimons]

Replying to osimons:

Finally, a side-effect of the code working as it should is actually that it supports multiple inheritence…

Just a correction of term used: 'multiple inheritance' means multiple levels of files each pointing to one parent. I suppose 'chaining' is a better term to use.

[Joachim Hoessler <hoessler@…>]

Replying to osimons:

Heh. Let's compare patches :-)

Your patch looks good - and more complete than mine. It works as advertised!

[Christian Boos]

Patch looks good, indeed.

However, check the indentation in the trac/tests/config.py part, 2nd test (tabs perhaps?)

[osimons]

Replying to cboos:

However, check the indentation in the trac/tests/config.py part, 2nd test (tabs perhaps?)

I've been staring at the patch and the files, and I see no indentation issues and no tabs. Are you perhaps referring to the list(config.options('a'))) part? I have a habit of pushing code on the 'same line' as far right as possible to indicate that it follows on from previous, but perhaps in this case it becomes more readable further a bit further to the left. Updated.

I'll commit the patch soon, and will remember to make a note on TracIni page about chaining when I do.

[osimons]

Patch committed in [8243:8244] for 0.11 and trunk.

Thanks for report and testing.