maxfortun commented on PR #506: URL: https://github.com/apache/tomcat/pull/506#issuecomment-1124214014
> Thank you for your contribution. I have a few comments: > > 1. The javadoc was copy/pasted and inaccurate: both classes claim to serve JSPs for errors, and neither of them do this. Changed the wording. > 2. Each valve is configured using an external properties file instead of within `server.xml` like valves are typically configured. The idea here is to be able to provide different resource bundles based on the locale of the requests. French errors may looks somewhat different from English ones. Do you believe It is a requirement to configure valves only using their attributes? > 3. The documentation (XML) disagrees with the implementation, including documenting attributes that don't have any effect and confusing them with entries in the file-based configuration and failing to document `nnn=` vs `http.nnn=` in any way. The documented attributes are not for the valve, but for the resource bundle. I believe that what the text states. How do you suggest I reword this to avoid confusion? > 4. Trace log statement(s) is/are not guarded by e.g. `log.isTraceEnabled` Added > 5. The URL parameters are not consistently sanitized using `URLEncoder.encode` Added > 6. There doesn't seem to be a reason to prohibit URL parameters from the "base URL" for each valve Added support for arbitrary url params. > For the "Proxy" error report valve: > > 1. No attempt is made to set the response's Content-Length or other appropriate headers coming from the proxied connection (I do see the Content-Type in there, which is good Added content-length > 2. This valve will fail if the request has already been using a Writer > 3. This valve will fail if `url.openStream` returns `null` Upon any failure valves return handling to ErrorReportValve. > 4. Performance: Please use `StringBuilder` instead of string-concatenation when possible Changed -- 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. To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
