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
