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/13 18:55:11 UTC
svn commit: r909860 - /tomcat/tc6.0.x/trunk/STATUS.txt
Author: kkolinko
Date: Sat Feb 13 17:55:11 2010
New Revision: 909860
URL: http://svn.apache.org/viewvc?rev=909860&view=rev
Log:
split EL fixes vote into individual patches, for clarity
veto the Enum EL patch backport
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=909860&r1=909859&r2=909860&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/STATUS.txt (original)
+++ tomcat/tc6.0.x/trunk/STATUS.txt Sat Feb 13 17:55:11 2010
@@ -107,59 +107,64 @@
* Fix various EL TCK failures
http://svn.apache.org/viewvc?view=rev&rev=899653 (signatures)
+ +1: markt, kkolinko
+ -1:
+
http://svn.apache.org/viewvc?view=rev&rev=899769 (CCE expected)
http://svn.apache.org/viewvc?view=rev&rev=899770 (CCE expected)
+ +1: markt, kkolinko
+ -1:
+ kkolinko: Maybe better name for that message, because it says about
+ arrays, yet name is rather generic
+
http://svn.apache.org/viewvc?view=rev&rev=899783 (ELException expected)
+ +1: markt, kkolinko
+ -1:
+
http://svn.apache.org/viewvc?view=rev&rev=899788 (PNFE expected)
+ +1: markt, kkolinko
+ -1:
+ kkolinko: I think o.a.jasper.el.ELResolverImpl#getType(ELContext,Object,Object)
+ should likewise throw a PropertyNotFoundException instead of returning null.
+ I have no test, though.
+
http://svn.apache.org/viewvc?view=rev&rev=899792 (ELException rather than IAE)
+ +1: markt, kkolinko
+ -1:
+
http://svn.apache.org/viewvc?view=rev&rev=899916 (ELException rather than IAE)
+ +1: markt, kkolinko
+ -1:
+
http://svn.apache.org/viewvc?view=rev&rev=899918 (Enum coercion test cases)
http://svn.apache.org/viewvc?view=rev&rev=899919 (Enum coercion bug)
+ +1: markt
+ -1: kkolinko:
+ As far as I am reading, there is no provision in the EL
+ spec, that enum -> enum conversion can be performed by using its
+ string value.
+
+ The current TC 6.0 behaviour here will be a ClassCastException, trying
+ to assign the value, and that should be an ELException instead.
+
+ In EL 2.1 and 2.2 specifications chapter 1.18.6 'Coerce A to an Enum Type T' says:
+ "If A is a String call Enum.valueOf(T.getClass(), A) and return the result."
+ Thus, our
+ result = Enum.valueOf(type, obj.toString());
+ should only be applicable if obj is a String.
+
+ In ELSupport#coerceToType(Object, Class), that implements 1.18.7, we
+ already throw an ELException if A is not a String.
+
http://svn.apache.org/viewvc?view=rev&rev=899935 (ELException expected)
- http://svn.apache.org/viewvc?view=rev&rev=899949 (ignore whitespace on comp)
- +1: markt
- +1: kkolinko:
- 899653: OK. We do not have @Deprecated annotations in those classes,
- so the patch is about adding @SuppressWarnings("dep-ann")
- 899769: With 899770 that backports the message string used here.
- 899770: OK
- (Maybe better name for that message, because it says about arrays,
- yet name is rather generic).
- 899783: OK
- 899788: OK
- (Likewise, o.a.jasper.el.ELResolverImpl#getType(ELContext,Object,Object)
- should probably throw a PropertyNotFoundException, instead of returning null.
- I have no proof, though.)
- 899792: OK
- 899916: OK
-
- 899918, 899919: OK, but there is probably an omission in the EL spec:
- I do not see why we do conversion Enum->Enum via toString() call.
-
- The EL spec chapter 1.18.6 'Coerce A to an Enum Type T' says
- "If A is a String call Enum.valueOf(T.getClass(), A) and return the result."
- It does not say what to do if A is not a String. (There is no
- explicit "Otherwise, error" statement below).
-
- In 1.18.7 (aka ELSupport#coerceToType(Object, Class)) we throw
- an error if A is not a String. Even if T has a PropertyEditor,
- we do not do editor.setAsText(obj.toString()), as the spec does
- not say to do so, but throw an exception.
-
- (In 1.18.7 the spec says "Otherwise, apply T's PropertyEditor",
- but PropertyEditor can be applied only is A is a String. Am I right?)
-
- Without 899919 patch we will throw a ClassCaseException when object type
- is a different type of enum, but other values are still converted
- via toString() call. The patch makes that behaviour consistent, even
- if I do not understand why it is allowed.
-
- 899935: OK
- 899949: OK,
- but why ValueExpressionImpl.equals() is implemented as comparing
- the hash codes? What will happen with false positives?
+ +1: markt, kkolinko
+ -1:
- -1:
+ http://svn.apache.org/viewvc?view=rev&rev=899949 (ignore whitespace on comp)
+ +1: markt, kkolinko
+ -1:
+ kkolinko: Why ValueExpressionImpl.equals() is implemented as comparing
+ the hash codes? What will happen with false positives?
* Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=48627
Regression in re-working of EL parsing
@@ -176,7 +181,7 @@
Provide option to stop server if there is an error during init
Port of Filip's patch from trunk
http://svn.apache.org/viewvc?view=revision&revision=752323
- +1: markt
+ +1: markt, kkolinko
-1:
* Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=48645
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r909860 - /tomcat/tc6.0.x/trunk/STATUS.txt
Posted by Mark Thomas <ma...@apache.org>.
On 13/02/2010 17:55, kkolinko@apache.org wrote:
> http://svn.apache.org/viewvc?view=rev&rev=899918 (Enum coercion test cases)
> http://svn.apache.org/viewvc?view=rev&rev=899919 (Enum coercion bug)
> + +1: markt
> + -1: kkolinko:
> + As far as I am reading, there is no provision in the EL
> + spec, that enum -> enum conversion can be performed by using its
> + string value.
Point taken although I'd be a lot happier if there was an otherwise
error at the end of that section. There is currently a big blackhole
there in the spec.
> + The current TC 6.0 behaviour here will be a ClassCastException, trying
> + to assign the value, and that should be an ELException instead.
There are still issue with the current behaviour. I'll fix the test so
converting via toString() is not expected and then fix the code accordingly.
I'll leave this proposal here for now until I see how much needs to change.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org