MyFaces Core
  1. MyFaces Core
  2. MYFACES-3520

False evaluation of variables/params with the same name (c:forEach "var" and "varStatus" should be page scoped)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.6
    • Fix Version/s: 2.0.15, 2.1.9
    • Component/s: None
    • Labels:
      None

      Description

      I have an scenario where an xhml is included. The included file has a parameter with the same name as some outer variable. In the included file the parameter is ignored and the outer variable is used.

      Some xhtml:
      <c:forEach var="item" begin="1" end="3">
      <div>
      <ui:include src="templateContextTestInclude.xhtml">
      <ui:param name="item" value="#

      {item + 10}

      " />
      </ui:include>
      </div>
      </c:forEach>

      templateContextTestInclude.xhtml:
      <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
      "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
      <html xmlns="http://www.w3.org/1999/xhtml"
      xmlns:ui="http://java.sun.com/jsf/facelets"
      xmlns:h="http://java.sun.com/jsf/html"
      xmlns:f="http://java.sun.com/jsf/core"
      xmlns:c="http://java.sun.com/jsp/jstl/core"
      xmlns:t="http://myfaces.apache.org/tomahawk">

      <body>
      <ui:composition>
      Item: <h:outputText value="#

      {item}

      " /><br/>
      </ui:composition>
      </body>
      </html>

      I found out that the "hierarchy" of VariableMappers is asked for a value before the 'DefaultVariableMapper' tests the current template/page context. That was not what I expected. Is that how it should be?
      In JSF1 it works that way.

      (If I change the VariableMapperWrapper locally to test the template/page context first the behaviour is as I would have expected (don't know if it is the right place ):
      public ValueExpression resolveVariable(String variable)
      {

      AbstractFaceletContext faceletContext = (AbstractFaceletContext) FacesContext.getCurrentInstance().getAttributes().get(FaceletContext.FACELET_CONTEXT_KEY);

      //Check on page and template context
      PageContext pageContext = faceletContext.getPageContext();
      if (pageContext != null && pageContext.getAttributeCount() > 0)
      {
      if (pageContext.getAttributes().containsKey(variable))
      {
      ValueExpression returnValue = pageContext.getAttributes().get(variable);
      if (_trackResolveVariables)

      { _variableResolved = true; }
      return returnValue;
      }
      }

      TemplateContext templateContext = faceletContext.getTemplateContext();
      if (templateContext != null && !templateContext.isParameterEmpty())
      {
      if (templateContext.getParameterMap().containsKey(variable))
      {
      ValueExpression returnValue = templateContext.getParameter(variable);
      if (_trackResolveVariables)
      { _variableResolved = true; }

      return returnValue;
      }
      }

      ValueExpression ve = null;
      ....
      )

      Thanks in advance,
      dennis

      1. MYFACES-3520-1.patch
        23 kB
        Leonardo Uribe

        Activity

        Hide
        Leonardo Uribe added a comment -

        The problem with this one is c:forEach relies on VariableResolver, and that is a bad idea by some reasons:

        • It can "pass" across everything, including other templates and composite components and breaks encapsulation principle.
        • The value is statically stored into the inner value expressions, and that means they can't be cached and reused across pages.

        My advice is replace c:forEach with t:dataTable or t:dataList with rowStatePreserved="true". They are more flexible as described in:

        http://lu4242.blogspot.com/2011/06/jsf-component-state-per-row-for.html

        Additionally, avoiding c:forEach eliminates the effect over state size.

        Unfortunately, after a lot of changes done in myfaces core, the conclusion is previous c:forEach behavior is not fixable. It is possible to use the template context map to store the var and disable EL expression caching, but anyway c:forEach is too inconvenient.

        Change the order of VariableResolver stuff is reasonable but is also not wanted. In fact, remove any local VariableResolver usage inside facelets algorithm is preferred, so it is just there for backward compatibility. Note myfaces changed the way c:set and ui:param works to match better the spec.

        So, the question is if we change c:forEach to use "template context scope" instead. Sounds reasonable. c:forEach var should not pass through ui:include or composite components. In this case, ui:include creates another template context, so with that the problem is fixed, and since the expression is still resolved through VariableResolver, the change will not break anything.

        Show
        Leonardo Uribe added a comment - The problem with this one is c:forEach relies on VariableResolver, and that is a bad idea by some reasons: It can "pass" across everything, including other templates and composite components and breaks encapsulation principle. The value is statically stored into the inner value expressions, and that means they can't be cached and reused across pages. My advice is replace c:forEach with t:dataTable or t:dataList with rowStatePreserved="true". They are more flexible as described in: http://lu4242.blogspot.com/2011/06/jsf-component-state-per-row-for.html Additionally, avoiding c:forEach eliminates the effect over state size. Unfortunately, after a lot of changes done in myfaces core, the conclusion is previous c:forEach behavior is not fixable. It is possible to use the template context map to store the var and disable EL expression caching, but anyway c:forEach is too inconvenient. Change the order of VariableResolver stuff is reasonable but is also not wanted. In fact, remove any local VariableResolver usage inside facelets algorithm is preferred, so it is just there for backward compatibility. Note myfaces changed the way c:set and ui:param works to match better the spec. So, the question is if we change c:forEach to use "template context scope" instead. Sounds reasonable. c:forEach var should not pass through ui:include or composite components. In this case, ui:include creates another template context, so with that the problem is fixed, and since the expression is still resolved through VariableResolver, the change will not break anything.
        Hide
        dennis hoersch added a comment -

        Sorry, I don't get any notice about your reply.

        "c:forEach var should not pass through ui:include or composite components.": Yes, that was what I expected.

        At the moment I have no idea how to change c:forEach that way to test it...

        Can you give any hints?

        Show
        dennis hoersch added a comment - Sorry, I don't get any notice about your reply. "c:forEach var should not pass through ui:include or composite components.": Yes, that was what I expected. At the moment I have no idea how to change c:forEach that way to test it... Can you give any hints?
        Hide
        Leonardo Uribe added a comment -

        Patch attached with junit tests. See related discussion in:

        http://markmail.org/message/k2oytkqoi6q4uu7i?q=MYFACES-3520

        [core] fix c:forEach var and varStatus scope (MYFACES-3520)

        Show
        Leonardo Uribe added a comment - Patch attached with junit tests. See related discussion in: http://markmail.org/message/k2oytkqoi6q4uu7i?q=MYFACES-3520 [core] fix c:forEach var and varStatus scope ( MYFACES-3520 )

          People

          • Assignee:
            Leonardo Uribe
            Reporter:
            dennis hoersch
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development