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 Thu, 09 Dec 2010 11:35:47 GMT
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

Mime
View raw message