davydotcom commented on PR #15465:
URL: https://github.com/apache/grails-core/pull/15465#issuecomment-4409372844
Addressed the review feedback in 7c89b78. Status of every concern raised:
## Resolved
| ID | Concern | Resolution |
|----|---------|------------|
| B1 | `Error` swallowed/rewrapped in `TagMethodInvoker.invokeTagMethod` |
`Error` rethrown directly so `StackOverflowError`/`AssertionError` surface
unchanged; matches closure-path semantics |
| B2 | Closure dispatch reflection regression in `getClosureTagProperty` |
Added `ClassValue<Map<String,Field>>` cache mirroring
`INVOKABLE_METHODS_BY_NAME`; eliminates per-call `getDeclaredField` walk and
exception-driven control flow |
| M1 | `ValidationTagLib.formatValue` made private | Restored as public +
re-added to `returnObjectForTags`; safe now that overload resolution no longer
uses the magic single-attribute fallback |
| M2 + jf#6 | `null` rejected for reference params; magic single-param
fallback | `toMethodArguments` now uses `containsKey` so an absent attribute
rejects the overload, `null` binds to reference-typed parameters, primitives
still reject `null` |
| M3 | `getDeclaredMethods()` ordering non-deterministic | Same-arity
tiebreaker on signature string; stable across HotSpot/Graal/J9 |
| jf#1 | Broken doc examples in `namespaces.adoc` / `tagReturnValue.adoc` |
Cleaned up |
| jf#2 | `isTagMethodCandidate` too broad | Signature-based exclusion
catches Object/GroovyObject overrides (`toString`/`hashCode`/`equals` declared
on user class), plus Spring lifecycle names (`destroy`, `onApplicationEvent`);
new `@grails.gsp.NotATag` annotation gives explicit per-method opt-out without
forcing annotations on every helper |
| B3 (residual) | Unconditional deprecation warning | Gated behind
`-Dgrails.taglib.warnDeprecatedClosures=false`; default behavior unchanged |
## Documented (not reverted)
**jf#4** — `registerTagMetaMethods` default `overrideMethods=true`. I tried
reverting to match 8.0.x but it broke 17 `FormTagLib` tests; with method-based
tags now being real methods on the class, the metaclass dispatcher must
override for function-style invocations like `tagLib.actionSubmit(map)` to
return captured output instead of writing to `out`. Documented as an
intentional behavior change in `upgrading70x.adoc` §16.1, including the
workaround (rename or move to a different namespace).
## Verified invalid
- **jf#5** ("ThreadLocal push outside try block") — `push` is at
`GroovyPage.java:497` *inside* the `try {` at line 490; `pop()` is also
no-op-safe on empty stacks.
- **B3 (Werror claim)** — `GrailsASTUtils.warning` is just
`System.err.println`; never went through Groovy's `WarningMessage` system, so
`-Werror` doesn't apply.
## Test coverage
New focused specs:
- `org.grails.taglib.TagMethodInvokerSpec` — Error/RuntimeException
propagation, Object/GroovyObject override exclusion, Spring lifecycle
exclusion, `@NotATag`, closure-field cache with subclass shadowing,
null/missing attribute binding, deterministic overload ordering.
- `grails.gsp.taglib.compiler.TagLibArtefactTypeAstTransformationSpec` —
default warning emission and system-property suppression.
- `ValidationTagLibSpec` — `<g:formatValue>` tag-syntax and function-syntax
invocation.
`:grails-taglib:test`, `:grails-web-taglib:test`, and `:grails-gsp:test` all
green on the merged base.
## Open follow-ups (post-merge polish)
The N1–N7 cosmetic items from @codeconsole's review are intentionally
deferred — happy to file follow-up issues if useful. Most impactful would be N4
(replace `propertyMissing('attrs')`/`propertyMissing('body')` in docs with
public `currentAttrs()`/`currentBody()` helpers on the `TagLibrary` trait).
--
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]
