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

GRAILS-8978: Respect grails.views.gsp.htmlcodec configuration #548

Merged
merged 1 commit into from
Jan 13, 2015

Conversation

candrews
Copy link
Contributor

@candrews candrews commented Nov 6, 2014

Commit 5037e65 intended for the grails.views.gsp.htmlcodec setting to take effect. This commit fixes the implementation so that setting is respected.

https://jira.grails.org/browse/GRAILS-8978

@candrews candrews force-pushed the patch-18 branch 3 times, most recently from 199124f to c1a2857 Compare November 6, 2014 04:37
public Encoder getEncoder() {
if(encoder == null){
GrailsApplication grailsApplication = grails.util.Holders.getGrailsApplication();
Copy link
Member

Choose a reason for hiding this comment

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

what was the motivation in using the static holder version rather than GrailsApplicationAware?

@candrews
Copy link
Contributor Author

candrews commented Nov 6, 2014

@graemerocher the setGrailsApplication method is never called. It appears that GrailsApplicationAware doesn't work for this case (I'm not sure if that's intentional or not, but I did confirm that this codec is the only one that tries to use GrailsApplicationAware).

@osscontributor
Copy link
Member

is HTMLCodec being subjected to DI? I think GrailsApplicationAware would cause the property to be auto initialized if the thing were a bean in the context. I don't think that is the case, or is it?

@candrews
Copy link
Contributor Author

candrews commented Nov 6, 2014

When I debugged through it, it didn't appear to be a bean. I suspect that it's not and not intended to be one which explains why GrailsApplicationAware/setGrailsApplication isn't executed.

In that case, I think either some changes need to be made making it a bean or an approach such as the one in this PR needs to be used.

@graemerocher
Copy link
Member

The correct fix would be get it to honour GrailsApplicationAware, we try to avoid the use of the statics going forward

@osscontributor
Copy link
Member

DefaultCodecLookup is already a bean in the application context and it is already GrailsApplicationAware. It might end up being really simple to add code there which initializes the grailsApplication property in the codecs.

@candrews
Copy link
Contributor Author

DefaultCodecLookup tries to perform injection on the codec class in DefaultCodecLookup.autowireCodecBean. However, grailsApplication is null, so it never actually does anything.
DefaultCodecLookup.grailsApplication comes from AbstractGrailsClass (DefaultCodecLookup extends AbstractGrailsClass). It looks like AbstractGrailsClass.setGrailsApplication(GrailsApplication) should be called, but is called too late.
The problem is in DefaultGrailsApplication.addArtefact

GrailsClass artefactGrailsClass = handler.newArtefactClass(artefactClass);
handler.newArtefactClass(artefactClass) creates the codec instance, but it doesn't have the grailsApplication until the next line, which is too late.

@candrews candrews force-pushed the patch-18 branch 10 times, most recently from c2cccc1 to 5ef9acc Compare November 18, 2014 01:52
@candrews
Copy link
Contributor Author

All set - IMHO, this PR is ready for merging.

I created a simple Grails app to test that the grails.views.gsp.htmlcodec setting takes effect, and it does.

The PR includes updates to the tests to test this functionality.

@osscontributor
Copy link
Member

Thanks Craig. We will take a look.

Respect grails.views.gsp.htmlcodec configuration

Commit 5037e65 intended for the grails.views.gsp.htmlcodec setting to take effect. This commit fixes the implementation so that setting is respected.

Issue: GRAILS-8978
@candrews
Copy link
Contributor Author

Polite prodding

It would be super cool to get this merged - let me know if there's anything I can do to help.

graemerocher added a commit that referenced this pull request Jan 13, 2015
GRAILS-8978: Respect grails.views.gsp.htmlcodec configuration
@graemerocher graemerocher merged commit 98856e0 into grails:2.4.x Jan 13, 2015
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.

3 participants