tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Konstantin Kolinko <knst.koli...@gmail.com>
Subject Re: svn commit: r783570 - /tomcat/current/tc4.1.x/STATUS.txt
Date Fri, 12 Jun 2009 16:55:32 GMT
2009/6/12 Mark Thomas <markt@apache.org>:
> Konstantin Kolinko wrote:
>> Thank you for detailed explanation.
>>
>> My analysis is the following:
>> hres.setLocale(locale);
>> call ->  o.a.c.Response.setLocale() -> o.a.c.connector.ResponseBase.setLocale()
>>
>> In o.a.c.connector.ResponseBase.setLocale() it calls
>> CharsetMapper.getCharset(locale)
>> and updates the contentType header.
>>
>> The problem is with those locales for which CharsetMapper.getCharset(locale)
>> returns null.  There is an error in ResponseBase.setLocale() that it will set
>>  contentType = contentType + ";charset=null"
>> in those cases. How about fixing that?
>>
>> My understanding is that it will solve the issue, and won't change the
>> error page encoding for existing applications.
>>
>> We cannot fix CharsetMapper, because it can be overwritten, but we can
>> fix those places where it is called.
>>
>> Here is the patch:
>> http://people.apache.org/~kkolinko/patches/2009-06-12_tc41_CharsetMapper.patch
>
> Yep, that would also fix the issue. However, that patch changes the
> behaviour of setLocale(). Whilst it shouldn't cause a regression there
> is a risk that it might for some applications.
>
> The 'right' / 'proper' fix for TC4 would be to implement
> STRICT_SERVLET_COMPLIANCE and in particular, the change to getWriter().
> Again, there is the risk of regression with this approach.
>
> TC6 and TC5 already use UTF-8 for Tomcat's default error page. The
> proposed TC4 page brings TC4 into line with TC5 and TC6.
>
> My personal preference is for the small as possible 'band-aid' approach
> to minimise the regression risk.
>

I do not like that your patch changes behavior where it was not
broken previously.

Theoretically, there might be someone who relies on the encoding
of those error pages, but, well, those pages are for humans,
and we are free to change them for readability or for any other purpose.

OK, I give my +1.


Anyway, just as a thought,
here is another version of setLocale patch,
http://people.apache.org/~kkolinko/patches/2009-06-12_tc41_CharsetMapper_v2.patch

I am not very interested in it, though.

Best regards,
Konstantin Kolinko

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


Mime
View raw message