geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Leonardo Uribe <lu4...@gmail.com>
Subject Re: ordering tck failures in geronimo
Date Fri, 10 Dec 2010 15:51:25 GMT
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
>

Mime
View raw message