jamesfredley commented on PR #15465: URL: https://github.com/apache/grails-core/pull/15465#issuecomment-4415816914
Following up on my earlier review with empirical reproducers for the issues that are introduced by this PR (i.e. behaviors that did not exist on `8.0.x` before these changes). I kept the scope tight — pre-existing surprises (like closure tags also being unable to use names like `raw` because `raw()` is a trait method) are excluded. A single Spock spec exercises each issue against the public `TagMethodInvoker` API on the PR's HEAD (`7c89b78`). All six tests pass on this branch, demonstrating the behaviors are real and observable from user code. **Reproducer:** [`Pr15465ReproducerSpec.groovy`](https://github.com/jamesfredley/grails-core/blob/pr15465-issue-reproducers/grails-gsp/grails-taglib/src/test/groovy/org/grails/taglib/Pr15465ReproducerSpec.groovy) on branch [`pr15465-issue-reproducers`](https://github.com/jamesfredley/grails-core/tree/pr15465-issue-reproducers). Run with: ``` ./gradlew :grails-taglib:test --tests org.grails.taglib.Pr15465ReproducerSpec ``` --- ### C1 — `-parameters` compile flag is silently required for the headline feature The PR's marquee feature — `def greeting(String name)` binding from a `name="..."` attribute — depends on `Parameter.getName()` returning the real source name at runtime. That requires `-parameters` for `JavaCompile` and `groovyOptions.parameters = true` for `GroovyCompile`. The framework's own `build-logic/plugins/.../CompilePlugin.groovy` sets both. The user-facing `grails-gradle/.../GrailsGradlePlugin.groovy` sets neither. Spring Boot's plugin enables `-parameters` for `JavaCompile` since 3.2 but never for `GroovyCompile`. A user app inheriting Grails' Gradle plugin defaults will see `Parameter.getName()` return `arg0`, `attrs.containsKey("arg0")` returns `false`, and the call surfaces as `MissingMethodException` with no hint about the missing flag. **Reproducer test:** [`C1: tag method binding silently fails when parameter names are not preserved`](https://github.com/jamesfredley/grails-core/blob/pr15465-issue-reproducers/grails-gsp/grails-taglib/src/test/groovy/org/grails/taglib/Pr15465ReproducerSpec.groovy#L40-L72) — compiles the same TagLib twice via `GroovyClassLoader` (once with `CompilerConfiguration.parameters = true`, once with the default `false`) and shows only the first invocation succeeds. **Suggestion:** have `GrailsGradlePlugin` apply both flags to user-app builds; or detect synthetic names (`arg0`, `arg1`) at TagLib registration and emit a startup error with remediation steps. --- ### H1 — convention-based discovery is opt-out, exposing helpers as tags `TagMethodInvoker.isTagMethodCandidate` accepts every public, non-static, non-getter/setter method on a TagLib class minus `Object`/`GroovyObject` overrides, eight hard-coded framework names, and `@NotATag`-annotated methods. That leaves any user helper method silently registered as a tag. This is a behavior change. Before this PR you opted IN by writing `Closure x = { ... }`. After, every public method is opt-in by default and the user opts OUT with `@NotATag`. Existing TagLibs recompiled against `8.0.x` silently gain new tags they never declared. The inheritance walk in `getCandidateMethods` makes this worse — methods on an abstract base TagLib are exposed on every subclass. **Reproducer tests:** - [`H1: a public helper method on a TagLib is silently registered as a tag`](https://github.com/jamesfredley/grails-core/blob/pr15465-issue-reproducers/grails-gsp/grails-taglib/src/test/groovy/org/grails/taglib/Pr15465ReproducerSpec.groovy#L78-L90) — `def formatDate(Date d)` and `def buildInternalUrl(String path)` both show up in `getInvokableTagMethodNames` alongside the intended `greet` tag. - [`H1b: helper methods inherited from an abstract base TagLib become tags on every subclass`](https://github.com/jamesfredley/grails-core/blob/pr15465-issue-reproducers/grails-gsp/grails-taglib/src/test/groovy/org/grails/taglib/Pr15465ReproducerSpec.groovy#L92-L100) — `sharedUtility` declared on an abstract parent shows up as a tag on the concrete child. **Suggestion:** flip polarity to opt-in (`@Tag` annotation), restrict candidate discovery to directly-declared methods (skip the hierarchy walk), or emit an INFO-log enumeration of discovered tag methods at startup so users can audit silent registrations. --- ### H2 — `registerTagMetaMethods` default for `overrideMethods` flipped from `false` to `true` Diff confirms the change in `TagLibraryMetaUtils.groovy`: ```diff - registerMethodMissingForTags(emc, lookup, namespace, tagName, addAll, false) + registerMethodMissingForTags(emc, lookup, namespace, tagName, addAll, overrideMethods) - static void registerTagMetaMethods(MetaClass emc, TagLibraryLookup lookup, String namespace) { + static void registerTagMetaMethods(MetaClass emc, TagLibraryLookup lookup, String namespace, + boolean overrideMethods = true) { ``` Documented in `upgrading70x.adoc §16.1`, but the runtime failure mode is invisible: a user TagLib defining `def actionSubmit(Map x)` (a name shared with `FormTagLib`'s default-namespace tag) silently has its method shadowed when the tag dispatcher is registered. No log, no exception, the user's method just stops being called. **Reproducer test:** [`H2: registerTagMetaMethods default for overrideMethods is now 'true' (was 'false' in 7.x)`](https://github.com/jamesfredley/grails-core/blob/pr15465-issue-reproducers/grails-gsp/grails-taglib/src/test/groovy/org/grails/taglib/Pr15465ReproducerSpec.groovy#L120-L138) — verifies the new method signature exists and documents the call-site change. **Suggestion:** keep the new default (it's load-bearing for the 17 `FormTagLib` tests the author flagged), but log at DEBUG when `registerTagMetaMethods` overrides a user-declared method, so users have a fighting chance of diagnosing this when their helper "stops working." --- ### M1 — Map-parameter binding requires the literal name `attrs`; Closure-parameter binding does not [`TagMethodInvoker.toMethodArguments` lines 252-259](https://github.com/apache/grails-core/blob/7c89b78/grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/TagMethodInvoker.java#L252-L259): ```java if (Map.class.isAssignableFrom(parameterType) && "attrs".equals(parameterName)) { ... } // name-required if (Closure.class.isAssignableFrom(parameterType)) { ... } // name-agnostic ``` The two rules are inconsistent. `def myTag(Map params)` does NOT receive the attrs map (it falls through to `containsKey("params")` and rejects the overload). `def myTag(String name, Closure renderer)` DOES bind `renderer` to body, even if the user intended `renderer` to be passed as an attribute. **Reproducer tests:** - [`M1: a Map-typed parameter named anything other than 'attrs' does NOT receive the attrs map`](https://github.com/jamesfredley/grails-core/blob/pr15465-issue-reproducers/grails-gsp/grails-taglib/src/test/groovy/org/grails/taglib/Pr15465ReproducerSpec.groovy#L106-L118) — `def canonical(Map attrs)` works, `def renamed(Map params)` throws `MissingMethodException`. - [`M1b: by contrast, a Closure-typed parameter is bound to body REGARDLESS of name (asymmetric)`](https://github.com/jamesfredley/grails-core/blob/pr15465-issue-reproducers/grails-gsp/grails-taglib/src/test/groovy/org/grails/taglib/Pr15465ReproducerSpec.groovy#L120-L132) — `def closureBody(Closure body)` and `def closureRenderer(Closure renderer)` both receive the body. **Suggestion:** make both rules name-required (canonical names `attrs` / `body`), or both name-agnostic (first `Map` is attrs, first `Closure` is body). Document the chosen semantic. --- ### Summary | ID | Severity | Reproducer | Status | |---|---|---|---| | C1 | Blocking for users | `C1: …silently fails when parameter names are not preserved` | Confirmed | | H1 | High | `H1`, `H1b` | Confirmed | | H2 | Medium-High | `H2: …overrideMethods is now 'true'` | Confirmed via diff + signature assertion | | M1 | Medium | `M1`, `M1b` | Confirmed | C1 is the one I'd really like to see addressed before merge — the headline feature silently does nothing in a fresh user app, and the diagnostic gives no clue why. H1 is the design polarity question: opt-out vs opt-in for tag-method discovery. The reproducer is on a branch on my fork — feel free to pull it down or leave it where it is. -- 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]
