Edgewall Software
Modify

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#8290 closed defect (fixed)

[PATCH] Allow plugins to provide configuration information

Reported by: Joachim Hoessler <hoessler@…> Owned by: osimons
Priority: normal Milestone: 0.11.5
Component: general Version: none
Severity: normal Keywords: configuration
Cc: ryano@…
Release Notes:
API Changes:

Description

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

Attachments (4)

config-provider.patch (6.9 KB) - added by Joachim Hoessler <hoessler@…> 5 years ago.
bug8290.patch (2.9 KB) - added by Joachim Hoessler <hoessler@…> 5 years ago.
Fix for wrong reading from of options registry
t8290-config_section-r8198-011.diff (1009 bytes) - added by osimons 5 years ago.
Patch for lookup and check of Option supplied options.
t8290-config_section2-r8198-011.diff (3.6 KB) - added by osimons 5 years ago.
Further improvements - including multiple inheritance…

Download all attachments as: .zip

Change History (20)

Changed 5 years ago by Joachim Hoessler <hoessler@…>

comment:1 Changed 5 years ago by cboos

  • Keywords configuration consider added
  • Milestone set to 0.12

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

comment:2 Changed 5 years ago by Jeff Hammel <jhammel@…>

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

comment:3 follow-ups: Changed 5 years ago by 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.

comment:4 Changed 5 years ago by osimons

  • Cc osimons added

comment:5 in reply to: ↑ 3 Changed 5 years ago by 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

comment:6 in reply to: ↑ 3 Changed 5 years ago by 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__().

comment:7 follow-up: Changed 5 years ago by osimons

  • Keywords consider removed
  • Milestone changed from 0.12 to 0.11.5
  • Type changed from enhancement to defect

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.

Changed 5 years ago by Joachim Hoessler <hoessler@…>

Fix for wrong reading from of options registry

comment:8 in reply to: ↑ 7 Changed 5 years ago by 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!

Changed 5 years ago by osimons

Patch for lookup and check of Option supplied options.

comment:9 follow-up: Changed 5 years ago by osimons

  • Cc osimons removed
  • Owner set to 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…

Changed 5 years ago by osimons

Further improvements - including multiple inheritance…

comment:10 follow-up: Changed 5 years ago by 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.

comment:11 in reply to: ↑ 10 Changed 5 years ago by 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.

comment:12 in reply to: ↑ 9 Changed 5 years ago by Joachim Hoessler <hoessler@…>

Replying to osimons:

Heh. Let's compare patches :-)

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

comment:13 follow-up: Changed 5 years ago by cboos

Patch looks good, indeed.

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

comment:14 in reply to: ↑ 13 Changed 5 years ago by 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.

comment:15 Changed 5 years ago by osimons

  • Resolution set to fixed
  • Status changed from new to closed

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

Thanks for report and testing.

comment:16 Changed 5 years ago by Ryan Ollos <ryano@…>

  • Cc ryano@… added

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed The owner will remain osimons.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from osimons to the specified user.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.