-
Notifications
You must be signed in to change notification settings - Fork 477
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
Simple solution to provide the environment. #1411
base: main
Are you sure you want to change the base?
Conversation
lyca
commented
Dec 10, 2020
- should work in all Spring Boot applications
- i am unsure about potential class loading issues around the static EnvironmentHolder in the StaticEnvironmentProvider (it is working in my deployments without issues so far)
- the solution provided in Property Resolving in @RequestMapping for Spring Boot #1375 for Add Support for Resolving Property Values in @RequestMapping(value) #361 seems to be overengineered
- fixes Unable to resolve place holders in @RequestMapping #1406
- will work in most Spring Boot applications - too simple, if nested contexts are used
I'm not sure I see the benefit of moving to this bean-based approach of getting the |
I currently do not see any method, that simply returns the |
Hi guys, when it will be merged? |
2e02d7a
to
856b6b9
Compare
Hi, is this ever to be merged? From reading #1328 I understood it was solved 2 years ago by @lyca and @odrotbohm stated its merged but for me its not working and can also not see those code changes in the latest source (using 2.6.9). Thanks for the clarification! |
They don't need to be in there, as the fix for #1328 is actually a better solution than what's been suggested here. Would you mind creating a ticket and leave a few details of what exactly doesn't work, ideally with an accompanying reproducer. |
Sure I created a small reproducer - thanks for reaching out and let me know if that's not the intended way of how this should be used and work. |
As lyca pointed out #1328 did not resolve the issue for spring boot apps, since ContextLoader.getCurrentWebApplicationContext() is null in this case. Therefore PropertyResolvingMappingDiscovery.resolveProperties does not resolve the properties contained in the mapping (verified on spring boot 3.1.5, spring-hateoas 2.1.2). Therefore I think this PR is justified. |
5828e78
to
e643c37
Compare
Completely correct... When will this fix be merged? |
Not sure what's the point of requesting a reproducer to be honest ....... |