Thanks, Jakob and Leonardo, the new changes work perfect ;-) By the way, I have run the TCK on my local environment, although there are some failure cases, they should be caused by webbeans integration.
2010/12/11 Jakob Korherr <jakob.korh...@gmail.com> > Hi guys, > > As discussed, I just committed a modified version of the proposed patch. > > Furthermore I added the custom SPI impl that I used for testing to the > issue [1] which may help for the real Geronimo impl. > > It would be great if you guys could check if this solution works for > you, and if so, we can close this issue and start the 2.0.3 release! > > Regards, > Jakob > > [1] > https://issues.apache.org/jira/secure/attachment/12466010/GeronimoFacesConfigurationMerger.java > > 2010/12/10 Jakob Korherr <jakob.korh...@gmail.com>: > > Hi Leo, > > > > OK great. That's fine with me. > > > > I will apply the appropriate changes and commit the patch! > > > > Regards, > > Jakob > > > > 2010/12/10 Leonardo Uribe <lu4...@gmail.com>: > >> Hi Jakob > >> > >> I think it is better do not include it as parameter and instead add some > >> comment on the javadoc, > >> saying the information to take into account is retrieved from > >> FacesConfigurationProvider. The fact > >> of use FacesConfigurationProvider is more an implementation detail than > a > >> requeriment, so if > >> somebody wants to do not use it is valid usage. > >> > >> regards, > >> > >> Leonardo Uribe > >> > >> 2010/12/10 Jakob Korherr <jakob.korh...@gmail.com> > >>> > >>> 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 <lu4...@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.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 > >>> > > >>> > > >>> > >>> > >>> > >>> -- > >>> 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 > -- Ivan
