Load plugin.yml/plugin.groovy earlier so @Conditional* annotations can be used

[jdaugherty]

Fixes #15408

Assisted-by: OpenCode <opencode@opencode.ai>
Assisted-by: Claude <claude@anthropic.com>

[sbglasius]

I think that if it's deprecated it should say why.

[jdaugherty]

I was proposing this be removed because I don't see a usage for it - you can fetch the configuration but when would it be valid to look at the shipped config? btw, i had marked this as draft to see what @matrei thinks of this. This is a rather large change for 7.0.x so we may want to do some further testing.

[bito-code-review]

You're right—deprecation annotations should include a reason for clarity. For this method, the reason could be that plugin property sources are now handled early by GrailsPluginEnvironmentPostProcessor.

[jamesfredley]

If this does not require changes in end Grails apps I am OK with this going into 7.0.x, if we think the risk is too high, then 7.1.x is also OK.

[jamesfredley]

Looks solid. A few minor items to take a look at.

The GrailsPluginConfigurationClass test removed the "plugin with both yml and groovy throws exception" test. That validation still exists in GrailsPluginEnvironmentPostProcessor.readPluginConfiguration() (line 293), but there's no unit test covering it in the new code. The GrailsPluginEnvironmentPostProcessorSpec should add a test for this.

[jamesfredley]

calls pluginClass.getDeclaredConstructor().newInstance() to read loadAfter/loadBefore/dependsOn. At this point in the Spring Boot lifecycle, there's no GrailsApplication, no application context, no plugin manager. If any plugin's no-arg constructor has side effects or requires dependencies, this could fail.

The code handles this gracefully (catches exceptions, continues with default ordering), but plugins with complex constructors will silently get default ordering rather than their declared ordering. This could subtly change the order configs are loaded compared to the existing behavior.

[jdaugherty]

I'd argue at this point they shoudln't be using Holders either. These dependencies would have to be autowired and that would happen after the constructor creation, so i view this as low risk.

[jdaugherty]

This change is incomplete since the plugin configuration will be loaded potentially when the plugin is disabled. We may be able to post process already loaded configuration and remove them, but I'm not sure without further research.

Here's the loading order in Spring:

• SpringApplication.run(...)
• Prepare SpringApplicationRunListeners
• Prepare Environment
Create/attach ConfigurableEnvironment
Invoke all EnvironmentPostProcessors
• Create ApplicationContext
• Prepare context
• Refresh context (bean creation begins)

The problem is the plugin manager has different discovery mechanisms (but very similar). Worst, one of those mechanisms appears to require the application context, which won't exist when we need to load configuration. I think the need for the application context (or rather the parent application context) isn't actually used though.

I'm doing further research on this to see how best to unify the plugin manager mechanisms so we can share the logic in both places.

[jdaugherty]

I've been working on this and will likely push a significant update in the next day or so. The core issue is the plugin filtering needs shared between the environment & the application post processors. The problem: the plugin manager is what currently does this filtering & it's a bean. The environment post processor can't use a bean b/c the context doesn't exist yet. The only practical place the filtering is used is when in unit tests it filters the plugins. I think the right solution is either moving the filter further out & sharing it between the plugin manager / EPP, or just moving the plugin manager away from being a bean completely.