forrest-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Thorsten Scherler <thorsten.scher...@wyona.com>
Subject Re: adding site-wide configuration files (was: [jira] Closed: (FOR-913) failure when ${project.home}/forrest.properties does not exist)
Date Mon, 07 Aug 2006 14:40:50 GMT
El lun, 07-08-2006 a las 10:45 +0100, Ross Gardler escribió:
> Mathieu Champlon wrote:
> > Ross Gardler a écrit :
> > 
> >> Mathieu Champlon wrote:
> 
> ...
> 
> >> Then why not provide us with a patch? (again a rhetorical question, 
> >> keep reading, I just want to point out we need your help)
> > 
> > (...)
> > 
> >> I've applied a guard around the necessary code. It should now work 
> >> without a forrest.properties file.
> 
> OK, I get it. Well I kind of got it before, hence my patch.
> 
> > I was actually hoping/waiting for the re-opening of the issue so I could
> > attach the patch.
> 
> Well, like you said it was real simple, almost as easy to do the change 
> as it was to open the issue again. Sorry I missed this in your post 
> though, help is always appreciated, I should have left it for you to do ;-)
> 
> >> Perhaps your use case requries a new site-wide properties file. It 
> >> would be easier to manage than setting properties in your ant build 
> >> file and is easy to implement.
> > 
> > 
> > Yes, good point !
> > 
> > (...)
> > 
> >> It is also worth noting that in a future version of Forrest (probably 
> >> 0.9) we will be moving to a new XML property configuration system. 
> >> This provides much more flexibility than the existing system. If you 
> >> add a site-wide file please be sure to add an equivalent XML conf file 
> >> (loaded in the same code).
> > 
> > 
> > I took a look at the configuration management before adding this new
> > feature and I have a few refactoring suggestions :
> > 
> > 1/ ForrestConfModule#getSystemProperty is never used and therefore
> > neither is defaultHome
> > 2/ import java.io.FileNotFoundException; is not needed anymore
> 
> OK.
> 
> > 3/ configuration variables precedence order
> > 
> > It seems from reading ForrestConfModule#initialize that configuration
> > variables come from four different sources, in this order (the first
> > defined wins) :
> > . system properties
> > . xml configuration files
> > . property configuration files
> > . plugin xml configuration files
> > 
> > System properties are defined in the ant scripts as the properties
> > starting with either 'forrest.' or 'project.', including -D properties
> > from command-line and properties loaded from files with <property
> > file=.../>.
> > Therefore all configuration variables defined in property files loaded
> > by forrest.build.xml are accessible as system properties in
> > ForrestConfModule (well actually not all of them, because 'proxy' isn't,
> > but it's used only in ant scripts).
> > So there is no need to reload ${project.home}/forrest.properties in
> > ForrestConfModule#initialize.
> > Moreover, as the system properties are loaded at the end but replace all
> > other properties (in ForrestConfModule#loadSystemProperties),
> > configuration values from xml files are actually replaced by values from
> > property files.
>  >
> > So the actual precedence order is :
> > . command-line properties
> > . property configuration files
> > . xml configuration files
> > . plugin xml configuration files
> > 
> > What I suggest is to refactor ForrestConfModule#initialize so that it
> > clearly reflects all this, simplifying it at the same time, mostly by
> > removing the unnecessary code and re-arranging the loading order.
> 
> That all sounds fine by me.

You forgot the ones from forrest-core.xconf.

ForrestConfModule does not implement configure(...) meaning it will use
the one from super (DefaultsModule). This is adding properties from the
configuration (which are different for defaults vs. project).

> 
> > 4/ ForrestConfModule#initialize is called twice
> > 
> > I don't exactly know why, it seems to be called from avalon, because
> > this method is actually an implementation of
> > org.apache.avalon.framework.activity.Initializable#initialize().
> > I haven't really looked further...
> 
> I have nothing to add to this. I didn't realise it was called twice. 
> Since this is a Cocoon input module, then this is a Cocoon thing. So we 
> need to take it up with them once we have a little more understanding of 
> why it is happening.

I think the logical explanation is that we use it *twice*. Once for the
"defaults" and the other for "projects". 

http://svn.apache.org/viewvc/forrest/trunk/main/webapp/WEB-INF/xconf/forrest-core.xconf?view=markup

I do not like this at all and may explain further (in another mail) why,
but bottom line is I wrote a input-module parameter generator (will
check it in shortly) and playing around with it I encountered problems
with this architecture. 

> 
> > 5/ there is no user.home forrest.properties.xml
> > 
> > There is a ${user.home}/forrest.properties loaded from the ant scripts,
> > but there is no corresponding xml configuration file. Is it on purpose ?
> > The user.home property from ant is not passed as system property because
> > it does not start with neither 'forrest.' nor 'project.', but it can be
> > specifically added.
> 
> This is not on purpose. The forrest.properties.xml was added for the 
> specific purpose of allowing plugins to extend the configuration options 
> available in the sitemap without having to touch core.
> 
> It is still experimental (i.e. incomplete) code, so adding this feature 
> would be a good idea.

Actually one big thing we forgot. 

public Iterator getAttributeNames is not implemented but used form
super. That returns only the properties we have defined in the
forrest-core.xconf, that is way to less, since we added a lot more.

Now we can either only return the keys defined in the xconf (this is
what super does) or all keys defined in our module
(filteringProperties). The problem is now that one can reach the same
value like {project:something} and {defaults:something}, which is not
good at all because we share the input module in 2 different module
confs.

Further the whole defaults input module does not makes sense anymore,
since the new properties system is fallback enabled, meaning we always
have a default.

salu2

> 
> > 6/ finally : adding site-wide properties files
> > 
> > Not much to say, only that the configuration files are only looked for
> > if a property named global.home exists (naming it site.home can be
> > confusing because 'site' has a special meaning in forrest already, so I
> > picked 'global').
> 
> Sounds fine.
> 
> We look forward to your patches. Thanks for your contribuion.
> 
> Ross

-- 
Thorsten Scherler
COO Spain
Wyona Inc.  -  Open Source Content Management  -  Apache Lenya
http://www.wyona.com                   http://lenya.apache.org
thorsten.scherler@wyona.com                thorsten@apache.org


Mime
View raw message