Skip to content
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

Decouple grails-plugin-controllers from Sitemesh #13624

Merged
merged 1 commit into from
Sep 7, 2024

Conversation

codeconsole
Copy link
Contributor

@codeconsole codeconsole commented Sep 6, 2024

Fixes #13623 - These changes make controllers independent of Sitemesh

Moved these attributes from ResponseRenderer.groovy

    static final String LAYOUT_ATTRIBUTE = "org.grails.layout.name";
    static final String NONE_LAYOUT = "_none_";

to ControllersGrailsPlugin.groovy

Any recommendations for a different location?

Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about creating a org.grails.plugins.web.controllers.SitemeshConstants class to encapsulate these values. This approach makes their context more explicit when used throughout the codebase.

@codeconsole
Copy link
Contributor Author

How about creating a org.grails.plugins.web.controllers.SitemeshConstants class to encapsulate these values. This approach makes their context more explicit when used throughout the codebase.

It’s only 2 properties. I was hoping for a more general location outside of the controllers plugin that has definitions for other things. If it is going to live under controllers, it can stay in the plugin definition class

@jamesfredley
Copy link
Contributor

I think it's OK in the plugin controller. There were not many great locations outside of the controller plugin. This was about the best I saw looking through 30-40 with constants.

public static final String DEFAULT_ENCODING = "UTF-8";
private static final String CHARSET_ATTRIBUTE = ";charset=";
private static final Pattern CHARSET_IN_CONTENT_TYPE_REGEXP = Pattern.compile(";\\s*charset\\s*=", Pattern.CASE_INSENSITIVE);

@codeconsole
Copy link
Contributor Author

I think it's OK in the plugin controller. There were not many great locations outside of the controller plugin. This was about the best I saw looking through 30-40 with constants.

public static final String DEFAULT_ENCODING = "UTF-8";
private static final String CHARSET_ATTRIBUTE = ";charset=";
private static final Pattern CHARSET_IN_CONTENT_TYPE_REGEXP = Pattern.compile(";\\s*charset\\s*=", Pattern.CASE_INSENSITIVE);

looks like those constants are only used in that class.

@codeconsole
Copy link
Contributor Author

Since grails-web-sitemesh this is causing issues further down the line. I am going to merge it. We can always move it later.

@codeconsole codeconsole merged commit b92629e into grails:7.0.x Sep 7, 2024
11 checks passed
@matrei
Copy link
Contributor

matrei commented Sep 7, 2024

Thats fine, I just wonder if we then could name them SITEMESH_LAYOUT_ATTRIBUTE and SITEMESH_LAYOUT_NONE?

@matrei
Copy link
Contributor

matrei commented Sep 7, 2024

The org.grails.plugins.web.controllers.metaclass.RenderDynamicMethods class looks like a candidate to hold these values.

@codeconsole
Copy link
Contributor Author

RenderDynamicMethods

Are all those constants related?

@codeconsole
Copy link
Contributor Author

Thats fine, I just wonder if we then could name them SITEMESH_LAYOUT_ATTRIBUTE and SITEMESH_LAYOUT_NONE?

yeah, that would make sense after we confirm final location for them.

@codeconsole
Copy link
Contributor Author

ok, so this rabbit hole just got a little deeper. It looks like this variable was also defined in another location. I found the new home. WebUtils.java

@codeconsole
Copy link
Contributor Author

codeconsole commented Sep 7, 2024

#13626

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grails 7: remove org.grails:grails-web-sitemesh from grails-plugin-controllers
3 participants