-
-
Notifications
You must be signed in to change notification settings - Fork 952
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't set layout if none is specified #13908
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! This resolves the render(text: '...')
problem and three more test are successful in grails-functional-tests
.
I have made some comments that I think will make the code clearer and also make it less coupled with a specific layout engine.
@@ -533,13 +533,12 @@ trait ResponseRenderer extends WebAttributes { | |||
} | |||
|
|||
private void applySiteMeshLayout(HttpServletRequest request, boolean renderView, String explicitSiteMeshLayout) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename method to setLayout
and possibly make static
?
Param renderView
is unused both before and after this change. Remove?
Rename param explicitSiteMeshLayout
to layout
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename all occurrences in this file of variable name explicitSiteMeshLayout
outside of applySiteMeshLayout()
to layoutArg
?
Also the |
Thanks to grails/grails-core#13908, 3 more tests can be re-enabled.
Instead of this… private void setLayout(HttpServletRequest request, String layout) {
if (layout == null || request.getAttribute(WebUtils.LAYOUT_ATTRIBUTE) != null) {
// layout has been set already
return
}
request.setAttribute WebUtils.LAYOUT_ATTRIBUTE, layout
} Could it just be this… private void setLayout(HttpServletRequest request, String layout) {
// if layout is provided and the layout attribute hasn’t already been set, set it
if (layout != null && request.getAttribute(WebUtils.LAYOUT_ATTRIBUTE) == null) {
request.setAttribute WebUtils.LAYOUT_ATTRIBUTE, layout
}
} |
true, I was working backwards. I guess I was trying to preserve as much of the original code as possible. |
My suggestion works against that goal. Please ignore. |
Thanks to grails/grails-core#13908, 3 more tests can be re-enabled.
Currently if no layout is specified, none is set. Since there is no concept of a none layout in Sitemesh 3, it attempts to find a layout named none.
Before 7 is final, more time will need to be spent perfecting layout resolution. There is not clear separation of concerns.
The following question needs to be addressed prior to RC1
Should layout resolution be specific to controllers? currently (and prior) this trait is applied to Controllers, RestControllers, and Interceptors. I am not sure if RestControllers or Interceptors should be so tightly coupled to SiteMesh.
Perhaps all this layout logic should be removed and done directly inside Controller.