matrei commented on PR #16184:
URL: https://github.com/apache/grails-core/pull/16184#issuecomment-5655465856
**Second round**, on 16eb980bb8 against 8.0.x (merge base a5758d657f is the
current tip of 8.0.x, so the branch is clean). Reviewed the six commits since
the first round, 3ac93a3aa9..16eb980bb8, and ran
`:grails-gsp-spring-boot:test`, `:grails-sitemesh3:test`,
`GroovyPageCompilerSpec`, `CoreGrailsPluginRegistrarSpec`, the gsp-spring-boot
example's tests, `codeStyle` for the touched modules and
`GroovyPagePluginFunctionalSpec` in grails-gradle. All green.
Every point from the first round is addressed: the encoding defect, the four
test gaps, the two doc gaps and both design notes. What is left is in the new
guide page, plus one test-hygiene detail.
### Docs: the tag library example in `gspInSpringBoot.adoc` does not compile
and is not what the example does
`grails-doc/src/en/guide/theWebLayer/gsp/gspInSpringBoot.adoc:80-81`:
```java
public AssetsTagLib assetsTagLib(AssetProcessorService
assetProcessorService) {
return new AssetsTagLib(assetProcessorService);
}
```
`AssetsTagLib` (asset-pipeline-grails 5.2.0-M3, the version the BOM manages)
has only a no-arg constructor; `assetProcessorService` is a plain `Object`
property. And `AssetProcessorService` is a Grails service artefact of that
plugin, so in an application with no plugin lifecycle there is no bean of it to
inject either - the snippet needs the reader to declare that too, and the tag
library also expects `grailsApplication` and a `tagLibraryLookup` from the
`TagLibraryInvoker` trait. None of this is exercised: per your own comment on
48175cf3fd, `asset-pipeline-grails` is off the example's class path and the
layout links the compiled files by name. The TIP at line 117 ("using all of
this - precompiled views, the asset pipeline, ...") therefore points the reader
at an application that does not demonstrate the section it closes.
Suggest either a neutral example that is actually covered - a
`@grails.gsp.TagLib` class the application declares as a `@Bean`, which is what
`StandaloneTagLibraryLookupSpec` and `TagLibraryLookupRegistrarSpec` test - or
wiring the asset tag library in the example for real and documenting what that
took. Keep the `@Artefact("TagLib")` sentence at line 85; that is covered.
### Docs: two rendering details in the same page
- `gspInSpringBoot.adoc:26,35`: `{version}` inside a `[source,groovy]` block
is not substituted; listing blocks only get verbatim subs. `pluginSupport.adoc`
uses `[source,groovy,subs="attributes"]` for the same line. Use
`subs="attributes+"` here so the `// <1>` callouts keep working.
- `gspInSpringBoot.adoc:31`: `id 'org.apache.grails.gradle.grails-gsp'` with
no version does not resolve in a plain Spring Boot build. Either `version
'{version}'` on the line, or say that the version comes from `pluginManagement`
/ the gradle BOM.
### Test: `CoreGrailsPluginRegistrarSpec` leaves duplicates behind
`grails-core/src/test/groovy/org/grails/plugins/CoreGrailsPluginRegistrarSpec.groovy:221-222`:
the feature removes the two Grails creators, the registrar under test puts
them back (`GroovyAwareAutoProxyCreators.registerWithAopConfigUtils` adds
anything the list does not contain), and then cleanup does
`priorityList.addAll(removed)` - so every run appends a second copy of both
classes to `APC_PRIORITY_LIST`. Harmless for `findPriorityForClass`, which uses
`indexOf`, but the Javadoc at line 216 says the fork is left as it was found,
and it is not. Restore only what is missing:
```groovy
cleanupActions << { removed.each { if (!priorityList.contains(it))
priorityList.add(it) } }
```
### Nit
`Sitemesh3LayoutTagLibSpec.groovy:80-89` pins the unencoded output of a tag
library built with no `codecLookup`. It makes the wiring load-bearing, as the
comment says, but it also turns the absence of a guard into specified
behaviour. If the intent is "the lookup is required", a `@Autowired(required =
true)` failure at context start is the stronger contract, and this case could
go.
### Verified as correct
- `Sitemesh3LayoutTagLib`: `@Autowired` on the property lands on the field,
as it does in `RenderSitemeshTagLib`; a Grails application is unaffected
because the codecs plugin always contributes `codecLookup`.
`StandaloneContextTest` fails with the annotation removed.
- `ReplaceViewResolverRegistrar` (`GspAutoConfiguration.java:428`): the
registrar runs after user configuration classes are loaded, so a user
`jspViewResolver` is seen and left alone; `TagLibraryLookupRegistrarSpec`
covers both branches. A bean of that name registered by a *later*
auto-configuration now hits `BeanDefinitionOverrideException` under Boot's
default rather than being silently aliased over - same timing as the old
`@Bean(name = {...})` form, and loud, which is the right failure. The alias is
registered regardless of `spring.gsp.replaceViewResolverBean`, as before.
- `GspAutoConfigurationSpec`: the `views.properties` fixture names real
`GroovyPage` subclasses with the metadata constants the compiler emits, so
`findPage` goes through `GroovyPageCompiledScriptSource`; reverting either
`setPrecompiledGspMap` or the `cleanUri` search path fails it in-module.
- `GroovyPageCompilerSpec` now writes generated sources under its `@TempDir`.
- `GroovyPagePluginFunctionalSpec`: the non-Grails project asserts `test`
depends on both `compileGroovyPages` and `compileWebappGroovyPages`; the Grails
project asserts neither. Passes against the branch.
- Example: session-scoped selection, cookie-keeping client in `JspViewTest`,
and `StandaloneContextTest` pins `pluginManager`, `grailsConfigProperties` and
`classLoader` (`GrailsApplication.CLASS_LOADER_BEAN`) absent. The stale Javadoc
sentence is gone.
- `upgrading80x.adoc` no longer names `Sitemesh3EnvironmentPostProcessor`.
The new page names only public API and configuration keys; the property
defaults it lists match `GspAutoConfiguration`. The `toc.yml` entry sits under
the GSP section next to `resources` and `makingChangesToADeployedApplication`,
and the `====` heading level matches those siblings.
- README rewrite is accurate to the module.
### Process
Unchanged from round one: whether this lands on 8.0.x before RC1 is still
open in the thread.
--
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]
