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: r1140383 - in /tomcat: tc5.5.x/trunk/STATUS.txt tc6.0.x/trunk/STATUS.txt
Date Tue, 28 Jun 2011 10:45:09 GMT
On 28/06/2011 11:38, Konstantin Kolinko wrote:
> 2011/6/28 Mark Thomas <markt@apache.org>:
>> On 28/06/2011 01:08, kkolinko@apache.org wrote:
>>> Modified: tomcat/tc6.0.x/trunk/STATUS.txt
>>> URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=1140383&r1=1140382&r2=1140383&view=diff
>>>
>>>    http://svn.apache.org/viewvc?rev=1138950&view=rev
>>>    http://svn.apache.org/viewvc?rev=1138953&view=rev
>>>    +1: markt, schultz
>>> -  -1:
>>> +  -1: kkolinko: 1) It would be OK if it were mangling only reserved words
>>> +    (where the tag wouldn't compile previously, so there were no regressions),
>>> +    but JspUtils.makeJavaIdentifier() mangles '_' character, which will break
>>> +    code that was previously working. E.g. <%=hello_world%>
>>> +    2) Maybe it would be better to prefix the names with [_]jsp_ and use some
>>> +    numbered suffix, to never collide with names that people may use in their
>>> +    code.
>>
>> I don't see anything in the JSP specification that says tag file
>> attributes must be exposed as Java variables with the same name. Given
>> that a tag file attribute can have any name valid in XML, that
>> requirement would be impossible to meet since may of those names would
>> be invalid as Java identifiers.
>>
>> There is a requirement to expose attributes (with the same name) as page
>> scoped variables. If I switch the test case to use ${hello_world} rather
>> than <%=hello_world%> it works. However, that causes it's own set of
>> problems when a Java keyword is used as an attribute name due to the
>> restrictions of the EL specification.
>>
>> AFAICT, the only solution that is guaranteed (by the specification) to
>> work for any attribute name in a tag file is:
>> ${pageScope['attributename']}
>> Unless I have missed something in the specification (always a
>> possibility) that anything else works is a fortunate side-effect of the
>> implementation.
>>
> 
> Interesting. Though I used this feature a lot, since TC 5.5.
> 
> Eclipse IDE used to hilite these usages in red several years ago, but
> nowadays it has support for such usage.
> 
>> On this basis, I think the new behaviour is compliant with the
>> specification.
> 
> There is one more problem: you cannot introduce new variables that
> will be seen by user's code, unless those are prefixed with reserved
> prefix of "jsp_" ( "_jsp_") per JSP.9.1.2. That is because someone can
> declare a local variable with the same name.

JSP.9.1.2 certainly implies that but enforcing that would break the
<%=attributeName%> usage. I'm happy not worrying about that for now.

>> That said, I take the point regarding regressions. I'm sure lots of
>> folks will have used <%=attributeName%> or ${attributeName} as a
>> shortcut although I don't currently see anything to support that usage
>> in the JSP specification.

I was thinking along these lines:

Index: java/org/apache/jasper/compiler/JspUtil.java
===================================================================
--- java/org/apache/jasper/compiler/JspUtil.java	(revision 1140509)
+++ java/org/apache/jasper/compiler/JspUtil.java	(working copy)
@@ -810,10 +810,8 @@
         }
         for (int i = 0; i < identifier.length(); i++) {
             char ch = identifier.charAt(i);
-            if (Character.isJavaIdentifierPart(ch) && ch != '_') {
+            if (Character.isJavaIdentifierPart(ch)) {
                 modifiedIdentifier.append(ch);
-            } else if (ch == '.') {
-                modifiedIdentifier.append('_');
             } else {
                 modifiedIdentifier.append(mangleChar(ch));
             }


Essentially, no longer treat '.' as a special case. That removes the
needs to escape _ which should prevent any regressions. However, I do
wonder why '.' was treated as a special case in the first place. Time to
do some testing...

Mark



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


Mime
View raw message