IntOption and BoolOption values saved even when not differing from default

[hyuga <hyugaricdeau@…>]

The Configuration.save method fails at checking whether IntOptions and BoolOptions differ from their default values. So when calling Configuration.save, if a value has been set on an IntOption or BoolOption, that value is saved to the file even if it is the same as the default for that option, which is not the desired behavior I don't think.

[hyuga <hyugaricdeau@…>]

It looks like the actual problem here is that Section.get and by extension Configuration.get don't always return a string as documented.

The problem is that if the option is not in the trac.ini file, or the global trac.ini file, its default value is retrieved from the Option registry. In the case of an IntOption, for example, the default value will be an integer. The problem is in how the value is returned. From Section.get:

        if not value:
            return u''
        elif isinstance(value, basestring):
            return to_unicode(value)
        else:
            return value

This leads to all kinds of inconsistencies. For example, if the default value for the option is 0, this returns u'' instead of u'0' as one would expect. Or if the default value is 100000 it will return 100000 as an int.

It should probably be something more like this:

        if value is None:
            return u''
        else:
            return to_unicode(value)

[hyuga <hyugaricdeau@…>]

I also noticed that there is some inconsistency with the default values from BoolOptions. For most of them, the defaults are set to 'true' or 'false' as it would appear in the file. But color_scale's default in versioncontrol.web_ui.browser is True., so with my suggested fix this would be returned as u'True', which is still not good.

[hyuga <hyugaricdeau@…>]

Suggesting a milestone for this since it seems to have gotten lost in the pile.

[osimons]

I'll put it on my to-do.

[ebray]

I'm not entirely sure how I feel about this patch, but it does fix the problem as described. Ensures that Section.get() always returns a unicode string, and currectly returns true/false for BoolOptions and returns an integer string for IntOptions (even if the value is 0).

[ebray]

A new version of the patch. I found that if an option's default value is None, then Option.__get__ would return u'None' which is certainly no good. I'm still not sure I like this fix, but it gets the job done, and always returns the correct value now.

[Ryan J Ollos]

Related issue. Several IntOptions and BoolOptions are shown as modified on the About page though they still have the default value:

[Ryan J Ollos]

Section.get states that it will return a string, so it seems clear that is a defect we should fix. We could use Option.dumps to convert the default option value to a string in Section.get. I'm debating whether Section.defaults should also return a dictionary of strings. Alternatively, Option.dumps could be called in Environment.get_configinfo.

The proposed changes are captured in log:rjollos.git:t6551. I'm still working to understand the steps required to produce the problem reported in comment:description.

[Ryan J Ollos]

I'm considering moving Environment.get_configinfo​ to a function of the config module since it's not generally useful as a method of the Environment class. Locating the code in the config module could also make maintenance easier.

[Jun Omae]

Currently, Option.dumps() with True and False serializes 'enabled' and 'disabled'. If enabled for BoolOption which has 'true' as default is set, Trac determines the value is different from the default.

$ ~/venv/trac/1.1.4/bin/python
Python 2.6.9 (default, Oct 22 2014, 19:47:46)
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from trac.test import EnvironmentStub
>>> from trac.loader import load_components
>>> from trac.config import Option, BoolOption
>>> env = EnvironmentStub()
>>> load_components(env)
>>> set(o.default for o in Option.get_registry().itervalues() if isinstance(o, BoolOption))
set([False, 'true', 'false', True])

It would be good to add Option.normalize(), e.g. True, 1, 'true' to 'enabled'. Additional proposed changes in jomae.git@t6551.2.

[Ryan J Ollos]

Thank you for the additional changes. I'll rebase the changes on the trunk now that #11982 is committed.

[Ryan J Ollos]

Related to the comment in #11437 about having a trac-admin $ENV config clean command, it could be useful to highlight the rows on the About page differently when the option is not defined in any enabled Component. We'd need special handling for options that only have a ConfigSection, such as ticket-workflow, or somehow indicate that arbitrarily defined options are allowed for a given section.

[Ryan J Ollos]

Minor refactoring of config module committed in [14017:14018].

[Ryan J Ollos]

Committed to trunk in [14050]. I'll work in #3037 to refactor the test suite and improve test coverage.