codeconsole commented on PR #16102:
URL: https://github.com/apache/grails-core/pull/16102#issuecomment-5346257007
Thanks — I went through all five. My read is that none of them should block
the merge, and I'd rather explain why than change things speculatively. Happy
to be pushed back on any of these.
**1. Cache duration 5s → 1s — I'd keep 5s.** It isn't arbitrary: it's the
historical Grails default. `I18nGrailsPlugin` set `field('cacheSeconds',
int).value(Settings.I18N_CACHE_SECONDS, '5')`, and this PR deliberately
preserves that so an upgrading application sees the same reload timing it had.
It's also what the deprecated `grails.i18n.cache.seconds` alias documents.
Anyone who prefers tighter feedback sets `spring.messages.cache-duration: 1s`,
which now works where it silently did nothing before.
**2. Multiple application descriptors → warn and merge — I'd keep the
error.** Merging doesn't remove the ambiguity, it hides it: two applications
both shipping `messages` would have their precedence decided by classpath
enumeration order, which is exactly the nondeterminism this design exists to
remove (and what @sbglasius flagged for plugin names earlier in this PR). The
scenario also isn't demonstrated — the full suite, 16k+ tests including every
multi-module test-example, passes with the check in place. The error already
names both descriptors and the likely cause. If a real multi-application
classpath turns up, I'd rather fix it with evidence in hand.
**3. Soften the BootstrapContext requirement — I don't think this case is
reachable, and the fallback would be worse.**
`GrailsBootstrapRegistryInitializer` is registered in `grails-core`'s
`META-INF/spring.factories` as a `BootstrapRegistryInitializer`, so
`SpringApplication` loads it automatically — `@SpringBootTest` included.
`ApplicationContextRunner` doesn't invoke `EnvironmentPostProcessor`s at all,
so the branch never runs there either. More importantly, the proposed fallback
of sorting alphabetically would silently produce *wrong* precedence:
correctness here depends on reverse topological order, which is what reproduces
the behaviour of the message source this replaces. A warning plus wrong
messages is worse than a clear failure.
**4. Legacy plugin fallback — out of scope, deliberately.** A Grails 7
plugin JAR already doesn't work in Grails 8 for reasons unrelated to i18n; the
upgrade guide states it directly in several places — "the plugin must be
rebuilt against the typed metadata API", "the plugin must be rebuilt for Grails
8", "the plugin's trait must be recompiled with Groovy 5", and plugin authors
"must compile against Spring Framework 7". Adding a runtime classpath scan to
preserve message bundles for artifacts that can't load anyway would reintroduce
the exact scan this PR removes — the one thing that makes the i18n path
AOT-hostile — for no working use case. Plugins are recompiled for 8 regardless;
a namespaced bundle works unchanged under both 7 and 8 (documented in the
upgrade section), so the migration is a one-time rename.
**5. Native image end-to-end verification — not currently possible.** There
is no native build path for a Grails application on `8.0.x` yet: no
`org.graalvm.buildtools` plugin, no `SpringBootAotPlugin`/`processAot` wiring,
and no native target in `end-to-end/`. That's what #16094 is for, and it's
still open. This PR doesn't claim native support works end to end — the
Limitations section says so explicitly, and calls out that the resource-pattern
choice follows Spring Boot's own precedent rather than being proven by a native
build. Once #16094 lands, an end-to-end native test is the right follow-up, and
this PR is what makes the i18n side ready for 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
