Skip to content

Conversation

@candrews
Copy link
Contributor

@candrews candrews commented Jun 4, 2014

@jonatasemidio
Copy link
Contributor

Hi Craig Andrews,

Here you are comparing String with a char type (String format == 'all' or "" with '') in a java file.
I know that it works with groovy, but I'm not sure about java files.

("" == '') in java files works?

@candrews
Copy link
Contributor Author

candrews commented Jun 4, 2014

@jonatasemidio you're absolutely right. I fixed that.

@candrews
Copy link
Contributor Author

candrews commented Jun 4, 2014

Sorry, I submitted this PR while still waiting for the Grails build to run. It takes a while to download all that stuff and run it, and I didn't want to forget to submit this. Thanks again for noticing my mistake, @jonatasemidio.

@jeffscottbrown
Copy link
Contributor

I think this may be a breaking change for anyone who is rendering a file named something.all.gsp. I doubt it will effect anyone, but it could. I am not saying that is necessarily enough to squash the idea, but it is worth noting. I have not tested it but from looking at the code I think that would be a problem. Is it?

@candrews
Copy link
Contributor Author

candrews commented Jun 5, 2014

@jeffbrown I don't think so. The change I've proposed just checks the format to see if it's "all" - it doesn't look at the actual file name being render. In other words, if a controller calls "render(view:'index.all.gsp')" that will work before and after this change.

@jeffscottbrown
Copy link
Contributor

I do not think that render view: 'index.all.gsp' is a problem. Is it the case that render view: 'index' currently will render index.all.gsp if it exists and after this change it will not?

@candrews
Copy link
Contributor Author

candrews commented Jun 5, 2014

@jeffbrown that's right. But, it didn't render index.all.gsp before Grails 2.3, and as far as I can tell, *.all.gsp is not documented anywhere, so I don't think that was an intentional change.

@jeffscottbrown
Copy link
Contributor

I don't think that *.all.gsp is explicitly documented but the idea that the format can be used there is documented. I think the user guide uses show.xml.gsp as an example, but that doesn't imply that xml is the only format supported (if it were, that wouldn't be useful).

I don't think this is a giant deal, just clarifying whether or not is a breaking change in the way that I questioned.

@jeffscottbrown
Copy link
Contributor

One other question. When the format is all and the call to findPage at https://github.com/grails/grails-core/blob/50ad6ebbb3c3aa441e00a345b9503afc2f7da0b8/grails-web-gsp/src/main/groovy/org/codehaus/groovy/grails/web/pages/discovery/GrailsConventionGroovyPageLocator.java#L152 returns null, is the system going to call findPage again with the exact same argument 3 lines later?

@candrews
Copy link
Contributor Author

candrews commented Jun 5, 2014

@jeffbrown yes, it would. Doesn't seem ideal... But doesn't cause a problem either. It could be optimized I think - if you'd like, I could update this PR accordingly.

jeffscottbrown added a commit that referenced this pull request Jun 19, 2014
GRAILS-11471: Don't try to resolve "*.all.gsp"
@jeffscottbrown jeffscottbrown merged commit 1756bcc into apache:2.3.x Jun 19, 2014
jdaugherty pushed a commit that referenced this pull request Apr 17, 2025
* Update example projects

* Add empty directory `src/integration-test/groovy/`

---------

Co-authored-by: Puneet Behl <behlp@objectcomputing.com>
jdaugherty pushed a commit to jdaugherty/grails-core that referenced this pull request Jun 28, 2025
…ncies

exclude org.graalvm dependencies from the deployable war/jar
jdaugherty pushed a commit to jdaugherty/grails-core that referenced this pull request Jun 28, 2025
…-dependencies"

This reverts commit 16f50d8, reversing
changes made to 493f64d.
jdaugherty pushed a commit to jdaugherty/grails-core that referenced this pull request Jun 28, 2025
…xclude

Revert "Merge pull request apache#503 from grails/exclude-org.graalvm-dependencies
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