matrei commented on PR #16759: URL: https://github.com/apache/grails-core/pull/16759#issuecomment-6419783494
Following up on the three points above: I tested them on 7.0.17 (asset-pipeline 5.0.35). I ran a production WAR four ways: with the stock filter order and with the encoding filter at `HIGHEST_PRECEDENCE` (redefining the `characterEncodingFilter` bean, which is all the `GrailsFilters` change does), each with `forceEncoding` off and on. All three hold, but I understated the asset change: it doesn't need `forceEncoding`. - **Ordering:** probe filters at `HIGHEST_PRECEDENCE + 1`, `GrailsFilters.FIRST`, -191 and -181 ran before the encoding filter on stock and after it with this PR. A `FilterRegistrationBean` at exactly `HIGHEST_PRECEDENCE` still runs first. - **Assets:** with this PR, every asset response gets `;charset=UTF-8` (`image/x-icon;charset=UTF-8`, `image/svg+xml;charset=UTF-8`, `text/css;charset=UTF-8`, `text/javascript;charset=UTF-8`), also with `forceEncoding` off, which is the default. asset-pipeline's filter [reads the request's character encoding](https://github.com/wondrify/asset-pipeline/blob/77aac07346ef965cb8f86fddf0b6e2d7711ceb78/asset-pipeline-grails/src/main/groovy/asset/pipeline/AssetPipelineFilter.groovy#L49) and [sets it on the response](https://github.com/wondrify/asset-pipeline/blob/77aac07346ef965cb8f86fddf0b6e2d7711ceb78/asset-pipeline-grails/src/main/groovy/asset/pipeline/AssetPipelineFilter.groovy#L95). On stock it runs at -190, before the encoding filter, so the request encoding is still null and no charset is added. This affects every app that uses asset-pipeline, not just the ones with `forceEncoding: true`. - **Responses written by early filters:** with `forceEncoding: true`, a filter at `HIGHEST_PRECEDENCE + 1` that writes `text/plain` without a charset now sends UTF-8 instead of ISO-8859-1. With it off, nothing changes. - **Filters that set their own request encoding:** with `forceEncoding: true`, a filter that sets `windows-1252` used to be overridden by the forced UTF-8. With this PR, its `windows-1252` wins. With it off, nothing changes. - **`getReader()`:** a filter at `HIGHEST_PRECEDENCE + 1` reading a JSON body sent without a charset got `®ñ` on stock and `®ñ` with this PR. Browsers ignore a charset on images, but on CSS and JS it overrides their own detection. That's one more reason I'd keep the global move out of 7.0.x. Either way, the upgrade note should mention it, since it currently only covers `forceEncoding: true`. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
