Load plugin.yml/plugin.groovy earlier so @Conditional* annotations can be used by jdaugherty · Pull Request #15409 · apache/grails-core

jdaugherty · 2026-02-18T16:21:56Z

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

grails-core/src/main/groovy/grails/plugins/GrailsPlugin.java

bito-code-review · 2026-02-18T21:54:17Z

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 · 2026-02-18T23:08:27Z

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.

grails-core/src/main/groovy/org/grails/config/GrailsPluginEnvironmentPostProcessor.java

jdaugherty · 2026-03-01T16:37:36Z

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 · 2026-03-01T23:58:55Z

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.

…n be used

matrei · 2026-03-12T13:18:37Z

@jdaugherty Why did you choose to write new classes in Java, and not Groovy?

matrei

This was a great effort @jdaugherty, thank you!

As changes a some of the public API of core functionality, I think it would be wise to target 8.0.x.

matrei · 2026-03-12T08:57:09Z

grails-bootstrap/src/main/groovy/grails/plugins/GrailsPluginInfo.java


    /**
-     * Returns the location of the Resource that represents the plugin descriptor (the *GrailsPlugin.groovy file)
+     * Returns the location of the Resource that represents the plugin descriptor (the *GrailsPlugin.groovy or grails-plugin.yml file)


Isn't the grails-plugin.yml created from the *GrailsPlugin.groovy file so this is essentially the same thing.

There are 2 code paths - binary plugins load from grails-plugin.yml & source based plugins load from GrailsPlugin. Source based plugins that are loaded dynamically do not have a grails-plugin.yml. I think this really exists today for tests only. I was trying to call out these 2 code paths in this comment.

grails-core/src/main/groovy/grails/boot/config/GrailsEnvironmentPostProcessor.java

grails-core/src/main/groovy/org/apache/grails/core/plugins/GrailsPluginDiscovery.java

...ng-support-core/src/main/resources/META-INF/services/grails.plugins.GrailsPluginConfigFilter

grails-core/src/main/groovy/org/apache/grails/core/GrailsBootstrapRegistryInitializer.java

grails-core/src/main/groovy/grails/plugins/GrailsPluginSorter.java

matrei · 2026-03-12T16:25:12Z

grails-core/src/main/groovy/grails/plugins/GrailsPlugin.java

+     * @return the plugin's property source from the environment, or {@code null} if the
+     *         GrailsApplication main context is not yet available or no matching property source exists
+     */
+    @Deprecated(forRemoval = true)


matrei · 2026-03-12T16:28:18Z

grails-core/src/main/groovy/grails/boot/config/GrailsEnvironmentPostProcessor.java

Why Java (general question for all new classes)?

The classes in this area have historically been java. Looking through the commit history, there have been numerous performance problems in this area and conversion to java was one of the "fixes". I did not want to introduce a new pattern as I was only trying to abstract the existing logic. I think if we introduce a hard contract, we could change this with some significant refactoring.

jdaugherty · 2026-03-12T17:53:58Z

This was a great effort @jdaugherty, thank you!

As changes a some of the public API of core functionality, I think it would be wise to target 8.0.x.

The problem I have with 8.0 is:

this works with all of the plugins in my current application with no breakage (that I can tell).
a core piece of Spring 3.x is broken currently without this change
there is a precedent of us already committing breaking changes to 7.1.
we can do a milestone to see if there's impact and then revert if there is?

…document it

jamesfredley · 2026-03-13T01:36:47Z

A potential start on backward-compatibility bridge shims for 7.1.x:
jdaugherty#5

matrei · 2026-03-13T08:08:27Z

The problem I have with 8.0 is:

This is excellent, but is it relevant?

1. this works with all of the plugins in my current application with no breakage (that I can tell).

@ConditionalOnProperty was introduced in Spring Boot 1.1

2. a core piece of Spring 3.x is broken currently without this change

Yeah, but not in this way, and that was a hard call.

3. there is a precedent of us already committing breaking changes to 7.1.

Unfortunately, not many projects try out milestones.

4. we can do a milestone to see if there's impact and then revert if there is?

If we can get the API backwards compatible, I have no problem including this in 7.1. I see that @jamesfredley already started this work in jdaugherty#5.

jamesfredley · 2026-03-18T22:26:56Z

I plan to approve this once the backward-compatibility bridge shims are in place for 7.1.x. Thank you @jdaugherty

github-project-automation bot added this to Apache Grails Feb 18, 2026

jdaugherty force-pushed the conditionalPluginConfiguration branch from 1232204 to e109334 Compare February 18, 2026 16:53

