geronimo-dev mailing list archives

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