Auto-upgrade constants and SS_Log

[chillu]

Follow up work from silverstripe/silverstripe-upgrader#51, intended to rewrite some additional high usage APIs.

• SS_Log: https://docs.silverstripe.org/en/4/changelogs/4.0.0#psr3-logging
• FRAMEWORK_PATH and other constants: https://docs.silverstripe.org/en/4/changelogs/4.0.0#module-paths
• Debug::log calls (presuambly to Injector::inst()->get(LoggerInterface::class)->log($messsage, 'notice'))

[maxime-rainville]

I think I'll do these each of those as individual PRs. They're not really related in any way.

[maxime-rainville]

Updating error logging.

Basically, I need to find any reference to SS_Log::log and replace it with a call to Injector::inst()->get(LoggerInterface::class). After that I just need to add a use statement for Injector and LoggerInterface.

RenameClassesVisitor class visitor as some similar logic. So I can probably base my self of that class.

This part should be relatively simple.

Extra bit of complexity

• The method called on the Logger object needs to match the log level (e.g.: error, warning, info, debug, etc.)
• If we can find those methods that make multiple calls to the logger and replace and assign the logger to a $logger variable rather than always calling Injector::inst()->get(LoggerInterface::class) it would make the call more readable.
• The logic for adding use statement can probably be abstracted out RenameClassesVisitor into a trait.

[robbieaverill]

The method called on the Logger object needs to match the log level (e.g.: error, warning, info, debug, etc.)

Note that they are not like for like, as previously discussed, and as mentioned as a bug report again with #8044

[maxime-rainville]

Squashing old module dir const

There's 4 distinct places where those need to be update:

• SS files (<img src="framework/images/image.png" />, <% require css("framework/css/styles.css") %>)
• Requirements calls (Requirements::css('framework/css/styles.css');)
• Direct const usage (FRAMEWORK_DIR . '/MyFile.php', BASE_PATH . '/composer.json')
• Private static icons variables for ModelAdmin and Page types. I might need to look at the YML for that as well.

I'm thinking each one of those cases will need to be a different rule/visitor. Although, I'll probably have a centralised constant/method to give me a list of all available the PATH constant that need to be converted.

I suspect this will be more appreciably more complex than the logging upgrade.

Extra bits of complexity

• If we can support a flag to detect custom module, or allow people to specify their own, that would be pretty cool, but it might be a bridge too far.
• I suspect there's probably, some weird combination out there that combine multiple of these case together. e.g.: Requirements::css(FRAMEWORK_DIR . '/css/styles.css');

Our current doc suggest this change:

-$moduleFilePath = FRAMEWORK_DIR . '/MyFile.php';
+$moduleFilePath = ModuleLoader::getModule('silverstripe/framework')->getResource('MyFile.php')->getRelativePath();

I think it would be better to adopt a syntax like that instead:

$moduleFilePath = ModuleLoader::resourcePath('silverstripe/framework: MyFile.php');

Results will be the same and it's more readable.

[chillu]

Moving this back into Icebox, it's getting too complex for the value it provides.

[sminnee]

Added the 3rd bullet point to the ACs based on something I found.

[chillu]

This has come up in a production incident context:

I noticed they had a failing outgoing call to a IP address, which didn’t use the proxy. Investigating that found that their curl call didn’t properly throw an exception on failure (namespacing issue) and even if it did they had namespaced SS_Log incorrectly so the problem wasn’t logged

I’ve confirmed that missing classes won’t cause a fatal until that actual exec path is followed, which is particularly bad for SS_Log. It’ll create a red herring, and mask the actual error which you were trying to log/surface via logging in the first place.

[maxime-rainville]

This is a somewhat different problem. Basically you can go through the upgrade and inspect command, and come out the other end calling classes that don't exists anymore.

We could try adding some logic to go through the entire codebase of the project and check if there's classes that can not be auto-loaded and throw warnings.

You should have gotten a warning when running the inspect command because of this line in framework's .upgrade.yml. Did you?