Hi guys, I really see no reason why Geronimo should implement its own faces-config merging and ordering algorithm or even have to think about relying on MyFaces internals. IMHO getFacesConfigData() should be removed from FacesConfigurationProvider, because this algorithm does not have to be customizable and thus should be removed from the SPI.
I created MYFACES-2998 with a more detailed description and a patch for this problem. Please review it and if there are no objections, I will commit the patch soon. I hope this will make your lifes a lot easier! Regards, Jakob 2010/12/8 Ivan <[email protected]>: > 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 > -- Jakob Korherr blog: http://www.jakobk.com twitter: http://twitter.com/jakobkorherr work: http://www.irian.at
