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 <xhh...@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 <lu4...@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
