> Side note (find it funny): you complain this property is skipped if you
use
> another constructor but same applies to the auto deploy one ;)
The auto deploy one (the no-arg constructor) sets this property :) While
the others - do not. Which results in a very unpleasant surprise when using
custom configuration factory. So may idea was to make the behavior
consistent among the different constructors. Also it's much easier to
override or check the value of the property if it's in system.properties,
rather than searching throughout the whole codebase. When I tried CDI
enabled app on my tomee with custom configuration factory, the deployment
started to fail because there were two BeanValidator classes and it was
pure luck that I accidentally discovered that hidden property.
Svetlin
2017-02-07 11:41 GMT+02:00 Romain Manni-Bucau <rmannibu...@gmail.com>:
> Hi
>
> 2017-02-07 10:31 GMT+01:00 Svetlin Zarev <svetlin.angelov.za...@gmail.com
> >:
>
> > Hello,
> >
> > In *one* of the configuration factory constructors a system property is
> > set:
> >
> > public ConfigurationFactory() {
> > this(!shouldAutoDeploy());
> > System.setProperty("bval.in-container", "true");
> > }
> >
> > What do you think about moving this to system.properties ? The reason is
> > that:
> >
>
> well, not an option cause 80% of "tomee" usages are not in tomee and don't
> have system.properties - but doesn't mean we can't move it, just that the
> file will not work
>
>
> > * it's really hidden and not obvious why it's there
> >
>
> Was the most common piece of code executed early enough when added and also
> a place you can override and change if you desire to keep default behavior
> or have a custom one. We can surely make it part of SystemInstance but
> override will be harder.
>
>
> > * it's being set every time in case of vanilla tomee
> >
>
> Was one of the goal, not sure I see what the issue is
>
>
> > * it's not set when using custom configuration factory when you use one
> of
> > the other constructors (and this breaks CDI)
> >
>
> Not set: was part of the goal to let the custom factory change that easily
> but agree we can find a better place
> Breaks CDI: depends the CDI config actually but by default yes cause
> BValExtension will try to add a validator and validator factory but openejb
> does it as well
>
>
> > * IMO it's a good idea to have all global properties at one place,
> instead
> > of being scattered in the source
> >
>
> Yes and no, there are multiple kind of properties:
>
> - global one ("enforced") like this one: not against aggregating them but
> gain shouldn't be that big
> - speific ones: this needs to be done per code area and not aggregated all
> together (you will not put tomee properties in openejb code for instance)
> - "in case ones": we have some of that kind where it is there just cause it
> can break and it enables the user to make its app working even if default
> is not desired. These ones are not all very official and I don't find it
> that bad to hide them a bit cause we shouldn't encourage them
>
> So story short: we can aggregate several ones but we can't - I think - put
> them all in a single SystemProperties class.
>
> Side note (find it funny): you complain this property is skipped if you use
> another constructor but same applies to the auto deploy one ;)
>
> Finally note official properties are listed on
> http://tomee.apache.org/admin/configuration/server.html
>
> If we miss a bit we can add them but this bval one is an internal one. At
> some point we can even drop it from bval and just detect we run in tomee
> and set it this way instead of using a system property - maybe better?
>
>
> >
> > Kind regards,
> > Svetlin
> >
>
