Copilot commented on code in PR #16011:
URL: https://github.com/apache/grails-core/pull/16011#discussion_r3610389201
##########
grails-core/src/cli/groovy/org/apache/grails/core/cli/ApplicationContextCommandRegistry.groovy:
##########
@@ -46,6 +53,37 @@ class ApplicationContextCommandRegistry {
commands[cmd.name] = cmd
}
}
+
+ loadLegacyCommands(ApplicationContextCommandRegistry.classLoader)
+ loadLegacyCommands(Thread.currentThread().contextClassLoader)
Review Comment:
`loadLegacyCommands` is invoked for both
`ApplicationContextCommandRegistry.classLoader` and the thread context
classloader unconditionally. If those are the same (or both can see the same
legacy command), this can instantiate the same legacy command twice
(constructors may have side effects) even though the second instance will be
discarded due to the name collision check. Consider only calling the second
load when the classloader differs to avoid duplicate instantiation.
##########
grails-core/src/cli/groovy/org/apache/grails/core/cli/ApplicationContextCommandRegistry.groovy:
##########
@@ -46,6 +53,37 @@ class ApplicationContextCommandRegistry {
commands[cmd.name] = cmd
}
}
+
+ loadLegacyCommands(ApplicationContextCommandRegistry.classLoader)
+ loadLegacyCommands(Thread.currentThread().contextClassLoader)
+ }
+
+ @SuppressWarnings('deprecation')
+ private void loadLegacyCommands(ClassLoader classLoader) {
+ // Instantiate each legacy command in isolation: a single stale Grails
7 command whose
+ // no-arg constructor (or getName()) throws under Grails 8 must be
skipped with a warning,
+ // never abort the whole registry and take valid legacy and
new-contract commands down with it.
+ List<Class<grails.dev.commands.ApplicationCommand>> legacyClasses =
GrailsFactoriesLoader.loadFactoryClasses(
+ grails.dev.commands.ApplicationCommand, classLoader,
FactoriesLoaderSupport.FACTORIES_RESOURCE_LOCATION)
+ for (Class<grails.dev.commands.ApplicationCommand> legacyClass :
legacyClasses) {
+ try {
+ grails.dev.commands.ApplicationCommand legacyCommand =
legacyClass.getDeclaredConstructor().newInstance()
+ ApplicationCommand command = new
LegacyApplicationCommandAdapter(legacyCommand)
+ String name = command.name
+ if (commands.containsKey(name)) {
+ continue
+ }
+ commands[name] = command
+ if (!legacyCommandWarningLogged) {
+ LOG.warn("Command '{}' from a Grails 7 plugin was loaded
through the deprecated grails.dev.commands compatibility layer. Ask the plugin
author to migrate to the org.apache.grails.core.cli command API and publish a
-cli companion artifact; this compatibility path will be removed in a future
major release.", name)
+ legacyCommandWarningLogged = true
+ }
+ }
+ catch (Throwable e) {
+ LOG.warn("Failed to load a Grails 7 legacy command from class
'{}' through the deprecated grails.dev.commands compatibility layer; skipping
it. Cause: {}",
+ legacyClass?.name, e.message)
+ }
Review Comment:
When a legacy command fails to load, the warning log omits the exception
itself, so there’s no stack trace to diagnose the root cause (only
`e.message`). Passing the throwable as the final argument lets SLF4J include
the stack trace while keeping the warning message the same.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
