Copilot commented on code in PR #16722:
URL: https://github.com/apache/grails-core/pull/16722#discussion_r4343855969
##########
grails-i18n/src/main/groovy/org/grails/plugins/i18n/I18nEnvironmentPostProcessor.java:
##########
@@ -147,12 +149,10 @@ private List<String> compose(ConfigurableEnvironment
environment, List<String> d
}
private List<String> declaredBasenames(ConfigurableEnvironment
environment) {
- String declared = environment.getProperty(BASENAME_PROPERTY);
- if (declared == null || declared.isBlank()) {
- return List.of();
- }
+ List<String> declared =
Binder.get(environment).bind(BASENAME_PROPERTY, Bindable.listOf(String.class))
+ .orElseGet(List::of);
List<String> basenames = new ArrayList<>();
- for (String basename : declared.split(",")) {
+ for (String basename : declared) {
Review Comment:
`Binder.bind(...)` can throw a `BindException` when the configured value is
present but not convertible to `List<String>` (e.g., an accidental map/object).
Previously, `getProperty` would typically yield `null`/string and the code
would continue without failing early. Consider catching bind failures here and
either (a) fall back to an empty list to preserve the prior lenient behavior,
or (b) rethrow with a targeted error message explaining the expected formats
for `spring.messages.basename` (string, comma-delimited string, or list). This
avoids unexpected startup failures with hard-to-diagnose binding errors.
##########
grails-i18n/src/test/groovy/org/grails/plugins/i18n/I18nEnvironmentPostProcessorSpec.groovy:
##########
@@ -118,6 +120,27 @@ locales=fr
environment.getProperty(I18nEnvironmentPostProcessor.BASENAME_PROPERTY) ==
'config/i18n/custom,messages'
}
+ void 'declared base names in #format form stay ahead of discovered
bundles'() {
+ given: 'Grails configuration exposes non-empty lists through indexed
properties'
+ NavigableMap config = new NavigableMap()
+ config.merge([spring: [messages: [basename: declared]]], false)
+ MockEnvironment environment = new MockEnvironment()
+ environment.propertySources.addLast(new
NavigableMapPropertySource('application', config))
Review Comment:
Using `addLast` gives this property source the lowest precedence (behind
`mockProperties` and any other sources added earlier). To make the test more
robust and clearly reflect “application” configuration precedence, prefer
adding it with highest precedence (e.g., `addFirst`, or inserting before
existing sources) so the bound value can’t be shadowed by other test setup.
--
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]
