jamesfredley commented on code in PR #15557:
URL: https://github.com/apache/grails-core/pull/15557#discussion_r3343105773
##########
grails-gsp/core/src/test/groovy/org/grails/gsp/GspCompileStaticSpec.groovy:
##########
@@ -86,6 +93,10 @@ class GspCompileStaticSpec extends Specification {
compileStatic << [true, false]
}
+ // Note: In Groovy 5, the g.message() syntax with g. prefix fails static
type checking
+ // because the type checking extension doesn't properly resolve the 'g'
taglib property.
Review Comment:
The `g.taglib()`-from-`@CompileStatic` resolution itself is handled in this
PR via the namespace-matching path in
`GroovyPageTypeCheckingExtension.methodNotFound` (the approach @paulk-asert
recommended for GROOVY-12041). These two `@IgnoreIf` tests are a *different*
STC behaviour: they assert that an **undeclared variable** in a GSP
(`${somename}`) raises a compile error. Under Groovy 5 the STC
`unresolvedVariable`/`unresolvedProperty` callbacks no longer fire when the
receiver inherits `getProperty(String)`, so that negative assertion can't be
made. Anchoring tickets: GROOVY-6362 (original support) and GROOVY-11817
(claimed restore). Related: #15669 (`@GrailsCompileStatic` taglib support for
controllers). Left disabled with the tickets referenced; they'll be re-enabled
once the STC callback regression is fixed upstream.
##########
grails-gsp/plugin/src/main/groovy/org/grails/plugins/web/taglib/UrlMappingTagLib.groovy:
##########
@@ -184,14 +184,14 @@ class UrlMappingTagLib implements TagLibrary {
// display previous link when not on firststep unless omitPrev is true
if (currentstep > firststep && !attrs.boolean('omitPrev')) {
linkParams.offset = offset - max
- writer << callLink(appendClass((Map) linkTagAttrs.clone(),
'prevLink')) {
+ writer << callLink(appendClass(new LinkedHashMap(linkTagAttrs),
'prevLink')) {
Review Comment:
This is outdated - the `(Map) ...clone()` -> `new LinkedHashMap(...)` change
is no longer part of this PR's diff (it's already on `8.0.x`). The only
remaining change this PR makes to `UrlMappingTagLib` is line 302,
`linkParams.putAll(paramsAttr as Map)`: an explicit cast because
`attrs.remove('params')` is typed `Object` and `Map.putAll` needs a `Map` under
`@CompileStatic`. No clone semantics are changed.
##########
grails-test-suite-web/src/test/groovy/org/grails/web/binding/DataBindingTests.groovy:
##########
@@ -404,16 +404,10 @@ class DataBindingTests extends Specification implements
ControllerUnitTest<TestC
}
void testAssociationsBinding() {
- when:
- def authorClass = new Author()
-
- Author.metaClass.static.get = { Serializable id ->
- def result = new Author()
- result.id = id as long
- result.name = "Mocked ${id}"
- result
- }
+ given:
+ GroovySpy(Author, global: true)
Review Comment:
Added a `cleanup:` block that calls
`GroovySystem.metaClassRegistry.removeMetaClass(Author)`, so the global
`GroovySpy` is torn down even if the feature method fails.
##########
grails-test-examples/app1/build.gradle:
##########
@@ -92,5 +92,6 @@ test {
apply {
from
rootProject.layout.projectDirectory.file('gradle/functional-test-config.gradle')
from
rootProject.layout.projectDirectory.file('gradle/test-webjar-asset-config.gradle')
+ from
rootProject.layout.projectDirectory.file('gradle/boot4-disabled-integration-test-config.gradle')
Review Comment:
Resolved on the current branch. `integrationTest` is no longer disabled for
app1: the `indy=false` parameterized-controller-action blocker is fixed
framework-side by the `OptimizingStatementWriter.ClassNodeSkip` tag in
`ControllerActionTransformer` (commit fb725b178b), and app1 does not use
SiteMesh 3. app1's `build.gradle` no longer contains any `integrationTest {
enabled = false }` / `boot4-disabled` reference, so the property is honoured
per-module rather than excluding both.
##########
grails-bootstrap/src/main/groovy/org/grails/config/NavigableMap.groovy:
##########
@@ -174,7 +170,14 @@ class NavigableMap implements Map<String, Object>,
Cloneable {
}
private static Object resolveConfigMapValue(Map map, Object... keys) {
- keys.inject(map) { acc, key -> acc instanceof Map ? acc[key] : null }
+ // Groovy 5: indexing a ConfigObject with a missing key inserts an
empty ConfigObject,
Review Comment:
Documented in the upgrade guide
(`grails-doc/src/en/guide/upgrading/upgrading80x.adoc`, new section 24.2
"Probing a ConfigObject for a missing key"): under Groovy 5, indexing a
`ConfigObject` with a missing key inserts an empty nested `ConfigObject`, so
config navigation should use `containsKey`/`get` rather than relying on `[]`
reading back as null - which is what the
`resolveConfigMapValue`/`readWithoutCreating` helpers now do.
##########
grails-core/src/main/groovy/grails/dev/commands/template/TemplateRendererImpl.groovy:
##########
@@ -55,7 +55,7 @@ class TemplateRendererImpl implements TemplateRenderer {
@Override
@CompileDynamic
void render(Map<String, Object> namedArguments) {
- if (namedArguments?.template && namedArguments?.destination) {
+ if (namedArguments?.template != null && namedArguments?.destination !=
null) {
Review Comment:
Good catch - restored Groovy truth on the `template` operand. The
`render(Map)` and `render(CharSequence, ...)` overloads now use `if (template
&& destination != null)`, so an empty `template` (e.g. `template: ''`) is falsy
and skips rendering as before. The `destination` operand keeps a `!= null`
check because it is a `File` being created and must not be gated on existence.
--
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]
