codeconsole commented on code in PR #15939:
URL: https://github.com/apache/grails-core/pull/15939#discussion_r3548486158
##########
grails-core/src/main/groovy/org/grails/plugins/DefaultGrailsPlugin.java:
##########
@@ -391,13 +391,17 @@ public void
doWithRuntimeConfiguration(RuntimeSpringConfiguration springConfig)
b.setVariable("resolver", getResolver());
if (plugin instanceof Plugin) {
- Closure c = ((Plugin) plugin).doWithSpring();
+ Plugin pluginObject = (Plugin) plugin;
+ BeanBuilder bb = new BeanBuilder(getParentCtx(), springConfig,
grailsApplication.getClassLoader());
+ bb.setBinding(b);
+ // Legacy closure-returning hook: doWithSpring() returns a
bean-defining closure
+ Closure c = pluginObject.doWithSpring();
if (c != null) {
- BeanBuilder bb = new BeanBuilder(getParentCtx(), springConfig,
grailsApplication.getClassLoader());
- bb.setBinding(b);
c.setDelegate(bb);
bb.invokeMethod("beans", new Object[]{c});
}
+ // Method-based hook: doWithSpring(BeanBuilder) registers beans
directly against the builder
+ pluginObject.doWithSpring(bb);
Review Comment:
Good question. I looked at how the sibling method-based TagLib PR (#15465)
handles a tag defined in both forms: it doesn't error —
`TagOutput.captureTagOutput` runs the closure and treats the method as a
fallback, and registration only emits a `LOG.debug`. But I don't think we
should copy that precedence here, because its rationale doesn't transfer.
TagLib method-tags are *auto-detected by convention*
(`TagMethodInvoker.isTagMethodCandidate` promotes any public `(Map attrs)` /
`(Closure body)` / `(Map attrs, Closure body)` method into a tag). So
closure-first exists there to stop the new automatic scanning from silently
overriding existing *explicit* closures — it's a "do no harm to existing
taglibs" guard. Our two `doWithSpring` forms are both *explicit* overrides, and
nothing pre-existing defines both, so there's no back-compat behavior to
protect and no accidental-collision risk.
Given both forms are deliberate, "both defined" almost always means a
migration between them is incomplete. Rather than error (which fails plugin
load and breaks that transient state) or silently drop one form (app-breaking
behavior hiding under a warning), the PR now **runs both and logs a WARN**
telling the author to consolidate. That keeps a mid-migration plugin working
while surfacing the issue.
I've pushed this. I also scoped the hook to the `Plugin` class rather than
the `GrailsApplicationLifeCycle` interface (the interface is map-coerced in the
testing support and Groovy's map proxies don't honour default methods, which
would break unit tests), and corrected the PR description, which previously
overclaimed the interface/adapter/post-processor coverage.
Happy to switch to a hard error instead if you'd prefer forcing
consolidation over tolerating the migration window.
--
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]