jdaugherty marked this pull request as draft February 18, 2026 16:57

jdaugherty force-pushed the conditionalPluginConfiguration branch from 46f5e8f to 9f81969 Compare February 18, 2026 17:17

sbglasius approved these changes Feb 18, 2026

View reviewed changes

grails-core/src/main/groovy/grails/plugins/GrailsPlugin.java Show resolved Hide resolved

jdaugherty mentioned this pull request Feb 19, 2026

docs: add Configuration Reference to Quick Reference section #15407

Closed

jamesfredley approved these changes Feb 20, 2026

View reviewed changes

jdaugherty force-pushed the conditionalPluginConfiguration branch from 462035b to ee32e61 Compare February 28, 2026 02:42

jdaugherty changed the base branch from 7.0.x to 7.1.x March 1, 2026 05:35

jdaugherty force-pushed the conditionalPluginConfiguration branch 2 times, most recently from 406aa4a to d7175cf Compare March 2, 2026 00:16

jdaugherty added 5 commits March 1, 2026 21:07

Load plugin.yml/plugin.groovy earlier so @conditional* annotations ca…

cdb9758

…n be used

Add separate documentation for 7.1

63d499c

Split the load logic from the initialization logic.

09cbe89

Update test logic for plugin discovery vs load split

0a9efaf

Remove duplicate method implementation

13ec1b0

jdaugherty force-pushed the conditionalPluginConfiguration branch from d7175cf to 13ec1b0 Compare March 3, 2026 20:00

jdaugherty added 9 commits March 3, 2026 20:41

Fix unit test loading & formatting

a807570

Fix styling

a93ab72

Licensing & javadoc cleanup

c5d0fa3

Update test for splitting plugin discovery and load logic

9a18bcc

Ensure configuration is loaded in the order the plugins would load

7ef4941

Fix comparison on grailsVersion vs pluginVersion

8b29bb1

Simulate the bootstrap process to fix test

5f46c00

Update test to correctly simulate the bootstrap registry process

c116933

Update tests to properly mock the GrailsPluginDiscovery process

cafd204

jdaugherty added 3 commits March 12, 2026 09:10

test: update tests to reflect their purpose & expand test coverage

a03b01f

feedback: update toString method from rename

e61414a

feedback: update javadoc for class file renames

7a44086

matrei reviewed Mar 12, 2026

View reviewed changes

jdaugherty added 10 commits March 12, 2026 13:08

cleanup: remove unused file

f39ddb5

feedback: use full path in javadoc @link

c2eefb4

feedback: remove unused method

79f08a8

feedback: groovy property getter

4c6c302

style: remove duplicate empty line

77760f7

style: use def where possible

f9a44e0

feedback: update javadoc to match parameter name

57926f8

feedback: groovy property setters

3132a34

feedback: use var where possible

7a89fa4

feedback: use groovy property

96d2897

jdaugherty force-pushed the conditionalPluginConfiguration branch from a7cd577 to c8e65a8 Compare March 12, 2026 17:32

feedback: remove explicit types

11cbf53

jdaugherty force-pushed the conditionalPluginConfiguration branch from c8e65a8 to 11cbf53 Compare March 12, 2026 17:34

jdaugherty added 7 commits March 12, 2026 13:34

feedback: remove unused result

6c66db1

feedback: remove explicit type

56e4d77

feedback: remove unnecessary cast

6918ab0

feedback: def & groovy property syntax

5a912fb

feedback: constructor cleanup

677beb1

feedback: reword purpose

e1ac741

feedback: styling cleanup

fec18c3

refactor: introduce a common interface for the discovery mechanism & …

20d2a8d

…document it

Uh oh!

Conversation

jdaugherty commented Feb 18, 2026

Uh oh!

Uh oh!

bito-code-review bot commented Feb 18, 2026

Uh oh!

jamesfredley commented Feb 18, 2026

Uh oh!

jamesfredley left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jdaugherty commented Mar 1, 2026

Uh oh!

jdaugherty commented Mar 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matrei commented Mar 12, 2026

Uh oh!

matrei left a comment

Choose a reason for hiding this comment

Uh oh!

matrei Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

jdaugherty Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matrei Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

matrei Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

jdaugherty Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

jdaugherty commented Mar 12, 2026

Uh oh!

jamesfredley commented Mar 13, 2026

Uh oh!

matrei commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesfredley commented Mar 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jamesfredley left a comment •

edited

Loading

jdaugherty commented Mar 1, 2026 •

edited

Loading

matrei commented Mar 13, 2026 •

edited

Loading