codeconsole commented on code in PR #16722:
URL: https://github.com/apache/grails-core/pull/16722#discussion_r4349477046
##########
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))
Review Comment:
Reproduced at 6e208ff3b8. With the descriptor hidden (`new URLClassLoader([]
as URL[], null)`) and `basename: [config/i18n/custom, other]` in a
`NavigableMapPropertySource`, `environment.getProperty(BASENAME_PROPERTY,
'messages')` returns `messages` after the processor runs; the comma-separated
form returns `config/i18n/custom,other`. Boot 4.1.1's `ResourceBundleCondition`
still reads `getProperty("spring.messages.basename", "messages")`, so for the
list form it looks only for `messages.properties`.
Composing with an empty discovered list when no descriptor is found would
cover it, since `compose(environment, List.of())` is just the declared names.
Nothing is published when nothing is declared, so 'Grails defaults apply even
to an application with no descriptor at all' still holds, and a feature with a
declared list can sit next to it.
##########
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:
Probed at 6e208ff3b8: a map-shaped value (`spring.messages.basename: {foo:
bar}`) binds to an empty list rather than throwing, so the processor falls back
to the discovered names. Anything that does fail to bind here would fail the
same way a moment later, when Boot binds `MessageSourceProperties.basename` (a
`List<String>` in 4.1.1) with the same `Binder`, so a catch would only defer
the error.
##########
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))
+
+ when:
+ new I18nEnvironmentPostProcessor(bootstrapContext)
+ .postProcessEnvironment(environment,
applicationUsing(getClass().classLoader))
+
+ then: 'the declared order is preserved before the discovered
application bundle'
+
environment.getProperty(I18nEnvironmentPostProcessor.BASENAME_PROPERTY) ==
+ 'config/i18n/custom,config/i18n/other,messages'
+
+ where:
+ format | declared
+ 'list' | [' config/i18n/custom ', '', 'config/i18n/other',
'config/i18n/custom']
+ 'comma-separated' | ' config/i18n/custom , ,
config/i18n/other,config/i18n/custom '
+ }
Review Comment:
Non-blocking. The `Binder` read also fixes a form the old `getProperty`
never read: indexed keys in a plain property source, which is what
`application.properties`, `--spring.messages.basename[0]=…` and
`SPRING_MESSAGES_BASENAME_0_` produce. Both rows here go through
`NavigableMapPropertySource`, which is deprecated along with `NavigableMap`, so
it is worth pinning that form directly. Checked: this passes at 6e208ff3b8 and
fails on 8.0.x.
```suggestion
'comma-separated' | ' config/i18n/custom , ,
config/i18n/other,config/i18n/custom '
}
void 'declared base names in indexed property form stay ahead of
discovered bundles'() {
given: 'the form application.properties, command-line arguments and
SPRING_MESSAGES_BASENAME_0_ produce'
MockEnvironment environment = new MockEnvironment()
environment.propertySources.addLast(new
MapPropertySource('application.properties', [
'spring.messages.basename[0]': 'config/i18n/custom',
'spring.messages.basename[1]': 'config/i18n/other'
]))
when:
new I18nEnvironmentPostProcessor(bootstrapContext)
.postProcessEnvironment(environment,
applicationUsing(getClass().classLoader))
then: 'Environment.getProperty never saw these, so the old reader
dropped them too'
environment.getProperty(I18nEnvironmentPostProcessor.BASENAME_PROPERTY) ==
'config/i18n/custom,config/i18n/other,messages'
}
```
--
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]
