On Wed, Aug 13, 2008 at 3:48 PM, Leonardo Uribe <[EMAIL PROTECTED]> wrote:
> > > On Wed, Aug 13, 2008 at 4:12 AM, [EMAIL PROTECTED] < > [EMAIL PROTECTED]> wrote: > >> Matthias Wessendorf schrieb: >> >> Simon, >>> >>> On Wed, Aug 13, 2008 at 10:53 AM, Simon Kitching <[EMAIL PROTECTED]> >>> wrote: >>> >>> >>>> Hi, >>>> >>>> Just a reminder of the email I posted a few weeks ago: I'm still very >>>> concerned about the recent ExtensionsFilter refactoring. >>>> >>>> The new code now parses the web.xml to extract some configuration >>>> information. This just feels wrong to me. >>>> The new code also is based around a custom FacesContext wrapper. I'm >>>> worried >>>> about possible interactions between this and other libraries that also >>>> wrap >>>> FacesContext. And I'm worried about the portability of this code to the >>>> upcoming JSF2.0. >>>> >>>> I know this stuff is needed to get tomahawk useable for portlets. But >>>> it's >>>> very important not to break existing users. >>>> >>>> What I would like to see is: >>>> * separate out the resource-serving stuff into a separate utilities >>>> module, >>>> that can be called from either a filter or a FacesContext. >>>> And call the common code from both ExtensionsFilter and >>>> TomahawkFacesContextWrapper. >>>> * have the TomahawkFacesContextFactory create a FacesContext wrapper >>>> only in >>>> the case where the filter is not configured, ie >>>> look in the request-scope for a flag placed there by the filter. That >>>> means >>>> there is no chance of breaking existing setups where >>>> the filter is defined explicitly. >>>> * ideally, also have some way of turning off the faces-context wrapping >>>> even >>>> when the filter is not present. >>>> * don't parse the web.xml at all. I know you said earlier that it is >>>> needed, >>>> but I just don't see why. In any case, I think we *must* >>>> find some other solution; the current approach is really hard to >>>> maintain. >>>> >>>> Until this is fixed, I would definitely vote -1 on any tomahawk release. >>>> It's a major issue. >>>> >>>> >>> >>> was there a jira issue for it ? >>> or can you point me to the svn rev? >>> >>> >> The best thing to do is to look at these classes: >> org.apache.myfaces.webapp.filter.ExtensionsFilter >> org.apache.myfaces.webapp.filter.TomahawkFacesContextFactory >> org.apache.myfaces.webapp.filter.TomahawkFacesContextWrapper >> org.apache.myfaces.webapp.filter.ExtensionsFilterConfig [1] >> >> The svn history of these files references MYFACES-434. >> There is no specific jira issue about my concerns; it is really an >> architecture/design disagreement rather than a specific bug. >> >> [1] ExtensionsFilterConfig.getExtensionsFilterConfig is invoked from >> TomahawkFacesContextWrapper. >> >> Regards, Simon >> >> > Hi > > I'll do a simple review (just to make clear the discussion). > ExtensionsFilter has the following responsibilities (all we know this but > sometimes is not very clear): > > 1. Wrap a multipart request (used for t:inputFileUpload), so the request > could be correctly decoded. > > 1.1 For do this, it receives some params from web.xml like this: > > <filter> > <filter-name>extensionsFilter</filter-name> > > <filter-class>org.apache.myfaces.webapp.filter.ExtensionsFilter</filter-class> > <init-param> > <param-name>uploadMaxFileSize</param-name> > <param-value>100m</param-value> > </init-param> > <init-param> > <param-name>uploadThresholdSize</param-name> > <param-value>100k</param-value> > </init-param> > </filter> > > 1.2 So, ExtensionsFilter must wrap the request that goes to faces pages > using a filter-mapping like this: > > <filter-mapping> > <filter-name>extensionsFilter</filter-name> > <url-pattern>*.jsf</url-pattern> > </filter-mapping> > <filter-mapping> > <filter-name>extensionsFilter</filter-name> > <url-pattern>/faces/*</url-pattern> > </filter-mapping> > > 2. ExtensionsFilter must serve resources (javascript, css...) of some > tomahawk components. > > 3. If a buffered instance of AddResource is configured, ExtensionsFilter > must buffer and add the resource reference > to the head of jsf pages (for example when it is used DefaultAddResource) > > The changes proposed on MYFACES-434 was the following: > > a. Use a TomahawkFacesContextWrapper to do tasks 1 and 3 of > ExtensionsFilter. It must be read the config params to know > how to wrap the request from web.xml, because this params are on the filter > init-param part. > > b. Add a ServeResourcePhaseListener for do the task 2 of ExtensionsFilter > (In portlets we could not be ExtensionsFilter working). > > c. Wrap the portlet request properly when it is multipart content, so > t:inputFileUpload could work on portlets > (to be done, Scott told me to wait some time). > > Actually, ExtensionsFilter only does task 2 (ServeResourcePhaseListener was > added on faces-config.xml) and tasks 1 and 3 > are done by TomahawkFacesContextWrapper. > > Now the problem. > > It could be some problems when you mix 1.1 libraries with 1.2 code that > wraps FacesContext (I don't remember the myfaces > issue, but the local example of this problem is when you mix orchestra(1.1) > myfaces core(1.2) with tomahawk(1.1.7-SNAPSHOT)). > > In order to anticipate the events, It could be good to remain the old > feature (if ExtensionsFilter is configured and > is working, do not use TomahawkFacesContextWrapper). Sounds good the > suggestions, but in my opinion, there are not > blocking issues for a release. > I have added the param org.apache.myfaces.DISABLE_TOMAHAWK_FACES_CONTEXT_WRAPPER, and if ExtensionsFilter is used before, do not use TomahawkFacesContextWrapper. If someone wants to implement the other suggestions go ahead, but remember do not break anything or reduce the features available right now. It takes a lot of effort and time make things work. > > But I don't see a solution for get the params for initialize the multipart > request wrapper different than parse the web.xml file > I'm open to suggestions, but any solution must remain the actual > behavior(it works and it is easier to users, since > the params has been configured there for a long time). > > regards > > Leonardo Uribe >
