Thanks for tacking this, Leonardo. I am not sure I understand the changes. Moving methods to the FacesConfigurationProvider is required, but it seems to me that it is not enough. With this spi, do I still need to provide those sorting/merging/... methods ? The only way I could see now is that, a. extend the wrapper class b. Use FacesConfigurationProviderFactory to get an instance of FacesConfigurationProvider. But that seems a little strange to me ...
I also moved this thread from geronimo-tck to dev-geronimo, as current discussion is not related to tck, and no related message in current email. thanks. 2010/12/8 Leonardo Uribe <[email protected]> > Hi > > 2010/12/7 David Jencks <[email protected]> > >> >> On Dec 7, 2010, at 10:12 AM, Leonardo Uribe wrote: >> >> Hi >> >> 2010/12/6 Ivan <[email protected]> >> >> So, at least could you please help to add the export the >>> org.apache.myfaces.config package in 2.0.3, that will make my life easier >>> now. And remove it once those methods are moved to >>> FacesConfigurationProvider in the future. >>> >> >> Instead do that, it is better to add them now. (I committed it on >> MYFACES-2945) It is a change already studied, so I don't see any problem >> doing it. I did some small changes too, so it could be good if you try it to >> see if everything is ok (I'll wait for your response) . >> >> 2010/12/7 David Jencks <[email protected]> >> >>> >>> >>> I have not compared the myfaces classes with the openejb jaxb tree for >>> faces-config.xml, so I have no idea how plausible this idea might be. Would >>> it be possible to annotate the myfaces object tree representing >>> faces-config.xml so that it could be read in by jaxb as well as by digester? >>> >>> For use in geronimo, I wonder if putting some or all of this code in a >>> fragment bundle hosted by the myfaces bundle would reduce the number of >>> exports needed. >>> >>> >> I don't think so, because do that requires to add a dependency to jaxb on >> myfaces impl, and we have one xml library dependency already >> (commons-digester). >> >> >> Classes containing annotations that aren't available at runtime should >> still load fine, so this would be a compile time dependency but optional at >> runtime. It would possibly make the commons-digester dependency optional at >> runtime as well (at least with non-myfaces additional code that uses jaxb) >> >> > I have never tried, but what I understand is only annotations with > @Retention(RetentionPolicy.SOURCE) are discarded by the compiler. But if it > is possible to add jaxb as an optional dependency, yes, myfaces could > include those annotations. Anyway in this moment it is just a possibility, > so I will not believe it until I see it. Note JSF 2.0 is JDK 1.5 compatible. > That means we should take care on keep MyFaces working in 1.5. > > regards, > > Leonardo Uribe > > >> thanks >> david jencks >> >> >> regards, >> >> Leonardo URibe >> >> >>> thanks >>> david jencks >>> >>> >>> For item b, I created a sub classes of DefaultFacesConfigurationProvider, >>> and just overrides those get*** methods, the class is look like : >>> ---> >>> public class GeronimoFacesConfigurationProvider extends >>> DefaultFacesConfigurationProvider { >>> >>> private FacesConfig annotationsFacesConfig; >>> >>> private List<FacesConfig> classloaderFacesConfigs; >>> >>> private List<FacesConfig> contextSpecifiedFacesConfigs; >>> >>> private FacesConfig webAppFacesConfig; >>> >>> private FacesConfig standardFacesConfig; >>> >>> public GeronimoFacesConfigurationProvider(FacesConfig >>> standardFacesConfig, FacesConfig webAppFacesConfig, FacesConfig >>> annotationsFacesConfig, List<FacesConfig> classloaderFacesConfigs, >>> List<FacesConfig> contextSpecifiedFacesConfigs) { >>> this.annotationsFacesConfig = annotationsFacesConfig; >>> this.classloaderFacesConfigs = classloaderFacesConfigs; >>> this.contextSpecifiedFacesConfigs = contextSpecifiedFacesConfigs; >>> this.webAppFacesConfig = webAppFacesConfig; >>> this.standardFacesConfig = standardFacesConfig; >>> } >>> >>> @Override >>> protected FacesConfig getAnnotationsFacesConfig(ExternalContext ectx, >>> boolean metadataComplete) { >>> return annotationsFacesConfig; >>> } >>> >>> @Override >>> protected List<FacesConfig> getClassloaderFacesConfig(ExternalContext >>> externalContext) { >>> return classloaderFacesConfigs; >>> } >>> >>> @Override >>> protected List<FacesConfig> >>> getContextSpecifiedFacesConfig(ExternalContext externalContext) { >>> return contextSpecifiedFacesConfigs; >>> } >>> >>> @Override >>> protected FacesConfig getStandardFacesConfig(ExternalContext >>> externalContext) { >>> return standardFacesConfig; >>> } >>> >>> @Override >>> protected FacesConfig getWebAppFacesConfig(ExternalContext >>> externalContext) { >>> return webAppFacesConfig; >>> } >>> >>> } >>> <--- >>> Thoughts ? >>> thanks. >>> >>> 2010/12/7 Leonardo Uribe <[email protected]> >>> >>>> Hi >>>> >>>> First of all, I want to note that if the default algorithm for >>>> FacesConfigResourceProvider is not able to find a.faces-config.xml and >>>> c.faces-config.xml, that means the Thread Context Class Loader needs to be >>>> fixed, because is not taking into account jar files under WEB-INF/lib. >>>> >>>> 2010/12/6 Ivan <[email protected]> >>>> >>>>> >>>>> >>>>> 2010/12/6 David Jencks <[email protected]> >>>>> >>>>>> >>>>>> On Dec 5, 2010, at 7:44 PM, Ivan wrote: >>>>>> >>>>>> > I am wondering whether the myfaces bundle could also export the >>>>>> spi.impl package, I hope to take advantage of some codes there, e.g. >>>>>> DefaultFacesConfigurationProvider, it contains some sort algorithm. >>>>>> > thanks. >>>>>> > >>>>>> >>>>>> >>>> Why export spi.impl package? the idea to have an spi api and impl is >>>> allow impl package to change and let api stable to prevent break code >>>> later, >>>> right? If something should be exposed from >>>> DefaultFacesConfigurationProvider >>>> (for example adding some methods on FacesConfigurationProvider), it should >>>> be discussed first. >>>> >>>> I agree to expose this two: >>>> >>>> >>>> + >>>> org.apache.myfaces.config.impl.digester.elements;version="${project.version}", >>>> + >>>> org.apache.myfaces.config.impl.digester;version="${project.version} >>>> >>>> because theorically the code there will not change (only between >>>> different versions of JSF), but the other ones: >>>> >>>> >>>> + >>>> org.apache.myfaces.spi.impl;version="${project.version}", >>>> + >>>> org.apache.myfaces.config;version="${project.version}", >>>> >>>> needs some argumentation first. >>>> >>>> >>>>> I'd say if you need to expose the default implementation of the spi >>>>>> then there is something wrong with the interface design. If the sorting >>>>>> is >>>>>> universal then perhaps it should not be in a class implemented by a >>>>>> service >>>>>> provider? >>>>>> >>>>> >>>>> >>>> Ok, as long as I undersand there is no reason why expose sorting >>>> algorithm to the container. Also, allow a different parser for FacesConfig >>>> was discussed before. I remember on MYFACES-2945 that it was proposed an >>>> interface with this methods: >>>> >>>> public abstract class FacesConfigurationProvider >>>> { >>>> 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); >>>> >>>> } >>>> >>>> But finally the final form was this: >>>> >>>> public abstract class FacesConfigurationProvider >>>> { >>>> >>>> public abstract FacesConfigData getFacesConfigData(ExternalContext >>>> ectx); >>>> >>>> } >>>> >>>> And this comment was added: >>>> >>>> "...After some attempts, the structure of the solution is not too >>>> different from the previous patch, because after all in >>>> DefaultFacesConfigurationProvider it is necessary to put all previous code >>>> plus ordering and feeding of FacesConfig files...." >>>> >>>> Note the first form allows to define an custom parser, because to >>>> retrieve FacesConfig you should locate them first and then parse them, but >>>> the second one does not. >>>> >>>> Yes, in Geronimo, we have a sepearted algorthim for sorting. >>>>> Currently, I still use the implementation from MyFaces to do it, maybe in >>>>> the future I could move them to Geronimo side or it will be abstracted >>>>> from >>>>> current default spi provider. I also need to provide a mock external >>>>> context >>>>> to make it work. or I might need to copy some codes from myfaces. >>>>> Another thing is that, I use the digester xml parser from MyFaces to >>>>> parse all the faces configuration files, while we using jaxb tree in the >>>>> past. This could be avoid, just did not want to add some codes to convert >>>>> two FacesConfig objects. >>>>> >>>> >>>> The idea behind refactor org.apache.myfaces.config.element package is >>>> give some base classes from myfaces, to allow use a different parser. Those >>>> changes were committed, but to allow that it is still necessary to commit >>>> some methods on FacesConfigurationProvider to do the "bridge". >>>> >>>> regards, >>>> >>>> Leonardo Uribe >>>> >>>> Thanks. >>>>> >>>>> >>>>>> thanks >>>>>> david jencks >>>>>> >>>>>> > >>>>>> > -- >>>>>> > Ivan >>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> Ivan >>>>> >>>> >>>> >>> >>> >>> -- >>> Ivan >>> >>> >>> >> >> > -- Ivan
