geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ivan <xhh...@gmail.com>
Subject Re: ordering tck failures in geronimo
Date Tue, 14 Dec 2010 03:08:30 GMT
Thanks, Jakob and Leonardo, the new changes work perfect ;-)
By the way, I have run the TCK on my local environment, although there are
some failure cases, they should be caused by webbeans integration.

2010/12/11 Jakob Korherr <jakob.korherr@gmail.com>

> Hi guys,
>
> As discussed, I just committed a modified version of the proposed patch.
>
> Furthermore I added the custom SPI impl that I used for testing to the
> issue [1] which may help for the real Geronimo impl.
>
> It would be great if you guys could check if this solution works for
> you, and if so, we can close this issue and start the 2.0.3 release!
>
> Regards,
> Jakob
>
> [1]
> https://issues.apache.org/jira/secure/attachment/12466010/GeronimoFacesConfigurationMerger.java
>
> 2010/12/10 Jakob Korherr <jakob.korherr@gmail.com>:
> > Hi Leo,
> >
> > OK great. That's fine with me.
> >
> > I will apply the appropriate changes and commit the patch!
> >
> > Regards,
> > Jakob
> >
> > 2010/12/10 Leonardo Uribe <lu4242@gmail.com>:
> >> Hi Jakob
> >>
> >> I think it is better do not include it as parameter and instead add some
> >> comment on the javadoc,
> >> saying the information to take into account is retrieved from
> >> FacesConfigurationProvider. The fact
> >> of use FacesConfigurationProvider is more an implementation detail than
> a
> >> requeriment, so if
> >> somebody wants to do not use it is valid usage.
> >>
> >> regards,
> >>
> >> Leonardo Uribe
> >>
> >> 2010/12/10 Jakob Korherr <jakob.korherr@gmail.com>
> >>>
> >>> Hi,
> >>>
> >>> OK, great! I added another comment to MYFACES-2945 explaining why I
> >>> did it this way.
> >>>
> >>> Please tell me your final opinion after reading this comment and I'll
> >>> commit an appropriate version of the proposed patch!
> >>>
> >>> Regards,
> >>> Jakob
> >>>
> >>> 2010/12/9 Leonardo Uribe <lu4242@gmail.com>:
> >>> > Hi
> >>> >
> >>> > I think it is not necessary to pass FacesConfigurationProvider as
> param
> >>> > for
> >>> > getFacesConfigData, because in theory we don't do anything with it
> >>> > before
> >>> > pass it and wrappers will not do anything with it later. I think it
> >>> > looks
> >>> > good to load it using
> >>> >
> >>> >
> FacesConfigurationProviderFactory.getFacesConfigurationProvider(ExternalContext).
> >>> >
> >>> > The other parts of the patch looks good.
> >>> >
> >>> > regards,
> >>> >
> >>> > Leonardo Uribe
> >>> >
> >>> > 2010/12/9 Jakob Korherr <jakob.korherr@gmail.com>
> >>> >>
> >>> >> Hi guys,
> >>> >>
> >>> >> I just uploaded a patch for a FacesConfigurationMerger SPI:
> >>> >> MYFACES-2945-FacesConfigurationMerger.patch
> >>> >>
> >>> >> Furthermore I added a quick code sample as comment on the
> MYFACES-2945
> >>> >> issue about how Geronimo can use this new SPI.
> >>> >>
> >>> >> Please take a look at the patch and if there are no objections,
I
> will
> >>> >> commit it soon!
> >>> >>
> >>> >> Regards,
> >>> >> Jakob
> >>> >>
> >>> >> 2010/12/9 Jakob Korherr <jakob.korherr@gmail.com>:
> >>> >> > Hi,
> >>> >> >
> >>> >> > I called it ugly, because of its implementation code in
> >>> >> > DefaultFacesConfigurationProvider: The method is already inside
of
> a
> >>> >> > FacesConfigurationProvider, but it does this:
> >>> >> >
> >>> >> > FacesConfigurationProvider provider =
> >>> >> > FacesConfigurationProviderFactory.
> >>> >> >            getFacesConfigurationProviderFactory(_externalContext).
> >>> >> >                getFacesConfigurationProvider(_externalContext);
> >>> >> >
> >>> >> > ...and then calls all the other methods of
> FacesConfigurationProvider
> >>> >> > on this provider instance.
> >>> >> >
> >>> >> > As I said, I know this is this way, because
> >>> >> > FacesConfigurationProvider
> >>> >> > can be wrapped, but IMHO it is really ugly.
> >>> >> >
> >>> >> >
> >>> >> > The method used on MYFACES-2998 was a first approach to solve
this
> >>> >> > problem in a better way. However, I really like those two
of your
> >>> >> > suggestions:
> >>> >> >
> >>> >> > 1) Leo #2: Create another SPI interface for getFacesConfigData()
> >>> >> > (please suggest a name for it, maybe
> >>> >> > FacesConfigurationMergerProvider?) and remove this method
form
> >>> >> > FacesConfigurationProvider.
> >>> >> > 2) Ivan: In a few words: let getFacesConfigData() on
> >>> >> > FacesConfigurationProvider, but provide an
> >>> >> > AbstractFacesConfigurationProvider which implements the merging
> and
> >>> >> > sorting to let custom SPI impls take advantage of it.
> >>> >> >
> >>> >> > At first, I really liked Ivan's proposal, but after thinking
more
> >>> >> > about it, it is not consistent with what we have right now
(we do
> not
> >>> >> > provide any other Abstract-SPI impl) and we would have the
huge
> >>> >> > merging and sorting code all in the SPI(-api) package, but
IMO it
> >>> >> > should really go into o.a.m.config.
> >>> >> >
> >>> >> > Thus I think that a new FacesConfigurationMergerProvider-SPI
as
> Leo
> >>> >> > proposed is the best choice here. Note that for this SPI it
is
> good
> >>> >> > practice to use other SPI impls.
> >>> >> >
> >>> >> > I will provide a patch for it soon and then we can discuss
it
> >>> >> > further!
> >>> >> >
> >>> >> > Regards,
> >>> >> > Jakob
> >>> >> >
> >>> >> > 2010/12/9 Ivan <xhhsld@gmail.com>:
> >>> >> >> my 2 cents, it seems for me less urgly ...
> >>> >> >> a. For the FacesConfigurationProvider , it is better to
have only
> >>> >> >> one
> >>> >> >> method
> >>> >> >> getFacesConfigData()
> >>> >> >> b. Create another abstract class
> AbstractFacesConfigurationProvider
> >>> >> >> which
> >>> >> >> extends FacesConfigurationProvider, and define those proctected
> >>> >> >> methods
> >>> >> >> of
> >>> >> >> get***, also place those sorting/merging codes there.
> >>> >> >> c. In the DefaultFacesConfigurationProvider, it only implements
> >>> >> >> those
> >>> >> >> get***
> >>> >> >> methods.
> >>> >> >> thanks.
> >>> >> >>
> >>> >> >> 2010/12/9 Leonardo Uribe <lu4242@gmail.com>
> >>> >> >>>
> >>> >> >>> Hi
> >>> >> >>>
> >>> >> >>> I agree with Jakob about faces-config merging and
ordering
> >>> >> >>> algorithm
> >>> >> >>> should not be exposed by MyFaces. Why is it not enough?.
At this
> >>> >> >>> point
> >>> >> >>> it is
> >>> >> >>> not clear the reasons. Note in this moment ordering
and sorting
> >>> >> >>> algoritm it
> >>> >> >>> is not being exposed by FacesConfigurationProvider
interface.
> >>> >> >>>
> >>> >> >>> Other different thing is
> >>> >> >>> FacesConfigurationProvider.getFacesConfigData().
> >>> >> >>> The intention of this method is provide a way to get
a
> Serializable
> >>> >> >>> object
> >>> >> >>> that represents all config information required to
initialize
> >>> >> >>> MyFaces
> >>> >> >>> and
> >>> >> >>> allow container to store it temporally somewere. In
this way it
> is
> >>> >> >>> possible
> >>> >> >>> to deploy and undeploy an application more quickly,
because if
> >>> >> >>> "nothing
> >>> >> >>> changes"(no added dependencies, no update from faces-config.xml
> >>> >> >>> files
> >>> >> >>> or
> >>> >> >>> classes) this information is always the same.
> >>> >> >>>
> >>> >> >>> On MYFACES-2945 and previous discussions it was proposed
two
> >>> >> >>> different
> >>> >> >>> options:
> >>> >> >>>
> >>> >> >>> 1. Add getFacesConfigData()
> >>> >> >>> 2. Add a set of methods to retrieve FacesConfig objects
instead.
> >>> >> >>>
> >>> >> >>>     public abstract FacesConfig
> >>> >> >>> getStandardFacesConfig(ExternalContext
> >>> >> >>> ectx);
> >>> >> >>>     public abstract FacesConfig
> >>> >> >>> getMetaInfServicesFacesConfig(ExternalContext ectx);
> >>> >> >>>     public abstract FacesConfig
> >>> >> >>> getAnnotationsFacesConfig(ExternalContext
> >>> >> >>> ectx, boolean metadataComplete);
> >>> >> >>>     public abstract List<FacesConfig>
> >>> >> >>> getClassloaderFacesConfig(ExternalContext ectx);
> >>> >> >>>     public abstract List<FacesConfig>
> >>> >> >>> getContextSpecifiedFacesConfig(ExternalContext ectx);
> >>> >> >>>     public abstract FacesConfig
> >>> >> >>> getWebAppFacesConfig(ExternalContext
> >>> >> >>> ectx);
> >>> >> >>>
> >>> >> >>> The first option has the advantage that it fill the
initial
> >>> >> >>> requeriment
> >>> >> >>> without add more complexity to the problem. The second
one seems
> to
> >>> >> >>> be
> >>> >> >>> more
> >>> >> >>> structured and opens the possibility to do other things
like how
> >>> >> >>> override
> >>> >> >>> MyFaces parsing for faces-config.xml files like it
is being
> >>> >> >>> discussed.
> >>> >> >>> If
> >>> >> >>> the container parse faces-config.xml files, with the
previous
> >>> >> >>> methods
> >>> >> >>> it is
> >>> >> >>> possible to prevent parse the same files twice.
> >>> >> >>>
> >>> >> >>> My first intention, as is shown on MYFACES-2945 was
that
> >>> >> >>> FacesConfigurationProvider does not included
> getFacesConfigData(),
> >>> >> >>> because
> >>> >> >>> it is possible to fill the initial objective with
these methods,
> >>> >> >>> but
> >>> >> >>> finally
> >>> >> >>> it was agreed the first option looks better, right?
> >>> >> >>>
> >>> >> >>> Now I see that on MYFACES-2998 that fact is questioned:
> >>> >> >>>
> >>> >> >>> JK>> Unfortunately it also provides a method
that should combine
> >>> >> >>> all
> >>> >> >>> these
> >>> >> >>> data: getFacesConfigData().
> >>> >> >>> JK>> This method is here due to refactorings,
but IMHO this is
> the
> >>> >> >>> last
> >>> >> >>> place where it should be put,
> >>> >> >>> JK>> because it gets "its own implementation"
via its Factory
> and
> >>> >> >>> then
> >>> >> >>> calls all of the above methods on
> >>> >> >>> JK>> it. I know this was introduced to support
wrapping the
> default
> >>> >> >>> impl,
> >>> >> >>> but it really is very, very ugly.
> >>> >> >>>
> >>> >> >>> In few words, why does it looks ugly? or with other
words, what
> can
> >>> >> >>> we
> >>> >> >>> do
> >>> >> >>> to make it cleaner? remove it? or just provide another
SPI
> >>> >> >>> interface
> >>> >> >>> and put
> >>> >> >>> that method there? In practice, getFacesConfigData()
merges all
> >>> >> >>> FacesConfig
> >>> >> >>> information, and "on the way" it does order
> applicationFacesConfig
> >>> >> >>> files
> >>> >> >>> (the ones obtained from getClassloaderFacesConfig()
and
> >>> >> >>> getContextSpecifiedFacesConfig() ) . To do that it
requires to
> call
> >>> >> >>> all six
> >>> >> >>> methods from FacesConfigurationProvider, there is
no other way,
> so
> >>> >> >>> I
> >>> >> >>> don't
> >>> >> >>> see why do that is considered ugly.
> >>> >> >>>
> >>> >> >>> At this moment we have the following courses of action:
> >>> >> >>>
> >>> >> >>> 1. Remove FacesConfigurationResource interface partially,
> because
> >>> >> >>> it
> >>> >> >>> is
> >>> >> >>> still too inmature and let it for myfaces core 2.0.4.
> >>> >> >>> 2. Create another SPI interface for getFacesConfigData()
(please
> >>> >> >>> suggest a
> >>> >> >>> name for it, maybe FacesConfigurationMergerProvider?)
and remove
> >>> >> >>> this
> >>> >> >>> method
> >>> >> >>> form FacesConfigurationResource. Apply the patch on
MYFACES-2998
> >>> >> >>> seems
> >>> >> >>> to be
> >>> >> >>> in this direction, but forget the reason why it is
wanted to
> expose
> >>> >> >>> getFacesConfigData() to the container.
> >>> >> >>> 3. Apply something like MYFACES-2998 patch, and refactor
this
> one
> >>> >> >>> later in
> >>> >> >>> myfaces core 2.0.4
> >>> >> >>>
> >>> >> >>> Suggestions are welcome.
> >>> >> >>>
> >>> >> >>> regards,
> >>> >> >>>
> >>> >> >>> Leonardo Uribe
> >>> >> >>>
> >>> >> >>>
> >>> >> >>>
> >>> >> >>>
> >>> >> >>
> >>> >> >>
> >>> >> >>
> >>> >> >> --
> >>> >> >> Ivan
> >>> >> >>
> >>> >> >
> >>> >> >
> >>> >> >
> >>> >> > --
> >>> >> > Jakob Korherr
> >>> >> >
> >>> >> > blog: http://www.jakobk.com
> >>> >> > twitter: http://twitter.com/jakobkorherr
> >>> >> > work: http://www.irian.at
> >>> >> >
> >>> >>
> >>> >>
> >>> >>
> >>> >> --
> >>> >> Jakob Korherr
> >>> >>
> >>> >> blog: http://www.jakobk.com
> >>> >> twitter: http://twitter.com/jakobkorherr
> >>> >> work: http://www.irian.at
> >>> >
> >>> >
> >>>
> >>>
> >>>
> >>> --
> >>> Jakob Korherr
> >>>
> >>> blog: http://www.jakobk.com
> >>> twitter: http://twitter.com/jakobkorherr
> >>> work: http://www.irian.at
> >>
> >>
> >
> >
> >
> > --
> > Jakob Korherr
> >
> > blog: http://www.jakobk.com
> > twitter: http://twitter.com/jakobkorherr
> > work: http://www.irian.at
> >
>
>
>
> --
> Jakob Korherr
>
> blog: http://www.jakobk.com
> twitter: http://twitter.com/jakobkorherr
> work: http://www.irian.at
>



-- 
Ivan

Mime
View raw message