tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <ma...@apache.org>
Subject Re: svn commit: r909979 - /tomcat/tc6.0.x/trunk/STATUS.txt
Date Mon, 15 Feb 2010 08:25:17 GMT
On 14/02/2010 06:47, kkolinko@apache.org wrote:
> +    1. There is a copy-paste issue in the patch:
> +       vec.add(tagVarInfos[i]); call was replaced by vec.add(varInfos[i]);
> +       which is wrong. This error is not present in trunk.
Fixed.

> +    2. isImplemetedAsFragment() method is wrong.
> +       1) A fragment can also be created with <jsp:attribute> when calling a tag,
> +         when the attribute is declared being of type JspFragment.
> +         -see JSP.5.10 <jsp:attribute>, or the places where the following method
is called:
> +         Generator#GeneratorVisitor#generateJspFragment(Node, String)
> +
> +       2) It should navigate up the parents chain. The SimpleTag can be one
> +         of our parents, not necessary the immediate one.
The name might be wrong. usesFragmentHelper might be better as that is
actually what matters and it should be using the exact same logic as
Generator to determine if a fragment helper is being used. If that isn't
the case then there is a bug.

> +    3. I think that BZ 48616 cannot/should not be fixed - see Comment 15 to
> +      the issue.
I disagree with your analysis in that comment (see below). I believe the
bug is valid.

> +    4. Should we fix BZ 42390 in TC 5.5, and bring on BZ 48616 there, if it
> +    was historically working? I have doubts.
Whatever is finally agreed for 6.0.x should be back-ported to 5.5.x

> +    Reviewing this as a whole, I have questions regarding the following field:
> +      ScriptingVariabler#ScriptingVariableVisitor#scriptVars
> +    It is used to introduce behaviour like requested in BZ 48616, but ...
> +    The BZ 42390 fix (r804734) effectively eliminates it. It looks like
> +    it'd be better to remove it and care about redeclarations somewhere
> +    else.
> +    - to discuss on dev@
r804734 seemed like a neat fix to 42390 but your concerns at the time
about regressions were well founded, as the (in my view valid)
regression raised in 48616 shows.

Whether or not redeclaration of a variable is required depends both on
how the user structures their page and how Tomcat then converts that to
Java. This should be transparent to the user. As long as the page does
not contain anything in direct contravention of the Servlet, JSP or EL
specs, the page should work.

Tomcat should not be forcing the user to use unnecessary workarounds
purely as a result of how Tomcat converts the JSP to Java. The fact the
Tomcat uses a fragment helper in some cases and not others should not
alter how the user has to code their page.

JSPs do not define variable scope in the same way that Java does and JSP
authors should not be expected to understand how Tomcat converts their
JSP to Java in order to work out whether or not they need to use a
Tomcat specific workaround. Tomcat should be tracking variable
declarations and working out whether or not they need to be redeclared.

I believe the original bug 42390 is valid but the fix (r804734) failed
to take account of valid use cases, introducing the regression bug
48616. Both 42390 and 48616 need to be fixed.

Mark



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message