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

BootStrap init closure servletContext cleanup #611

Merged
merged 5 commits into from
Feb 19, 2025

Conversation

gsartori
Copy link
Contributor

Copy link
Contributor

@jdaugherty jdaugherty left a comment

Choose a reason for hiding this comment

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

I am looking at the CI issue

@matrei
Copy link
Contributor

matrei commented Feb 19, 2025

@gsartori I know I'm nagging about this, but why are we injecting an object that is not used?
From our previous discussions around the BootStrap class I have understood that your argument is that it has an educational value on the possibility to do dependency injection in the BootStrap class.
I can buy that argument for the starter app templates, but this is not a starter app template.

@gsartori
Copy link
Contributor Author

gsartori commented Feb 19, 2025

The pattern I've applied while updating the relevant projects was to consider empty BootStrap files as "files from the app template" as they were plain files from the (old) template. I agree with you though, in this context there is an implementation, we can remove the injected proeprty (I went through all the projects in a "lawnmower" mode :)

@jdaugherty
Copy link
Contributor

@gsartori Since we have it as a test in the other cases, can we remove it in this case per @matrei comment? I've merged the other PRs.

@gsartori
Copy link
Contributor Author

gsartori commented Feb 19, 2025

@jdaugherty done

@jdaugherty jdaugherty merged commit 5f13558 into grails:4.0.x Feb 19, 2025
7 checks passed
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