You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geronimo.apache.org by Ivan <xh...@gmail.com> on 2010/12/08 03:05:16 UTC
Re: ordering tck failures in geronimo
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 <lu...@gmail.com>
> Hi
>
> 2010/12/7 David Jencks <da...@yahoo.com>
>
>>
>> On Dec 7, 2010, at 10:12 AM, Leonardo Uribe wrote:
>>
>> Hi
>>
>> 2010/12/6 Ivan <xh...@gmail.com>
>>
>> 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 <da...@yahoo.com>
>>
>>>
>>>
>>> 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 <lu...@gmail.com>
>>>
>>>> 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 <xh...@gmail.com>
>>>>
>>>>>
>>>>>
>>>>> 2010/12/6 David Jencks <da...@yahoo.com>
>>>>>
>>>>>>
>>>>>> 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
Re: ordering tck failures in geronimo
Posted by Leonardo Uribe <lu...@gmail.com>.
Hi
Good to know that, thanks. In these moments I'm running the necessary steps
for send the vote, so I hope to release these artifacts this week.
regards,
Leonardo Uribe
2010/12/13 Ivan <xh...@gmail.com>
> 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 <ja...@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 <ja...@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 <lu...@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 <ja...@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 <lu...@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 <ja...@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 <ja...@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 <xh...@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 <lu...@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
>
Re: ordering tck failures in geronimo
Posted by Leonardo Uribe <lu...@gmail.com>.
Hi
Good to know that, thanks. In these moments I'm running the necessary steps
for send the vote, so I hope to release these artifacts this week.
regards,
Leonardo Uribe
2010/12/13 Ivan <xh...@gmail.com>
> 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 <ja...@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 <ja...@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 <lu...@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 <ja...@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 <lu...@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 <ja...@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 <ja...@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 <xh...@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 <lu...@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
>
Re: ordering tck failures in geronimo
Posted by Ivan <xh...@gmail.com>.
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 <ja...@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 <ja...@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 <lu...@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 <ja...@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 <lu...@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 <ja...@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 <ja...@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 <xh...@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 <lu...@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
Re: ordering tck failures in geronimo
Posted by Ivan <xh...@gmail.com>.
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 <ja...@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 <ja...@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 <lu...@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 <ja...@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 <lu...@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 <ja...@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 <ja...@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 <xh...@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 <lu...@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
Re: ordering tck failures in geronimo
Posted by Jakob Korherr <ja...@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 <ja...@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 <lu...@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 <ja...@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 <lu...@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 <ja...@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 <ja...@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 <xh...@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 <lu...@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
Re: ordering tck failures in geronimo
Posted by Jakob Korherr <ja...@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 <ja...@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 <lu...@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 <ja...@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 <lu...@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 <ja...@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 <ja...@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 <xh...@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 <lu...@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
Re: ordering tck failures in geronimo
Posted by Jakob Korherr <ja...@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 <lu...@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 <ja...@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 <lu...@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 <ja...@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 <ja...@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 <xh...@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 <lu...@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
Re: ordering tck failures in geronimo
Posted by Jakob Korherr <ja...@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 <lu...@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 <ja...@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 <lu...@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 <ja...@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 <ja...@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 <xh...@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 <lu...@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
Re: ordering tck failures in geronimo
Posted by Leonardo Uribe <lu...@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 <ja...@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 <lu...@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 <ja...@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 <ja...@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 <xh...@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 <lu...@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
>
Re: ordering tck failures in geronimo
Posted by Leonardo Uribe <lu...@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 <ja...@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 <lu...@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 <ja...@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 <ja...@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 <xh...@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 <lu...@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
>
Re: ordering tck failures in geronimo
Posted by Jakob Korherr <ja...@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 <lu...@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 <ja...@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 <ja...@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 <xh...@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 <lu...@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
Re: ordering tck failures in geronimo
Posted by Jakob Korherr <ja...@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 <lu...@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 <ja...@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 <ja...@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 <xh...@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 <lu...@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
Re: ordering tck failures in geronimo
Posted by Leonardo Uribe <lu...@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 <ja...@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 <ja...@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 <xh...@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 <lu...@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
>
Re: ordering tck failures in geronimo
Posted by Jakob Korherr <ja...@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 <ja...@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 <xh...@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 <lu...@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
Re: ordering tck failures in geronimo
Posted by Jakob Korherr <ja...@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 <ja...@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 <xh...@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 <lu...@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
Re: ordering tck failures in geronimo
Posted by Jakob Korherr <ja...@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 <xh...@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 <lu...@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
Re: ordering tck failures in geronimo
Posted by Jakob Korherr <ja...@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 <xh...@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 <lu...@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
Re: ordering tck failures in geronimo
Posted by Ivan <xh...@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 <lu...@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
Re: ordering tck failures in geronimo
Posted by Ivan <xh...@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 <lu...@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
Re: ordering tck failures in geronimo
Posted by Leonardo Uribe <lu...@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
Re: ordering tck failures in geronimo
Posted by Leonardo Uribe <lu...@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
Re: ordering tck failures in geronimo
Posted by Jakob Korherr <ja...@gmail.com>.
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 <xh...@gmail.com>:
> 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 <lu...@gmail.com>
>>
>> Hi
>>
>> 2010/12/7 David Jencks <da...@yahoo.com>
>>>
>>> On Dec 7, 2010, at 10:12 AM, Leonardo Uribe wrote:
>>>
>>> Hi
>>>
>>> 2010/12/6 Ivan <xh...@gmail.com>
>>>
>>>> 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 <da...@yahoo.com>
>>>>
>>>>
>>>> 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 <lu...@gmail.com>
>>>>>
>>>>> 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 <xh...@gmail.com>
>>>>>>
>>>>>>
>>>>>> 2010/12/6 David Jencks <da...@yahoo.com>
>>>>>>>
>>>>>>> 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
Re: ordering tck failures in geronimo
Posted by Jakob Korherr <ja...@gmail.com>.
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 <xh...@gmail.com>:
> 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 <lu...@gmail.com>
>>
>> Hi
>>
>> 2010/12/7 David Jencks <da...@yahoo.com>
>>>
>>> On Dec 7, 2010, at 10:12 AM, Leonardo Uribe wrote:
>>>
>>> Hi
>>>
>>> 2010/12/6 Ivan <xh...@gmail.com>
>>>
>>>> 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 <da...@yahoo.com>
>>>>
>>>>
>>>> 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 <lu...@gmail.com>
>>>>>
>>>>> 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 <xh...@gmail.com>
>>>>>>
>>>>>>
>>>>>> 2010/12/6 David Jencks <da...@yahoo.com>
>>>>>>>
>>>>>>> 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