2017-02-07 11:56 GMT+01:00 Svetlin Zarev <svetlin.angelov.za...@gmail.com>:
> > 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.
well you hardcode it so still surprising for users having configured it ;)
anyway not the central piece of that thread - yet ;)
> 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.
>
>
Hmm should fail cause Validator injection is ambiguous, the other issues
are not linked to that property but BValCdiFilter implementation which is
set by default whatever this system property is
> 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.zarev@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
> > >
> >
>
