You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2020/06/30 19:12:43 UTC
[GitHub] [tomcat] Holomark opened a new pull request #310: Remove extraneous spaces around the colons, which are breaking the co…
Holomark opened a new pull request #310:
URL: https://github.com/apache/tomcat/pull/310
This specific string is used to produce the context list in the text-based manager (`/manager/text/list`) and the spaces are breaking the tools that rely on it.
[Usage in the ManagerServlet](https://github.com/Holomark/tomcat/blob/21d2000c99134b7ec54424ea210cf9b2fd6aae38/java/org/apache/catalina/manager/ManagerServlet.java#L1079)
For instance, the cargo maven plugin (https://codehaus-cargo.github.io/cargo/Maven2+plugin.html), when trying to undeploy a project attempts first to find it in the list. If Tomcat's host system is configured in French, cargo will receive an incorrect input and infer that it is not currently deployed in Tomcat, even if it is.
That specific bug is how I became aware of the problem.
The commit that introduced those specific spaces invoked the french grammar; that is correct but this line is not a piece of natural language and those rules do not apply.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o edited a comment on pull request #310: Remove extraneous spaces around the colons of the output of the contexts list in the text-based manager
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on pull request #310:
URL: https://github.com/apache/tomcat/pull/310#issuecomment-652065030
It is a logical error to have an automated interface to be to stable across locales, infact, it should be locale-agnostic. The issue you have observed due to the fact that French spelling mandates a space before a colon. We likely need to review all other automated output from the text interface to make it properly consumable by machine clients.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on pull request #310: Remove extraneous spaces around the colons of the output of the contexts list in the text-based manager
Posted by GitBox <gi...@apache.org>.
rmaucher commented on pull request #310:
URL: https://github.com/apache/tomcat/pull/310#issuecomment-652262531
hostManagerServlet.listitem has the same issue. I agree there doesn't seem to be a purpose to using a string here since there's nothing to translate, so the best solution is to remove it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] Holomark commented on pull request #310: Remove extraneous spaces around the colons of the output of the contexts list in the text-based manager
Posted by GitBox <gi...@apache.org>.
Holomark commented on pull request #310:
URL: https://github.com/apache/tomcat/pull/310#issuecomment-652226783
I understand your comment, but I really believe this principle doesn't apply here. I may have not made my example clear, I am not trying to bend Tomcat to the whims of a plugin.
Here is a sample output from http://localhost:8080/manager/text/list on my machine, with a typical Tomcat installation:
```
OK - Listed applications for virtual host [localhost]
/:running:0:ROOT
/webstart:running:0:webstart
/manager:running:0:manager
/docs:running:0:docs
```
Here is the same output, in the context of a French configured system
```
OK - Applications listées pour l''hôte virtuel (virtual host) [localhost]
/ : running : 0 : ROOT
/webstart : running : 0 : webstart
/manager : running : 0 : manager
/docs : running : 0 : docs
```
As you can expect, an automated script splits the line on the colons. I believe this is a fair assumption.
This is not about translation, this is breaking the contract.
Actually I'd say the initial mistake was to put this line with the localized strings.
I was tempted to propose the following correction:
Those lines [from the ManagerServlet](https://github.com/Holomark/tomcat/blob/21d2000c99134b7ec54424ea210cf9b2fd6aae38/java/org/apache/catalina/manager/ManagerServlet.java#L1079)
```java
if (context.getState().isAvailable()) {
writer.println(smClient.getString("managerServlet.listitem",
displayPath,
"running",
"" + context.getManager().findSessions().length,
context.getDocBase()));
} else {
writer.println(smClient.getString("managerServlet.listitem",
displayPath,
"stopped",
"0",
context.getDocBase()));
}
```
... would be tweaked to:
```java
if (context.getState().isAvailable()) {
writer.println(String.join(":",
displayPath,
"running",
"" + context.getManager().findSessions().length,
context.getDocBase()));
} else {
writer.println(String.join(":",
displayPath,
"stopped",
"0",
context.getDocBase()));
```
... and the `managerServlet.listitem` line in LocalStrings_xx.properties removed (It is not used elsewhere).
I stepped back and proposed only a correction that seemed more innocuous to `LocalStrings_fr.properties`, as I have never contributed to this project, and barely in any other open source project.
Am I wrong ? I may be missing something.
Thank you for your time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #310: Remove extraneous spaces around the colons of the output of the contexts list in the text-based manager
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #310:
URL: https://github.com/apache/tomcat/pull/310#issuecomment-652065030
It is a logical error to have an automated interface to be to stable across locales, infact, it should be locale-agnostic. The issue you have observed due to the fact that French spelling mandates a space before a colon.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] Holomark edited a comment on pull request #310: Remove extraneous spaces around the colons of the output of the contexts list in the text-based manager
Posted by GitBox <gi...@apache.org>.
Holomark edited a comment on pull request #310:
URL: https://github.com/apache/tomcat/pull/310#issuecomment-652226783
I understand your comment, but I really believe this principle doesn't apply here. I may have not made my example clear, I am not trying to bend Tomcat to the whims of a plugin.
Here is a sample output from http://localhost:8080/manager/text/list on my machine, with a typical Tomcat installation:
```
OK - Listed applications for virtual host [localhost]
/:running:0:ROOT
/webstart:running:0:webstart
/manager:running:0:manager
/docs:running:0:docs
```
Here is the same output, in the context of a French configured system
```
OK - Applications listées pour l''hôte virtuel (virtual host) [localhost]
/ : running : 0 : ROOT
/webstart : running : 0 : webstart
/manager : running : 0 : manager
/docs : running : 0 : docs
```
As you can expect, an automated script splits the line on the colons. I believe this is a fair assumption, as far as I understand the text-based manager _is_ meant to be script-friendly.
This is not about translation, this is breaking the contract.
Actually I'd say the initial mistake was to put this line with the localized strings.
I was tempted to propose the following correction:
Those lines [from the ManagerServlet](https://github.com/Holomark/tomcat/blob/21d2000c99134b7ec54424ea210cf9b2fd6aae38/java/org/apache/catalina/manager/ManagerServlet.java#L1079)
```java
if (context.getState().isAvailable()) {
writer.println(smClient.getString("managerServlet.listitem",
displayPath,
"running",
"" + context.getManager().findSessions().length,
context.getDocBase()));
} else {
writer.println(smClient.getString("managerServlet.listitem",
displayPath,
"stopped",
"0",
context.getDocBase()));
}
```
... would be tweaked to:
```java
if (context.getState().isAvailable()) {
writer.println(String.join(":",
displayPath,
"running",
"" + context.getManager().findSessions().length,
context.getDocBase()));
} else {
writer.println(String.join(":",
displayPath,
"stopped",
"0",
context.getDocBase()));
```
... and the `managerServlet.listitem` line in LocalStrings_xx.properties removed (It is not used elsewhere).
I stepped back and proposed only a correction that seemed more innocuous to `LocalStrings_fr.properties`, as I have never contributed to this project, and barely in any other open source project.
Am I wrong ? I may be missing something.
Thank you for your time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf closed pull request #310: Remove extraneous spaces around the colons of the output of the contexts list in the text-based manager
Posted by GitBox <gi...@apache.org>.
markt-asf closed pull request #310:
URL: https://github.com/apache/tomcat/pull/310
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on pull request #310: Remove extraneous spaces around the colons of the output of the contexts list in the text-based manager
Posted by GitBox <gi...@apache.org>.
markt-asf commented on pull request #310:
URL: https://github.com/apache/tomcat/pull/310#issuecomment-654739480
Thanks for the PR. I've applied it to 10.0.x and I'll back-port it shortly.
I merged this manually so I could squash it into two commits and fix the indenting.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org