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
>
