Leonardo Uribe schrieb:
On Wed, Aug 13, 2008 at 3:48 PM, Leonardo Uribe <[EMAIL PROTECTED]
<mailto:[EMAIL PROTECTED]>> wrote:
On Wed, Aug 13, 2008 at 4:12 AM, [EMAIL PROTECTED]
<mailto:[EMAIL PROTECTED]> <[EMAIL PROTECTED]
<mailto:[EMAIL PROTECTED]>> wrote:
Matthias Wessendorf schrieb:
Simon,
On Wed, Aug 13, 2008 at 10:53 AM, Simon Kitching
<[EMAIL PROTECTED] <mailto:[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).
Thanks for the very clear description above.
And thanks for the changes you have made so far.
I know very little about portlets, but assume that a portlet engine
typically is deployed as a servlet within a servlet container, and then
internally invokes the relevant portlets, providing the PortletContext
object etc? And that what we are talking about here with respect to
extracting the ExtensionFilter config params is this kind of setup?
The docs for PortletContext that I have found say:
<quote>
There is one context per "portlet application" per Java Virtual Machine.
(A "portlet application" is a collection of portlets, servlets, and
content installed under a specific subset of the server URL namespace,
such as |/catalog|. They are possibly installed via a |.war| file.) As a
web application, a portlet application also has a servlet context. The
portlet context leverages most of its functionality from the servlet
context of the portlet application.
Attibutes stored in the context are global for /all/ users and /all/
components in the portlet application.
</quote>
So somePortletContext.getAttribute() accesses "application scope" data
common to all portlets.
And ServletContext.getAttribute() is also "application scope" data.
Aren't they the *same* application scope in the setup we are talking
about here, ie if the ExtensionsFilter calls
ServletContext.setAttribute("foo", "bar");
then when something calls
PortletContext.getAttribute("foo");
doesn't that return "bar"?
Regards,
Simon
