8.x Make SiteMesh 3 the default GSP layout#15713
Merged
codeconsole merged 34 commits intoJun 17, 2026
Merged
Conversation
…s-layout Introduce a GspLayout one-of feature group so SiteMesh 3 (grails-sitemesh3) and the legacy SiteMesh 2 grails-layout are both selectable but never applied together (enforced by OneOfFeatureValidator): - GspLayout: abstract OneOfFeature parent (Category.VIEW), WEB/WEB_PLUGIN - Sitemesh3: default member; auto-applied unless another GspLayout is selected; adds grails-sitemesh3 - GrailsLayout: opt-in member; adds grails-layout (SiteMesh 2) The GspLayout features now own the layout dependency, so GrailsGsp's 'if (!isFeaturePresent(Sitemesh3)) add grails-layout' block is removed. Selecting both sitemesh3 and grails-layout now fails fast.
codeconsole
force-pushed
the
feat/sitemesh3-default-layout
branch
from
2fc6775 to
b0e0990
Compare
jdaugherty
approved these changes
Jun 3, 2026
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #15713 +/- ##
==================================================
- Coverage 48.3255% 48.2569% -0.0687%
- Complexity 15216 15236 +20
==================================================
Files 1875 1877 +2
Lines 85818 86023 +205
Branches 14969 15007 +38
==================================================
+ Hits 41472 41512 +40
- Misses 37975 38135 +160
- Partials 6371 6376 +5
🚀 New features to boost your workflow:
|
Contributor
|
Gaps to review and potentially address before SM3 becomes default
Not gaps (verified parity)Resolution order (6 steps), Minor / cosmeticCache-interval key differs: SM3 |
sbglasius
approved these changes
Jun 4, 2026
matrei
reviewed
Jun 4, 2026
matrei
left a comment
matrei
left a comment
Contributor
There was a problem hiding this comment.
There seems to be a glitch in the UI when a default feature is replaced:
jdaugherty
previously requested changes
Jun 4, 2026
jdaugherty
left a comment
jdaugherty
left a comment
Contributor
There was a problem hiding this comment.
I forgot about the test apps pointing at grails-layout by default. We'll need to update that as part of this switch and ensure no regressions.
Contributor
|
I wish github didn't scroll on tables like that in comments, but there are a number of key features that will need to be added to the sitemesh 3 plugin to match sitemesh 2's historical feature set. When the tests are all run against sitemesh 3 it will surface most of these, but the comment above should be close to comprehensive. |
…itch
Address review feedback on the SiteMesh 3 default change:
- Align the SiteMesh 3 feature title with SiteMesh 2 ("GSP SiteMesh 3
Layouts") and clean up GspLayoutSpec (single quotes, drop the
short-lived SNAPSHOT-repo assertions).
- Fix the UI glitch where replacing the default layout left both
sitemesh3 and grails-layout selected. The two decorators are now
driven by a single GspLayoutImpl option (SITEMESH3 default,
GRAILS_LAYOUT) instead of a visible feature card, mirroring the
servlet and reloading one-of groups. Both members are invisible
DefaultFeatures that apply based on the selected option, so the
default-features endpoint always resolves exactly one member.
Threads the option through Options, FeatureFilter, ApplicationController
and ContextFactory, exposes it via select-options (GspLayoutImplDTO /
GspLayoutImplSelectOptions) for the UI dropdown, and adds a --gsp-layout
CLI option. Updates BuildBuilder and GspLayoutSpec and adds
SelectOptionsControllerSpec.
sbglasius
approved these changes
Jun 5, 2026
The test apps already gate the layout dependency on the SITEMESH3_TESTING_ENABLED environment variable, but no CI lane set it, so the suite only ever exercised the legacy grails-layout. Set the flag at the workflow level in the CI and Groovy joint builds so the example apps and grails-test-suite-uber resolve grails-sitemesh3 by default. No build-script changes needed: the flag stays as-is, the SiteMesh 2 anchors keep their grails-layout coverage, and developers can still drop the flag to run against SiteMesh 2 locally.
codeconsole
force-pushed
the
feat/sitemesh3-default-layout
branch
from
f816246 to
021059d
Compare
Contributor
Author
In the filterless SiteMesh 3 pipeline the GSP capture taglib writes the full <head>/<body> markup to the response buffer and also captures the head/body into a Sitemesh3CapturedPage. When no decorator is selected (e.g. a view with no <meta name="layout"> and no matching convention or default layout), SiteMeshView writes content.getData() back. The captured page had neither renderedContent nor pageBuffer set, so getData() reconstructed only from (empty) properties and emitted an empty <html><head></head><body></body></html>. Attach the original response buffer as the captured page's rendered content in the no-merge branch so the original page is written back when nothing decorates it. Decorated (meta-layout) pages are unaffected: they take the decorate branch, which reads the head/body child properties. Verified against grails-test-examples/app1 integration tests under SITEMESH3_TESTING_ENABLED=true: ConfigTestControllerSpec, ControllerIncludesSpec, ControllerFromPluginSpec and ConditionalOnPropertyFromPluginYmlSpec now pass, with no regression to meta-layout pages (BookFunctionalSpec).
The grails-fields plugin renders embedded objects by wrapping the sub-fields in <g:applyLayout name="_fields/embedded" params="[...]">. SiteMesh 3's applyLayout built the body Content from the request-scoped Sitemesh3CapturedPage (the outer page being decorated) and ignored the tag's params, so the _fields/embedded layout's <g:layoutBody/> rendered empty and <g:pageProperty name="legend"/> / pageProperty(name:'type') had no values. The result was an empty <fieldset class="embedded "> with none of the address.* inputs. SiteMesh 2's GrailsLayoutTagLib pushed a fresh layout page and copied params to page properties, which is why the same grails-fields plugin worked under SiteMesh 2 but not SiteMesh 3. applyLayout now pushes a fresh Sitemesh3CapturedPage for the body render and restores the outer page in a finally block, wires the rendered body in via setBodyBuffer so <g:layoutBody/> works, applies each params entry as a page property for <g:pageProperty>, and emits the raw body verbatim when no decorator is resolved (matching SiteMesh 2's no-decorator fallback). Verified under SITEMESH3_TESTING_ENABLED=true: scaffolding-fields RelationshipsFunctionalSpec (embedded address fields) passes and the full scaffolding-fields integration suite is green, with no regression to meta-layout pages or the earlier undecorated-page fix.
database-cleanup, scaffolding-fields and test-phases declared grails-layout directly in addition to grails-dependencies-starter-web. starter-web already provides the layout plugin and flips it with SITEMESH3_TESTING_ENABLED, so with the flag on these apps ended up with both grails-sitemesh3 (via starter-web) and grails-layout (direct) on the classpath, and failed to boot with a jspViewResolver bean-type clash. Remove the redundant direct grails-layout dependency so each app gets exactly one layout plugin from starter-web: SiteMesh 2 when the flag is off, SiteMesh 3 when it is on. The dedicated SiteMesh 2 layout coverage remains on gsp-layout, which does not use starter-web.
A controller's "render template: 'x'" renders the GSP directly with renderView=false and must not be decorated with a layout. Under the filterless SiteMesh 3 integration the template view was still wrapped by GrailsSiteMeshView and Sitemesh3LayoutFinder picked the default layout, so partials came back wrapped in the application layout (e.g. the main layout's title instead of the partial's own). SiteMesh 2 suppressed this via GrailsLayoutSelector keyed on the renderView flag. Sitemesh3LayoutFinder.selectDecoratorPaths now returns no decorator when the current render has renderView=false and no explicit layout was requested. An explicit "render template: ..., layout: 'x'" still sets the LAYOUT_ATTRIBUTE and is honoured, and normal view renders (renderView=true) are unaffected, so decorated pages still decorate. Verified under SITEMESH3_TESTING_ENABLED=true: LayoutWithTemplateSpec passes; BookFunctionalSpec (meta-layout pages), ControllerIncludesSpec (incl. include-from-template), ConfigTestControllerSpec and scaffolding-fields RelationshipsFunctionalSpec remain green.
It was disabled when SiteMesh 3 lacked Spring Boot 4 support, which the filterless integration has since restored. This is the dedicated SiteMesh 3 layout regression app, mirroring the gsp-layout SiteMesh 2 anchor.
Three gaps surfaced by re-enabling the gsp-sitemesh3 example, which tests the layout engine itself rather than just using it: - Nested <g:applyLayout> chains lost the inner layout's title, head and body wrappers: applyLayout overwrote the captured body with the full rendered document and passed the inner chain's decorated output on as raw text. Never overwrite a captured body buffer, and parse uncaptured full-document bodies through the content processor so chained decoration keeps head/title/body, as SiteMesh 2 does. - Error-dispatched views (404/500 pages) rendered undecorated because Sitemesh3LayoutFinder guarded on the composite isRenderView(), which is false for any response status >= 300. Replace the guard with the SiteMesh 2 parity mechanism: a Sitemesh3RenderViewMutator registered as grailsRenderViewMutator that unwraps the SiteMesh view only for "render template:" partials, so error views decorate via their meta layout while partials stay undecorated. - JSP views rendered empty on Tomcat 11 because JstlView forwards and ApplicationDispatcher suspends the wrapped response after the forward, discarding everything SiteMesh writes. Render InternalResourceView inner views via include instead, and honor the JSP's meta layout. Verified: gsp-sitemesh3 integration suite 15/15, grails-sitemesh3 unit tests 32/32 (new mutator/finder/resolver specs), no regressions in the app1 layout/include/template specs or scaffolding-fields embedded fields.
Contributor
|
I re-checked the current PR head (
|
The refreshed org.sitemesh 3.3.0-SNAPSHOT defaults the Boot starter to the view-resolver integration (the servlet filter is now opt-in via sitemesh.integration=filter, and the starter registers its own disabled "sitemesh" guard bean), and SiteMeshViewResolver now switches forward-based JSP inner views to include dispatch itself, keyed on DispatchMode's container detection. Remove the plugin pieces that existed to compensate: - the NoopSitemeshFilter and its "sitemesh" FilterRegistrationBean, which suppressed the upstream filter auto-configuration - Sitemesh3EnvironmentPostProcessor and its spring.factories/imports registrations, which forced sitemesh.integration and wrap-mode defaults; Sitemesh3AutoConfiguration now uses matchIfMissing instead - the InternalResourceView alwaysInclude special case in GrailsSiteMeshViewResolver, now handled upstream before the createSiteMeshView hook; the resolver spec stubs Tomcat 11 server info to verify the inherited behavior Verified against the updated snapshot: grails-sitemesh3 unit tests and codeStyle, gsp-sitemesh3 integration suite 15/15 (including the JSP demo on Tomcat 11), app1 layout/include/template specs and scaffolding-fields embedded fields all green.
The hibernate7 examples were cloned from the hibernate5 tree before the SiteMesh toggle was inverted, so they still gated on the removed SITEMESH3_TESTING_ENABLED variable and silently defaulted to SiteMesh 2. Bring them in line with the other examples: grails-sitemesh3 by default, grails-layout via SITEMESH2_TESTING_ENABLED.
Sitemesh3CapturedPage.extractHead matched "<title" as a bare prefix, so
an element such as <titlebar> or <title-x> appearing before the real
title made the strip start at the wrong tag and delete everything up to
the real </title>, silently dropping head content. Require the character
after the prefix to terminate the tag name ('>' or whitespace) and keep
scanning otherwise. Covered by a new Sitemesh3CapturedPageSpec.
Sitemesh3LayoutFinder only consulted the configured default layout and returned nothing when unset, while SiteMesh 2's GroovyPageLayoutFinder falls back to the implicit "application" layout — so apps relying on layouts/application.gsp silently lost decoration when switching. The finder now mirrors SiteMesh 2 exactly: the configured default is used as-is, and only when none is configured is the implicit application layout tried (a configured-but-missing default does not additionally fall back). The plugin also honors SiteMesh 2's grails.views.layout.default config key when the SiteMesh 3 specific grails.sitemesh.default.layout is unset.
Both were hardcoded to grails-layout as SiteMesh 2 anchors. With the gsp-layout example serving as the dedicated SiteMesh 2 regression anchor, switch them to the standard SITEMESH2_TESTING_ENABLED toggle so the default build exercises SiteMesh 3 — notably adding the only Jetty-with-SiteMesh-3 integration coverage in the repository.
The module exists to let plain Spring Boot applications use GSP (with SiteMesh 3 layouts and JSP taglib support), so it must ship: add it to the published projects list. Re-enable the gsp-spring-boot test example so the standalone Boot + GSP combination is at least compiled and GSP-precompiled by the default build; the example currently carries no tests, so functional coverage remains a follow-up. Verified: grails-gsp-spring-boot publishToMavenLocal produces the artifact, and the re-enabled example builds cleanly.
SiteMesh 3's g:applyLayout only handled name, params and the tag body; the SiteMesh 2 implementation also selects the content to decorate via template (with model and the other g:render attributes), url, and action/controller (delegating to g:include with params), plus parse to force a fresh head/title/body parse and contentType for the SiteMesh context. Mirror those semantics: each content source produces a buffer that flows through the existing captured-page pipeline (fresh page push and restore, capture reuse or content-processor parse, params as page properties, raw emission when no decorator resolves), and the model now reaches the layout render through GrailsSiteMeshViewContext. Deliberate differences from the SiteMesh 2 code, not its docs: encoding is accepted but unused (SiteMesh 2 never read it either), url content is always parsed (as in SiteMesh 2, where no captured page exists for that path), and contentType cannot switch parsers since SiteMesh 3 uses a single configured ContentProcessor. The fresh captured page is pushed for url/include renders too, preserving the nested-render isolation. Adds six gsp-sitemesh3 end-to-end cases (template, model, url, action/controller, parse, no-decorator), GrailsSiteMeshViewContext model unit cases, and documents the previously missing action, controller and parse attributes in the applyLayout tag reference.
…lt-layout # Conflicts: # settings.gradle
codeconsole
changed the title
Contributor
|
I confirmed with forge locally that selecting sitemesh2 does what's expected: 
Otherwise, it uses sitemesh3 by default. |
jdaugherty
reviewed
Jun 17, 2026
jdaugherty
left a comment
jdaugherty
left a comment
Contributor
There was a problem hiding this comment.
The only change I would suggest is a separate workflow to run the sitesmesh2 work since legacy apps may still use it and we don't have those tests running by default. I'm ok to merge without this or address later if we make gsp changes though. Will defer to @jamesfredley on if we should do this. Assuming he approves, I approve
| @@ -76,27 +110,130 @@ class RenderSitemeshTagLib implements TagLibrary { | |||
| // causing "request is not active anymore" errors. | |||
| Closure applyLayout = { Map attrs, body -> | |||
Contributor
There was a problem hiding this comment.
To make this faster, you should use the method version instead of closure.
SiteMesh 3 is now the default GSP layout engine, so the default CI build no longer exercises the legacy SiteMesh 2 (grails-layout) path that existing applications may still depend on. Add a dedicated workflow that sets SITEMESH2_TESTING_ENABLED=true, which flips the starter, the uber unit suite and the functional test examples back onto :grails-layout, and runs the core and functional test lanes against it. The lane runs on demand (workflow_dispatch), weekly (schedule), and on push/pull_request changes under the GSP, test-suite, test-example and dependency paths where SiteMesh 2 versus 3 behavior lives. --rerun-tasks forces the tests to execute against the grails-layout classpath rather than reusing outputs cached from a SiteMesh 3 build. Addresses the reviewer request for a separate workflow covering the SiteMesh 2 work on PR apache#15713. Assisted-by: claude-code:claude-opus-4.8
Contributor
|
I re-verified my earlier SM2 parity list against the current PR head and addressed the outstanding CI item. Most items have been resolved by the commits pushed since my last review; status below. SM2 parity list - current status
SM2 CI lanePer @jdaugherty's suggestion (a separate workflow to run the SiteMesh 2 work, since legacy apps may still use it and it isn't exercised by default), I added Triggers: One caveat worth noting: GitHub only runs |
jamesfredley
approved these changes
Jun 17, 2026
✅ All tests passed ✅🏷️ Commit: d7667d5 Learn more about TestLens at testlens.app. |
codeconsole
added a commit
that referenced
this pull request
Jun 24, 2026
Convert the seven closure-based tags in RenderSitemeshTagLib (applyLayout, pageProperty, ifPageProperty, layoutTitle, layoutHead, layoutBody, content) to method handlers so GSP dispatch uses the faster reflective method-invocation path added in #15465 instead of cloning a closure on every invocation. This also clears the closure-based-tag deprecation warning the #15465 compiler check emits for this class. The conversion is behavior-preserving: GSP dispatch already passes TagOutput.EMPTY_BODY_CLOSURE (never null) as the body to two-arg tags, so the existing body checks resolve identically whether the tag is a closure field or a method. Sitemesh3LayoutTagLib is intentionally left as closures, matching its SiteMesh 2 twin GrailsLayoutTagLib: those grailsLayout-namespace, @CompileStatic capture tags driven by GrailsLayoutPreprocessor were deliberately kept as closures in #15465. Follow-up to #15713. Addresses review feedback on RenderSitemeshTagLib.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes SiteMesh 3 the default GSP layout in generated applications, mutually exclusive with the legacy SiteMesh 2
grails-layoutfeature.Introduces a
GspLayoutone-of feature group (enforced byOneOfFeatureValidator):GspLayout— abstract one-of parent (Category.VIEW, WEB/WEB_PLUGIN)Sitemesh3— default member; auto-applied unless anotherGspLayoutis selected; addsgrails-sitemesh3GrailsLayout— opt-in member; addsgrails-layout(SiteMesh 2)GrailsGspis intentionally not modified: its existingif (!isFeaturePresent(Sitemesh3))guard already skipsgrails-layoutwhen SiteMesh 3 applies, so SiteMesh 2 remains available viaGrailsLayout. Selecting bothsitemesh3andgrails-layoutnow fails fast.Depends on #15710
Split out from #15710 per review feedback ("making this the default & updating to sitemesh 3 should be separate PRs"). This branch is stacked on #15710, so until #15710 merges this PR's diff also shows the enable-SiteMesh-3 commits; it will reduce to just the make-default change once #15710 lands. Please merge #15710 first.