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.korh...@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.korh...@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 <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 > > > > > > -- > Jakob Korherr > > blog: http://www.jakobk.com > twitter: http://twitter.com/jakobkorherr > work: http://www.irian.at >
