codeconsole commented on PR #15755:
URL: https://github.com/apache/grails-core/pull/15755#issuecomment-4906776587
Agreed on all points — adopting your direction, and closing this PR in favor
of a replacement rather than patching it. The independently useful pieces have
already been extracted (#13863 got the hardening delta, #15916 has the
i18n/URL-mappings `@ConditionalOnMissingBean` migrations, stacked on #13863).
Since we're close to the last 8.0.0 milestone and major changes shouldn't
land in an RC, this goes directly to your best case in 8.0 rather than staging
it across two majors. What the replacement PR does:
1. **Retimes the existing entry point — no new method.**
`doWithSpring`/`doWithRuntimeConfiguration` runs before Spring Boot
auto-configuration, via a `BeanDefinitionRegistryPostProcessor` added ahead of
`ConfigurationClassPostProcessor`. The plugin manager and `GrailsApplication`
are built exactly once from your `PluginDiscovery` — no second plugin manager,
no double plugin instantiation. This is the follow-up you scoped out of #15409
("we could move plugins completely to the autoconfiguration workflow"),
completed.
2. **Artefact discovery moves into the same early phase**, so `doWithSpring`
closures that iterate artefacts (controllers, services, interceptors) work
unmodified.
3. **`BeanRegistrar` is the native, documented registration API for
plugins.** Plugins expose a `BeanRegistrar` and we apply it pre-refresh through
Spring's own `BeanRegistryAdapter` — the same mechanism
`GenericApplicationContext.register(BeanRegistrar...)` uses.
4. **The bean DSL is deprecated in 8.0** (`@Deprecated(since = '8.0')` on
`doWithSpring`) pointing at (3) — the visible half of #14915.
One deliberate exclusion: reimplementing the DSL's *internals* on top of
`BeanRegistry` (the other half of #14915). The DSL supports constructs with no
clean `BeanRegistry` equivalent (parent/abstract bean definitions,
late-resolved bean references, `BeanConfiguration` post-processing), the
plumbing is unobservable to users, and the translation work gets deleted anyway
when the DSL is removed. Deprecated DSL closures keep draining through the
existing `RuntimeSpringConfiguration` path — just earlier — until removal.
Things we verified rather than assumed, in case any of it contradicts your
notes:
- **Dev reload is unaffected.** No reload path re-runs `doWithSpring` —
`informOfClassChange` → `onChange` writes straight to the registry, bypassing
`GrailsApplicationPostProcessor` — so the initial drain's position doesn't
matter to reloading.
- **App-over-plugin overrides keep working.** Grails forces
`allowBeanDefinitionOverriding=true` by default (`GrailsApp`), so
`resources.groovy`/application beans still replace early-registered plugin
beans. The one edge: an app that explicitly sets
`spring.main.allow-bean-definition-overriding=false` now gets a
`BeanDefinitionOverrideException` where the old single-drain merge was silent —
arguably more correct for someone opting into strictness, and it's called out
in the upgrade notes.
- **AOT is neutral-to-better.** `refreshForAotProcessing` executes all
registry post-processors, so early registration participates in AOT identically
— and build-time `@ConditionalOnMissingBean` evaluation now sees plugin beans,
so baked condition outcomes match runtime semantics instead of baking in an
auto-configured bean the late drain would override.
- **Closures touching `applicationContext` pre-refresh:** audited every
`doWithSpring` in-repo; all uses are pre-refresh-safe surface (environment,
classLoader, resources, the registry itself — nothing calls `getBean`). The
verification sweep did catch two real regressions this way — OSIV detection
gating on the auto-configured `dispatcherServlet` bean, and artefact scanning
missing the injected `packageNames()` — both fixed in the replacement PR, with
details there.
Net effect for 8.0: one lifecycle phase, `BeanRegistry` as the one true way,
DSL deprecated but working — no transitional API ever exists, which I think
addresses the "new way, then yet another way over 2 major versions" concern
directly.
Superseded by #15934 — closing.
--
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]
