You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@turbine.apache.org by pa...@apache.org on 2017/12/06 16:51:52 UTC
svn commit: r1817310 -
/turbine/core/trunk/src/java/org/apache/turbine/Turbine.java
Author: painter
Date: Wed Dec 6 16:51:52 2017
New Revision: 1817310
URL: http://svn.apache.org/viewvc?rev=1817310&view=rev
Log:
Set the configuration parameter to honor the value given by the user in TR.props
Modified:
turbine/core/trunk/src/java/org/apache/turbine/Turbine.java
Modified: turbine/core/trunk/src/java/org/apache/turbine/Turbine.java
URL: http://svn.apache.org/viewvc/turbine/core/trunk/src/java/org/apache/turbine/Turbine.java?rev=1817310&r1=1817309&r2=1817310&view=diff
==============================================================================
--- turbine/core/trunk/src/java/org/apache/turbine/Turbine.java (original)
+++ turbine/core/trunk/src/java/org/apache/turbine/Turbine.java Wed Dec 6 16:51:52 2017
@@ -374,11 +374,14 @@ public class Turbine
TurbineConstants.PARAMETER_ENCODING_KEY,
TurbineConstants.PARAMETER_ENCODING_DEFAULT);
+ // RunData needs the following encoding set to override the default charset
+ configuration.setProperty(TurbineConstants.LOCALE_DEFAULT_CHARSET_KEY, inputEncoding);
+
if (log.isDebugEnabled())
{
log.debug("Input Encoding has been set to " + inputEncoding);
}
-
+
getServiceManager().setConfiguration(configuration);
// Initialize the service manager. Services
CharacterEncoding, was: Re: svn commit: r1817310 -
/turbine/core/trunk/src/java/org/apache/turbine/Turbine.java
Posted by Thomas Vandahl <tv...@apache.org>.
Hi folks,
On 12.04.18 15:58, Georg Kallidis wrote:
> default, not any hard-coded default. The locale based mimetype charset
> check and set in this method could never be called IMO and may be better
> moved to another place. It may be (re)used only optionally e.g. by setting
> services.VelocityService.default.page to a class extending DefaultPage
> overriding the method doBuildBeforeAction (calling rundata.setCharSet, per
> request, executed in ExecutePageValve). As a result the charSet field
> value in rundata is never set, which might be a problem, when the content
> type does not match "text/".
I suggest to move the encoding/charset handling to a valve. That way,
you can switch it on and off or change it as required.
See my commit in r1836928. WDYT?
Bye, Thomas
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@turbine.apache.org
For additional commands, e-mail: dev-help@turbine.apache.org
Re: Re: svn commit: r1817310 - /turbine/core/trunk/src/java/org/apache/turbine/Turbine.java
Posted by Georg Kallidis <ge...@cedis.fu-berlin.de>.
Hi,
as this is an ongoing issue (and the last obstacle before upgrading to
Turbine 4.1) I just want to sum up before committing any solutions.
> The DefaultTurbineRunData.getDefaultCharSet()
> currently pulls from Turbine configuration constants, and without the
> current patch, this value never gets updated by the user defined
property.
What's intended?
- input.encoding is used for is setting req.setCharacterEncoding, if the
(tomcat-)container does not already set this.
- locale.default.charset is on the other side used (only) in rundata's
getDefaultCharSet method to define the charset in getContentType, if the
content type matches text and in getCharSet - called once when hit first
time. "Default" does in this context just mean a Turbine configured
default, not any hard-coded default. The locale based mimetype charset
check and set in this method could never be called IMO and may be better
moved to another place. It may be (re)used only optionally e.g. by setting
services.VelocityService.default.page to a class extending DefaultPage
overriding the method doBuildBeforeAction (calling rundata.setCharSet, per
request, executed in ExecutePageValve). As a result the charSet field
value in rundata is never set, which might be a problem, when the content
type does not match "text/".
As a result, the first step would be IMO, that we just remove the line in
Turbine.java (input.encoding overriding locale.default.charset) and check
(together), why in the aforementioned context "this value never gets
updated".
E.g. in https://wiki.apache.org/tomcat/FAQ/CharacterEncoding it is
mentioned, that
"In Tomcat 8 starting with 8.0.0 (8.0.0-RC3, to be specific), the default
value of URIEncoding attribute on the <Connector> element depends on
"strict servlet compliance" setting. The default value (strict compliance
is off) of URIEncoding is now UTF-8. If "strict servlet compliance" is
enabled, the default value is ISO-8859-1.
Best regards, Georg
Von: Jeffery Painter <je...@jivecast.com>
An: dev@turbine.apache.org
Datum: 11.12.2017 19:01
Betreff: Re: svn commit: r1817310 -
/turbine/core/trunk/src/java/org/apache/turbine/Turbine.java
Hi Thomas,
It sounds like you recommending then that we get rid of
input.encoding=<user defined> completely from the TR.props config. Is
that correct? It has been in there since 2.3.x days
I searched through the code base and the method call to
Turbine.getDeafultInputEncoding() is never referenced anywhere currently
except in one test case. The DefaultTurbineRunData.getDefaultCharSet()
currently pulls from Turbine configuration constants, and without the
current patch, this value never gets updated by the user defined property.
It would make more sense to me that it should be handled as you mention
below, but I am not sure where to start to make all the fixes required.
Similar issues exist around updating the default locale.
If we were to get rid of it completely, do you agree UTF-8 should be the
new default or are there reasons it should be something else? How do we
handle modifying the default locale to also conform?
I could not move to Turbine 4.0 without UTF-8 support, and this fix
allows for it. Seems a bit clunky to have to set it manually the way I
have done though. Open to your thoughts/suggestions on a better way
forward :-)
Thanks,
Jeff
On 12/08/2017 10:52 AM, Thomas Vandahl wrote:
> On 06.12.17 17:51, painter@apache.org wrote:
>> Author: painter
>> Date: Wed Dec 6 16:51:52 2017
>> New Revision: 1817310
>>
>> URL: http://svn.apache.org/viewvc?rev=1817310&view=rev
>> Log:
>> Set the configuration parameter to honor the value given by the user in
TR.props
> -1
>
> This change will only cause confusion.
>
> We should remove the configuration setting then completely (as you will
> be overwriting it in any case) and rely on
> Turbine.getDefaultInputEncoding() only (making this static).
>
> Bye, Thomas
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@turbine.apache.org
> For additional commands, e-mail: dev-help@turbine.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@turbine.apache.org
For additional commands, e-mail: dev-help@turbine.apache.org
Re: svn commit: r1817310 -
/turbine/core/trunk/src/java/org/apache/turbine/Turbine.java
Posted by Jeffery Painter <je...@jivecast.com>.
Hi Thomas,
I am with you. I think easier to let Tomcat (or similar) handle and not
confuse things by adding additional parameters in Turbine that are
better handled elsewhere. I don't feel comfortable extracting all the
parts from Turbine core without breaking things at the moment :-)
Maybe we can add this to things to do for the next release of Turbine.
--
Jeffery
On 12/11/2017 04:11 PM, Thomas Vandahl wrote:
> Hi Jeffery,
>
> On 11.12.17 18:55, Jeffery Painter wrote:
>> It sounds like you recommending then that we get rid of
>> input.encoding=<user defined> completely from the TR.props config. Is
>> that correct? It has been in there since 2.3.x days
> I'd rather get rid of "locale.default.charset" and friends and replace
> them with input.encoding or introduce a default.encoding setting to rule
> them all.
>
>> I searched through the code base and the method call to
>> Turbine.getDeafultInputEncoding() is never referenced anywhere currently
>> except in one test case. The DefaultTurbineRunData.getDefaultCharSet()
>> currently pulls from Turbine configuration constants, and without the
>> current patch, this value never gets updated by the user defined property.
> Normally, the input encoding should be read from the servlet request. I
> don't see any real reason to respond with a charset different from what
> is requested.
>
>> It would make more sense to me that it should be handled as you mention
>> below, but I am not sure where to start to make all the fixes required.
>> Similar issues exist around updating the default locale.
>>
>> If we were to get rid of it completely, do you agree UTF-8 should be the
>> new default or are there reasons it should be something else? How do we
>> handle modifying the default locale to also conform?
> IIRC, the default encoding of HTTP requests according to RFC2616 is
> ISO-8859-1. So if we agree with the RFC, we must not change the default.
>
>> I could not move to Turbine 4.0 without UTF-8 support, and this fix
>> allows for it. Seems a bit clunky to have to set it manually the way I
>> have done though. Open to your thoughts/suggestions on a better way
>> forward :-)
> I strongly believe that the encoding setting is the responsibility of
> the servlet container, not the application, let alone the framework. All
> settings in the Turbine configuration are only defaults for the case
> when the container does not provide an encoding (and always have been).
>
> The two settings you tried to pull straight are probably redundant. I
> remember having trouble when I set them to different values (I need to
> double check, though). So my suggestion is to cut it down to just one
> default encoding for good.
>
> For methods to set the container encoding, see for example
> https://wiki.apache.org/tomcat/FAQ/CharacterEncoding
>
> Does that make sense to you?
>
> Bye, Thomas
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@turbine.apache.org
> For additional commands, e-mail: dev-help@turbine.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@turbine.apache.org
For additional commands, e-mail: dev-help@turbine.apache.org
Re: svn commit: r1817310 -
/turbine/core/trunk/src/java/org/apache/turbine/Turbine.java
Posted by Thomas Vandahl <tv...@apache.org>.
Hi Jeffery,
On 11.12.17 18:55, Jeffery Painter wrote:
> It sounds like you recommending then that we get rid of
> input.encoding=<user defined> completely from the TR.props config. Is
> that correct? It has been in there since 2.3.x days
I'd rather get rid of "locale.default.charset" and friends and replace
them with input.encoding or introduce a default.encoding setting to rule
them all.
> I searched through the code base and the method call to
> Turbine.getDeafultInputEncoding() is never referenced anywhere currently
> except in one test case. The DefaultTurbineRunData.getDefaultCharSet()
> currently pulls from Turbine configuration constants, and without the
> current patch, this value never gets updated by the user defined property.
Normally, the input encoding should be read from the servlet request. I
don't see any real reason to respond with a charset different from what
is requested.
> It would make more sense to me that it should be handled as you mention
> below, but I am not sure where to start to make all the fixes required.
> Similar issues exist around updating the default locale.
>
> If we were to get rid of it completely, do you agree UTF-8 should be the
> new default or are there reasons it should be something else? How do we
> handle modifying the default locale to also conform?
IIRC, the default encoding of HTTP requests according to RFC2616 is
ISO-8859-1. So if we agree with the RFC, we must not change the default.
> I could not move to Turbine 4.0 without UTF-8 support, and this fix
> allows for it. Seems a bit clunky to have to set it manually the way I
> have done though. Open to your thoughts/suggestions on a better way
> forward :-)
I strongly believe that the encoding setting is the responsibility of
the servlet container, not the application, let alone the framework. All
settings in the Turbine configuration are only defaults for the case
when the container does not provide an encoding (and always have been).
The two settings you tried to pull straight are probably redundant. I
remember having trouble when I set them to different values (I need to
double check, though). So my suggestion is to cut it down to just one
default encoding for good.
For methods to set the container encoding, see for example
https://wiki.apache.org/tomcat/FAQ/CharacterEncoding
Does that make sense to you?
Bye, Thomas
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@turbine.apache.org
For additional commands, e-mail: dev-help@turbine.apache.org
Re: svn commit: r1817310 -
/turbine/core/trunk/src/java/org/apache/turbine/Turbine.java
Posted by Jeffery Painter <je...@jivecast.com>.
Hi Thomas,
It sounds like you recommending then that we get rid of
input.encoding=<user defined> completely from the TR.props config. Is
that correct? It has been in there since 2.3.x days
I searched through the code base and the method call to
Turbine.getDeafultInputEncoding() is never referenced anywhere currently
except in one test case. The DefaultTurbineRunData.getDefaultCharSet()
currently pulls from Turbine configuration constants, and without the
current patch, this value never gets updated by the user defined property.
It would make more sense to me that it should be handled as you mention
below, but I am not sure where to start to make all the fixes required.
Similar issues exist around updating the default locale.
If we were to get rid of it completely, do you agree UTF-8 should be the
new default or are there reasons it should be something else? How do we
handle modifying the default locale to also conform?
I could not move to Turbine 4.0 without UTF-8 support, and this fix
allows for it. Seems a bit clunky to have to set it manually the way I
have done though. Open to your thoughts/suggestions on a better way
forward :-)
Thanks,
Jeff
On 12/08/2017 10:52 AM, Thomas Vandahl wrote:
> On 06.12.17 17:51, painter@apache.org wrote:
>> Author: painter
>> Date: Wed Dec 6 16:51:52 2017
>> New Revision: 1817310
>>
>> URL: http://svn.apache.org/viewvc?rev=1817310&view=rev
>> Log:
>> Set the configuration parameter to honor the value given by the user in TR.props
> -1
>
> This change will only cause confusion.
>
> We should remove the configuration setting then completely (as you will
> be overwriting it in any case) and rely on
> Turbine.getDefaultInputEncoding() only (making this static).
>
> Bye, Thomas
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@turbine.apache.org
> For additional commands, e-mail: dev-help@turbine.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@turbine.apache.org
For additional commands, e-mail: dev-help@turbine.apache.org
Re: svn commit: r1817310 -
/turbine/core/trunk/src/java/org/apache/turbine/Turbine.java
Posted by Thomas Vandahl <tv...@apache.org>.
On 06.12.17 17:51, painter@apache.org wrote:
> Author: painter
> Date: Wed Dec 6 16:51:52 2017
> New Revision: 1817310
>
> URL: http://svn.apache.org/viewvc?rev=1817310&view=rev
> Log:
> Set the configuration parameter to honor the value given by the user in TR.props
-1
This change will only cause confusion.
We should remove the configuration setting then completely (as you will
be overwriting it in any case) and rely on
Turbine.getDefaultInputEncoding() only (making this static).
Bye, Thomas
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@turbine.apache.org
For additional commands, e-mail: dev-help@turbine.apache.org