jdaugherty commented on PR #15824:
URL: https://github.com/apache/grails-core/pull/15824#issuecomment-5208595820
I'm not sure why this was merged. We need to revert it until this can be
reviewed as I did not agree with this PR.
The premise doesn't hold up
Issue #14915 (type: blocker) says BeanBuilder "is ultimately backed by xml"
and needs reworking. That was never true for the common path. In the pre-merge
BeanBuilder.java, the XML fields are referenced in exactly four places:
- 161, 171–174 — construction
- 226, 237, 241 — xmlns
- 955 — createDynamicElementReader (the namespace DSL)
Ordinary bean(Foo) { ... } in resources.groovy went through
springConfig.addBeanDefinition / programmatic BeanDefinition all along. The XML
reader was constructed and then never used unless you wrote xmlns or imported a
.xml.
It doesn't reduce the XML coupling
importBeans(*.xml) still uses XmlBeanDefinitionReader. xmlns still uses
DefaultNamespaceHandlerResolver + XmlReaderContext +
BeanDefinitionParserDelegate. They moved into BeanBuilderXmlSupport and are
built on first use. If Spring actually drops those classes, Grails is in
exactly the same spot.
And there's no pressure today — I checked the jars on the classpath:
┌─────────────────────────────────┬────────────────────┐
│ Class │ spring-beans 7.0.8 │
├─────────────────────────────────┼────────────────────┤
│ XmlBeanDefinitionReader │ not deprecated │
├─────────────────────────────────┼────────────────────┤
│ DefaultNamespaceHandlerResolver │ not deprecated │
├─────────────────────────────────┼────────────────────┤
│ BeanDefinitionParserDelegate │ not deprecated │
├─────────────────────────────────┼────────────────────┤
│ XmlReaderContext │ not deprecated │
└─────────────────────────────────┴────────────────────┘
Same for MvcNamespaceHandler in spring-webmvc 7.0.8. Nothing was breaking or
even warning.
My BeanRegistrar question was never answered — the author's reply pivoted to
a comparison table against #16019 and argued "land this for Spring 7
BeanBuilder compatibility," but there was no compatibility break to fix.
BeanRegistrar is the rework #14915 asked for and it isn't here.
What it actually buys
Real but small: BeanBuilder construction no longer forces
getUnrefreshedApplicationContext() → initialiseApplicationContext(), so a
BeanBuilder that never builds a context no longer creates a
GrailsApplicationContext. It also cleans up a latent bug where
setClassLoader(null) passed the raw null param to the reader while the resolver
got the defaulted field.
What I'd have flagged
Broken protected extension hook. initializeSpringConfig() and
setClassLoader() no longer dispatch to protected
initializeBeanBuilderForClassLoader(ClassLoader). It survives as dead code that
now forces XML init if a subclass calls it — inverting the PR's own intent.
grails.spring.BeanBuilder is public API. WebBeanBuilder is the only in-repo
subclass and doesn't override it, but external plugin subclasses silently stop
being called. The author documented this in the PR body as a "follow-up"
instead of fixing it.
Unremarked behavior change in importBeans. The new helper adds
beanReader.setBeanClassLoader(classLoader), which the old code did not do. That
switches XML-imported definitions from deferred beanClassName resolution to
eager class resolution at import time, using BeanBuilder's classloader rather
than the target factory's. The new test asserts
createBeanDefinition('xmlBean').beanClass == String, which only passes because
of that change — so the test enshrines it. Not mentioned anywhere in the PR
description.
Duplicate test FQN. The new
grails-spring/src/test/groovy/grails/spring/BeanBuilderTests.groovy collides by
fully-qualified name with the existing 1002-line
grails-test-suite-uber/src/test/groovy/grails/spring/BeanBuilderTests.groovy —
which already covers the refactored paths (testImportSpringXml,
testUseSpringNamespaceAsMethod, testUseTwoSpringNamespaces,
testSpringNamespaceBean). Per CLAUDE.md #11 that suite should have been
extended, not shadowed. Two grails.spring.BeanBuilderTests in aggregate reports
is a maintenance hazard.
Docs oversell it. whatsNew and upgrading80x now tell users Grails 8 "keeps
the resources.groovy BeanBuilder DSL backward compatible on Spring Framework
7," implying a Spring 7 problem was solved. Nothing was broken, and XML is
still how xmlns works.
Merged without human approval. reviewDecision is empty — the only reviews
are Copilot (COMMENTED) and the author's own comment. Per CONTRIBUTING, "all
other changes" require 1 reviewer, review-then-commit.
Suggested follow-up
Reopen #14915 (or file the BeanRegistrar work fresh) since the blocker is
closed but unaddressed, and a small follow-up to restore the
initializeBeanBuilderForClassLoader dispatch, de-dupe the test class name, and
either revert or document the setBeanClassLoader change. Want me to draft that
issue text or a post-merge review comment?
--
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]
