jdaugherty commented on code in PR #16184:
URL: https://github.com/apache/grails-core/pull/16184#discussion_r3961091718
##########
grails-web-url-mappings/src/main/groovy/org/grails/web/mapping/DefaultLinkGenerator.groovy:
##########
@@ -76,7 +76,13 @@ class DefaultLinkGenerator implements LinkGenerator,
PluginManagerAware {
GrailsPluginManager pluginManager
- @Autowired
+ /**
+ * The mappings a link to a controller and action is created from. An
application that maps no
+ * URLs - one routing with Spring MVC, say - has none, and still generates
links to a resource
+ * or to a path, so this is the one kind of link it cannot generate rather
than a reason for it
+ * not to start.
+ */
+ @Autowired(required = false)
Review Comment:
This has to stay required. The URL mappings holder is core to the link
generator and to the GSP tags built on it (`ApplicationTagLib` autowires both),
and turning it into a nullable field with a runtime `IllegalStateException`
softens a Grails invariant to accommodate a consumer outside this repo: the
asset pipeline's auto-configuration contributes a `grailsLinkGenerator` to an
application that maps no URLs.
The fix belongs on that side. Whatever contributes a `DefaultLinkGenerator`
to a Spring MVC application should also contribute an empty
`grailsUrlMappingsHolder` (`new DefaultUrlMappingsHolder([])`), or
`GspAutoConfiguration` can do so under `@ConditionalOnMissingBean(name =
"grailsUrlMappingsHolder")`. Please revert this and the null check below, along
with the `LinkGeneratorSpec` cases that pin a generator without a holder.
##########
grails-core/src/main/groovy/grails/boot/config/GrailsEarlyPluginRegistrationPostProcessor.java:
##########
@@ -120,6 +127,20 @@ public void
postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry) t
return;
}
+ // Two things make a context a Grails application: GrailsApp launched
it, which it records by
+ // stashing the sources it was given, or one of its sources is a
Grails application class.
+ // This initializer is registered for every Spring Boot application
with grails-core on its
+ // class path, and the plugin lifecycle is not something the rest of
them asked for: it would
+ // contribute a GrailsApplication, a plugin manager and the beans of
every plugin found, over
+ // the top of whatever the libraries they did ask for auto-configure
for themselves.
+ boolean launchedByGrails =
+
applicationContext.getBeanFactory().getSingleton(APPLICATION_SOURCE_CLASSES_BEAN_NAME)
!= null;
+ Class<?>[] applicationSources =
resolveApplicationSourceClasses(registry);
+ if (!launchedByGrails &&
!containsApplicationClass(applicationSources)) {
Review Comment:
The class Javadoc above (line 87) still says that when the application was
not started through `GrailsApp` "the phase proceeds without application
classes". With this gate it stands down instead; please update it.
One case worth spelling out, here or in the upgrade note: a
`@SpringBootTest(classes = SomeConfig)` inside a Grails application that does
not name the `Application` class used to get the plugin lifecycle and now gets
none. `@Integration` is unaffected because it always names the application
class.
This is likely a critical regression. We should either document this in the
upgrade notes or restore the behavior for Grails apps to automatically
configure SpringBootTest annotated tests.
##########
grails-gsp/grails-sitemesh3/src/main/groovy/org/grails/plugins/sitemesh3/Sitemesh3GrailsPlugin.groovy:
##########
@@ -147,9 +147,13 @@ class Sitemesh3GrailsPlugin extends Plugin {
// The SiteMesh 3 specific key wins; fall back to the legacy
// grails.views.layout.default key so existing apps keep their
- // configured default layout when switching.
+ // configured default layout when switching, and finally to
SiteMesh's own
+ // sitemesh.decorator.default - the key a Spring Boot application
using GSP for views
+ // configures, and the one Sitemesh3EnvironmentPostProcessor
derives from the Grails
+ // keys above, so it only decides when neither of them is set.
String defaultLayout =
config.getProperty('grails.sitemesh.default.layout') ?:
- config.getProperty('grails.views.layout.default')
+ config.getProperty('grails.views.layout.default') ?:
+ config.getProperty('sitemesh.decorator.default')
Review Comment:
This is a behaviour change for Grails applications, not only a standalone
fallback. An application that set `sitemesh.decorator.default` without either
Grails key previously got the finder's `application` default and now gets the
configured layout. Probably what people want, but it should be stated in the
upgrade notes alongside the layouts.adoc change, which currently reads as a
description of existing behaviour.
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/views/gsp/GroovyPagePlugin.groovy:
##########
@@ -437,6 +436,22 @@ class GroovyPagePlugin implements Plugin<Project> {
}
}
+ // The archives below take the compiled pages by copy. They belong on
the test class path as
+ // well, so a test of the application loads the same pages it ships -
the view registry among
+ // them. They cannot be registered as source set output: that output
is what `classes` builds,
+ // and compileGroovyPages runs after `classes`, so it would cycle.
+ //
+ // The main runtime class path is deliberately left alone. A boot
archive packages every
+ // directory of it into its own classes directory, and the pages are
copied there already, so
+ // each page would arrive twice - and an application run from the
build renders its templates
+ // as they are edited, which is what a page compiled ahead of the edit
would stand in the way
+ // of.
+ FileCollection compiledPages = project.files(destDir,
webappDestDir).builtBy(compileGroovyPages)
+ SourceSet testSourceSet = SourceSets.findSourceSet(project,
SourceSet.TEST_SOURCE_SET_NAME)
+ if (testSourceSet != null) {
+ testSourceSet.runtimeClasspath = testSourceSet.runtimeClasspath +
compiledPages
Review Comment:
This puts every Grails project's `test` task behind `compileGroovyPages`.
The classpath is `builtBy` the compile task, so `./gradlew test` now forks and
compiles all of `grails-app/views` first, and any GSP edit invalidates `test`
and reruns the whole unit suite.
A Grails project gets nothing for that cost. Unit tests render GSP from
source through `GroovyPageUnitTestResourceLoader`, and integration tests run
with the project directory available, so `GroovyPagesGrailsPlugin` never
populates the precompiled map. The consumer is a standalone Spring Boot
application such as the gsp-spring-boot example, whose tests live in `src/test`
and whose build strips the templates. It is right that applying this plugin
makes that work without the application wiring anything itself.
Please scope it to that case: add the compiled pages to the test runtime
classpath only when `GrailsGradlePlugin` is not applied to the project, so a
Grails application or plugin build is left as it was. `webappDestDir` also
needs `builtBy(compileWebappGroovyPages)`; as written it can be stale on the
classpath.
Detect the Grails build lazily, not with `hasPlugin` at configure time and
not in `afterEvaluate`. Both plugins have historically been applied with `apply
plugin:` in either order, and either can arrive from another plugin or a later
callback, so an eager check is order-dependent. Record presence with
`project.plugins.withType(GrailsGradlePlugin) { grailsBuild = true }`, which
fires on apply however the plugin is applied, and make the decision inside a
provider that Gradle evaluates when it builds the task graph, after every
configuration-phase hook has run:
```groovy
FileCollection compiledPages = project.files(project.provider {
grailsBuild ? [] : [project.files(destDir,
webappDestDir).builtBy(compileGroovyPages, compileWebappGroovyPages)]
})
```
Gradle infers the `builtBy` dependencies from the provider's value, so
`test` depends on the compile tasks only when the pages are actually
contributed. The same works in the other direction if you prefer:
`GrailsGradlePlugin` already has a
`withPlugin('org.apache.grails.gradle.grails-gsp')` block and could flip a lazy
`Property<Boolean>` on this plugin's extension from there, provided the
extension is registered directly in `apply()` rather than inside the
`withPlugin('groovy')` callback.
##########
grails-core/src/main/groovy/org/grails/plugins/CoreGrailsPlugin.groovy:
##########
@@ -91,13 +93,19 @@ class CoreGrailsPlugin extends Plugin {
private static final String SPRING_PROXY_TARGET_CLASS_CONFIG =
'spring.aop.proxy-target-class'
def beans = {
- bean(ClassLoader).primary() { GrailsApplication grailsApplication ->
+ // Both of these are the GrailsApplication read through another type,
so both stand down for
+ // an application that has none: this configuration is contributed by
every Spring Boot
+ // application with grails-core on its class path, where only a Grails
application has the
+ // plugin lifecycle that builds one.
+ bean(ClassLoader).primary().annotate(ConditionalOnBean, value:
GrailsApplication) { GrailsApplication grailsApplication ->
Review Comment:
This is Spring Boot conditional wiring inside the core plugin's `beans`
block, and it makes core beans depend on Boot's condition ordering.
`@ConditionalOnBean(GrailsApplication)` holds only because the early
registration phase promotes `grailsApplication` before
`ConfigurationClassPostProcessor` evaluates the generated auto-configuration.
Any context where that ordering does not hold loses both beans, and
`GroovyPagesGrailsPlugin` then fails on `ref('classLoader')`.
Conventionally these beans exist because there is a Grails application; the
core plugin should not have to defend against contexts without one. If the
intent is that a non-Grails Boot application gets nothing from core, gate the
generated `CoreAutoConfiguration` as a whole rather than individual beans. As
it stands the surface is mixed: these two back off while the rest still land in
plain Boot applications, including `GrailsPlaceholderConfigurer` with
`ignoreUnresolvablePlaceholders=true`, which `CoreAutoConfigurationSpec` now
asserts as intended behaviour.
##########
grails-gsp/grails-web-gsp-taglib/src/main/groovy/org/grails/web/pages/StandaloneTagLibraryLookup.java:
##########
@@ -39,7 +42,12 @@
* @author Lari Hotari
* @since 2.4.0
*/
-public class StandaloneTagLibraryLookup extends TagLibraryLookup implements
ApplicationListener<ContextRefreshedEvent> {
+public class StandaloneTagLibraryLookup extends TagLibraryLookup
+ implements SmartInitializingSingleton,
ApplicationListener<ContextRefreshedEvent> {
+
+ /** What {@link Artefact} marks a tag library with, the way a Grails
plugin declares one. */
+ private static final String TAG_LIB_ARTEFACT = "TagLib";
Review Comment:
Use `TagLibArtefactHandler.TYPE` rather than a local copy of the string. It
lives in grails-taglib, which this class already depends on for
`DefaultGrailsTagLibClass`.
##########
grails-doc/src/en/guide/upgrading/upgrading80x.adoc:
##########
@@ -3025,3 +3025,21 @@ Grails 7
(`org.apache.grails.data:grails-datamapping-async`) coordinates in a pr
----
./rename_gradle_artifacts.sh -l my/project/location
----
+
+==== 53. Grails Plugins Run for a Grails Application Only
+
+The Grails plugin lifecycle ran for any Spring Boot application that had
`grails-core` on its class path,
+whether or not it was a Grails application. An application that depended on a
Grails library — GSP for its
+views, say — was given a `GrailsApplication`, a plugin manager, and the beans
of every plugin found on the
+class path, over the top of what the libraries it did ask for auto-configure
for themselves.
+
+The lifecycle now runs only for a Grails application: one that `GrailsApp`
launched, or one where a source of
+the context is a Grails application class, which is what
`grails-app/init/Application.groovy` is. A Grails
+application is unaffected either way. A Spring Boot application using a Grails
library gets that library's
+auto-configuration and nothing else.
+
+This matters if you deliberately put a Grails plugin on the class path of an
application that is not a Grails
+application and relied on its `doWithSpring` or `beanRegistrar` beans being
contributed. Those beans are no
Review Comment:
Please also cover the in-application case: a `@SpringBootTest` in a Grails
application that names a configuration class other than `Application` no longer
gets the plugin lifecycle. That is the change a Grails user is most likely to
hit.
##########
grails-gsp/core/src/main/groovy/org/grails/gsp/compiler/GroovyPageCompiler.groovy:
##########
@@ -151,16 +151,14 @@ class GroovyPageCompiler {
}
// write the view registry to a properties file (this is read
by GroovyPagesTemplateEngine at runtime)
+ // The registry names every page compiled here, whether or not
this run had to recompile it, so it
+ // is written whole rather than merged into what an earlier
run left behind. Merging kept naming
+ // pages that have since been renamed, removed or registered
under a different prefix, against
+ // classes no longer on the class path.
File viewregistryFile = new File(targetDir,
'gsp/views.properties')
viewregistryFile.parentFile.mkdirs()
// Use SortedProperties to ensure a consistent order of
entries for reproducible builds
Properties views =
CollectionFactory.createSortedProperties(false)
- if (viewregistryFile.exists()) {
- // only changed files are added to the mapping, read the
existing mapping file
- viewregistryFile.withInputStream { stream ->
- views.load(new InputStreamReader(stream, 'UTF-8'))
- }
- }
views.putAll(compileGSPRegistry)
Review Comment:
Writing the registry whole is the right fix, but it now relies on every
caller handing the compiler the full source set on each run;
`GroovyPageForkCompileTask` and `GroovyPageCompilerTask` both do today. Worth
saying so in this comment, since the old merge existed for callers that passed
only changed files.
--
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]
