You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by kk...@apache.org on 2010/02/14 07:47:40 UTC
svn commit: r909979 - /tomcat/tc6.0.x/trunk/STATUS.txt
Author: kkolinko
Date: Sun Feb 14 06:47:40 2010
New Revision: 909979
URL: http://svn.apache.org/viewvc?rev=909979&view=rev
Log:
veto
Modified:
tomcat/tc6.0.x/trunk/STATUS.txt
Modified: tomcat/tc6.0.x/trunk/STATUS.txt
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=909979&r1=909978&r2=909979&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/STATUS.txt (original)
+++ tomcat/tc6.0.x/trunk/STATUS.txt Sun Feb 14 06:47:40 2010
@@ -185,7 +185,33 @@
Test cases for both bugs and the JSP TCK pass with this patch applied.
http://people.apache.org/~markt/patches/2010-02-02-bug42390.patch
+1: markt
- -1:
+ -1: kkolinko:
+ 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.
+
+ 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.
+
+ 3. I think that BZ 48616 cannot/should not be fixed - see Comment 15 to
+ the issue.
+
+ 4. Should we fix BZ 42390 in TC 5.5, and bring on BZ 48616 there, if it
+ was historically working? I have doubts.
+
+ 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@
Additional patches (trivial):
http://svn.apache.org/viewvc?rev=905643&view=rev (misprint)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r909979 - /tomcat/tc6.0.x/trunk/STATUS.txt
Posted by Konstantin Kolinko <kn...@gmail.com>.
2010/2/15 Mark Thomas <ma...@apache.org>:
> On 14/02/2010 06:47, kkolinko@apache.org wrote:
>> + 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.
>
A JspFragment has its own scope of local variables. Thus, effectively
it should start with an empty map in
ScriptingVariableVisitor#scriptVars. The usesFragmentHelper method
performs as if ScriptingVariableVisitor#scriptVars were empty, but
only for that very tag that caused that JspFragment to appear. Once we
try to nest a classic tag in it, it fails.
In r910395.I added a test that demonstrates that. The alternative patch,
https://issues.apache.org/bugzilla/show_bug.cgi?id=48616#c20
does work here.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r909979 - /tomcat/tc6.0.x/trunk/STATUS.txt
Posted by Mark Thomas <ma...@apache.org>.
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