New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to send a reason phase. #7

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
3 participants
@violetagg
Contributor

violetagg commented Mar 27, 2017

Add an option to send a reason phase. By default a reason phrase will not be sent.
This feature is marked as deprecated and will be removed in Tomcat 9.
This change is based on r1702765 and r1785641.

Add an option to send a reason phase. By default a reason phrase will…
… not be sent.

This feature is marked as deprecated and will be removed in Tomcat 9.
This change is based on r1702765 and r1785641.
@@ -553,6 +553,17 @@
<properties>
<property
name="org.apache.coyote. USE_CUSTOM_STATUS_MSG_IN_HEADER"><p>If this is

This comment has been minimized.

@martin-g

martin-g Mar 27, 2017

Contributor

It seems there is a space between coyote. and USE_CUSTOM_STATUS_MSG_IN_HEADER.
Stackoverflow Driven Development (i.e. copy/paste) will fail :-)

@martin-g

martin-g Mar 27, 2017

Contributor

It seems there is a space between coyote. and USE_CUSTOM_STATUS_MSG_IN_HEADER.
Stackoverflow Driven Development (i.e. copy/paste) will fail :-)

This comment has been minimized.

@violetagg

violetagg Mar 27, 2017

Contributor

The usage of space is intentional. With this space we control the "Property" column width. Look at the whole file.

@violetagg

violetagg Mar 27, 2017

Contributor

The usage of space is intentional. With this space we control the "Property" column width. Look at the whole file.

@@ -0,0 +1,19 @@
# Licensed to the Apache Software Foundation (ASF) under one or more

This comment has been minimized.

@martin-g

martin-g Mar 27, 2017

Contributor

Why this file is needed ?
Without it it is more clear that the English version will be used.

@martin-g

martin-g Mar 27, 2017

Contributor

Why this file is needed ?
Without it it is more clear that the English version will be used.

This comment has been minimized.

@violetagg

violetagg Mar 27, 2017

Contributor

I'll remove it

@violetagg

violetagg Mar 27, 2017

Contributor

I'll remove it

private String st_200 = null;
private String st_302 = null;
private String st_400 = null;
private String st_404 = null;

This comment has been minimized.

@martin-g

martin-g Mar 27, 2017

Contributor

Maybe also add st_500.
Internal errors are also common.

@martin-g

martin-g Mar 27, 2017

Contributor

Maybe also add st_500.
Internal errors are also common.

This comment has been minimized.

@violetagg

violetagg Mar 27, 2017

Contributor

I'll do it in a separate change so that I will be able to back port it.

@violetagg

violetagg Mar 27, 2017

Contributor

I'll do it in a separate change so that I will be able to back port it.

This comment has been minimized.

@violetagg

violetagg Mar 28, 2017

Contributor

Fixed with r1788905

@violetagg

violetagg Mar 28, 2017

Contributor

Fixed with r1788905

@violetagg

This comment has been minimized.

Show comment
Hide comment
@violetagg

violetagg Mar 27, 2017

Contributor

Fixed with r1788881

Contributor

violetagg commented Mar 27, 2017

Fixed with r1788881

@violetagg violetagg closed this Mar 27, 2017

* not be sent.
*/
@Deprecated
public boolean getSendReasonPhrase() {

This comment has been minimized.

@michael-o

michael-o Mar 28, 2017

Member

Is this commonly used throughout the Tomcat code? By convention, this has to be isSendReasonPhrase.

@michael-o

michael-o Mar 28, 2017

Member

Is this commonly used throughout the Tomcat code? By convention, this has to be isSendReasonPhrase.

This comment has been minimized.

@violetagg

violetagg Mar 28, 2017

Contributor

In the most cases "get" methods are used. However I can see also some "is" methods.

@violetagg

violetagg Mar 28, 2017

Contributor

In the most cases "get" methods are used. However I can see also some "is" methods.

This comment has been minimized.

@michael-o

michael-o Mar 28, 2017

Member

Seems like non consistent. Worth a cleanup on Tomcat 9 and some deprecations below.

@michael-o

michael-o Mar 28, 2017

Member

Seems like non consistent. Worth a cleanup on Tomcat 9 and some deprecations below.

This comment has been minimized.

@violetagg

violetagg Mar 28, 2017

Contributor

This change does not exist in Tomcat 9 at all.
In Tomcat 9 there is no an option to send a reason phrase. A reason phrase will not be sent at all.
If you are speaking for the rest of the properties please open an enhancement.

@violetagg

violetagg Mar 28, 2017

Contributor

This change does not exist in Tomcat 9 at all.
In Tomcat 9 there is no an option to send a reason phrase. A reason phrase will not be sent at all.
If you are speaking for the rest of the properties please open an enhancement.

This comment has been minimized.

@michael-o

michael-o Mar 28, 2017

Member

Yes, I was referring to the rest. I will file one.

@michael-o

michael-o Mar 28, 2017

Member

Yes, I was referring to the rest. I will file one.

This comment has been minimized.

@michael-o

michael-o Mar 29, 2017

Member

I have grepped through Tomcat 8.5 code with boolean get: 418 hits. A massive change. This should likely be fixed in Tomcat 9 only. I will file an issue to track this.

@michael-o

michael-o Mar 29, 2017

Member

I have grepped through Tomcat 8.5 code with boolean get: 418 hits. A massive change. This should likely be fixed in Tomcat 9 only. I will file an issue to track this.

*/
@Deprecated
public static final boolean USE_CUSTOM_STATUS_MSG_IN_HEADER =
Boolean.valueOf(System.getProperty(

This comment has been minimized.

@michael-o

michael-o Mar 28, 2017

Member

Why this huge overhead? Use Boolean#getBoolean.

@michael-o

michael-o Mar 28, 2017

Member

Why this huge overhead? Use Boolean#getBoolean.

This comment has been minimized.

@violetagg

violetagg Mar 28, 2017

Contributor

I'll make an additional change in order to use Boolean#getBoolean.
Thanks

@violetagg

violetagg Mar 28, 2017

Contributor

I'll make an additional change in order to use Boolean#getBoolean.
Thanks

This comment has been minimized.

@violetagg

violetagg Mar 28, 2017

Contributor

Fixed with r1789216

@violetagg

violetagg Mar 28, 2017

Contributor

Fixed with r1789216

* not be sent.
*/
@Deprecated
public static final boolean USE_CUSTOM_STATUS_MSG_IN_HEADER =

This comment has been minimized.

@michael-o

michael-o Mar 28, 2017

Member

This property name is misleading. It implies that custom reason phrases can be passed and sent. This isn't true. The reason phrases are fix, sent by Tomcat directly.

@michael-o

michael-o Mar 28, 2017

Member

This property name is misleading. It implies that custom reason phrases can be passed and sent. This isn't true. The reason phrases are fix, sent by Tomcat directly.

This comment has been minimized.

@michael-o

michael-o Mar 28, 2017

Member

Why is this property necessary at all if you can configure it at AbstractProtocol level?

@michael-o

michael-o Mar 28, 2017

Member

Why is this property necessary at all if you can configure it at AbstractProtocol level?

This comment has been minimized.

@violetagg

violetagg Mar 28, 2017

Contributor

Actually you can do that.
Look at

message = response.getMessage();

@violetagg

violetagg Mar 28, 2017

Contributor

Actually you can do that.
Look at

message = response.getMessage();

This comment has been minimized.

@michael-o

michael-o Mar 29, 2017

Member

So you are referring to use sendError(int, String) for the message, right? For status below 400, there is now way to pass a custom message then because sendError(int, String) will invoke org.apache.catalina.connector.Response.sendError(int, String) and internally call setError(). Is this really intended?

@michael-o

michael-o Mar 29, 2017

Member

So you are referring to use sendError(int, String) for the message, right? For status below 400, there is now way to pass a custom message then because sendError(int, String) will invoke org.apache.catalina.connector.Response.sendError(int, String) and internally call setError(). Is this really intended?

This comment has been minimized.

@violetagg

violetagg Apr 4, 2017

Contributor

This can be used by Valves as they have direct access to Tomcat's Request/Response objects and also there is a deprecated method setStatus(int, String) that we cannot prevent.

@violetagg

violetagg Apr 4, 2017

Contributor

This can be used by Valves as they have direct access to Tomcat's Request/Response objects and also there is a deprecated method setStatus(int, String) that we cannot prevent.

sc.414=URI de Requ\u00eate Trop Grande
sc.415=Type de Support Non Support\u00e9
sc.416=Etendue de Requ\u00eate Irr\u00e9alisable
sc.417=Echec d'Attente

This comment has been minimized.

@michael-o

michael-o Mar 28, 2017

Member

Attention, if you pass this to MessageFormat the apostrophe is lost. If so, use '' -- two apostrophes to escape it.

@michael-o

michael-o Mar 28, 2017

Member

Attention, if you pass this to MessageFormat the apostrophe is lost. If so, use '' -- two apostrophes to escape it.

This comment has been minimized.

@violetagg

violetagg Mar 28, 2017

Contributor

I can see it is not only in this properties file.
Would you mind to report this and describe this issue so that it gets fixed in all properties files?

@violetagg

violetagg Mar 28, 2017

Contributor

I can see it is not only in this properties file.
Would you mind to report this and describe this issue so that it gets fixed in all properties files?

This comment has been minimized.

@michael-o

michael-o Mar 29, 2017

Member
* @return <code>true</code> if the message is safe to use in an HTTP
* header else <code>false</code>
*/
public static boolean isSafeInHttpHeader(String msg) {

This comment has been minimized.

@michael-o
@michael-o

michael-o Mar 29, 2017

Member
<p>If not specified, the default value of <code>false</code> will be used.</p>
<p><strong>Note:</strong> This option is deprecated and will be removed
in Tomcat 9. The reason phrase will not be sent.</p>
</property>

This comment has been minimized.

@michael-o

michael-o Mar 29, 2017

Member

You should probably tell that custom messages are passed via sendError(int, String). Aren't they?

@michael-o

michael-o Mar 29, 2017

Member

You should probably tell that custom messages are passed via sendError(int, String). Aren't they?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment