Use a closure to delay instantiation

[naxoc]

This PR solves a problem we've had for a long time.

Let me first describe the problem quick for context: The register_migrators() function in PluginSetup.php called get_instance() (that in turn calls the constructor in that class) on over 50 classes no matter what command was run. wp plugin list would do that for no good reason for instance. The excessive constructor calls were a tiny bit of a performance problem, but a much bigger problem with understanding the context one's command would run in. It made the code harder to grok and we risked "polluting" settings in a command with settings from a completly different command that happened to be registered first. For example, the Logger class was instantiated over 30 times.

To fix this, I created a new interface and a trait: RegisterCommandInterface and WpCliCommandTrait. The interface has two stubs: get_instance() (to just ensure singletons – nothing new there) and register_commands() that does the same as the old (and now deprecated) register_commands() in InterfaceCommand.

I added register_command_classes() in PluginSetup.php that calls the register_commands() function on the classes. Also nothing new here.

What's interesting is in the WpCliCommandTrait. The get_command_closure() command wraps the part of registration in command classes that was done with something like [$this, 'cmd_my_command_to_call'] with a closure, so it now looks like this: self::get_command_closure( 'cmd_my_command_to_call' ). See AdsMigrator or any command class in this PR for an example. The closure makes sure that the commmand class is not instantiated until the actual command is called.

My idea going forward is to use the closure for all command registration. It's actually easier than it was before. All that is needed is to use WpCliCommandTrait; and implement RegisterCommandInterface. The trait provides the singleton get_instance() so no need for that either. A nice side effect of the trait is that we can delete tons of duplicated get_instance() functions.

Make sure you have Automattic/newspack-migration-tools#12 checked out. You should read the part added to the README under Working on the NMT and this repository at the same time to allow a branch on the NMT.

How to review

This is a lot of code changed. The important things to look at are:

• WpCliCommandTrait.php
• PluginSetup.php
• newspack-custom-content-migrator.php

All other classes are refactored to use the new setup, but there is not that much changed in each.

How to test

Really nothing should have changed. This is a very boring refactor.

Before checking out the branch:

• Add this to Logger.php in this repo:

    public function __construct() {
        static $instantiations = 0;
        WP_CLI::line( 'Logger class initialized ' . ++$instantiations . ' times.' );
    }

• Run wp plugin list – you will get a lot of lines with instantiation.

After checking out the branch

• Don't forget to run composer update or composer run-script update-with-nmt-symlinked (see the README).
• Add the constructor code from above to Logger.php again
• Run wp plugin list – you will get zero lines with instantiation.

[eddiesshop]

I think my only qualm with this PR is with the name RegisterCommandInterface.php. What you think about renaming it to better match the trait that can be used with it, e.g. WpCliCommandRegistrationInterface or something like that?

[naxoc]

What you think about renaming it to better match the trait that can be used with it, e.g. WpCliCommandRegistrationInterface or something like that?

I'm not too keen on signaling like that. I'd prefer a comment or docs explaining that it is possbile to use the trait. I tried my best to loosely couple things so a class implementing RegisterCommandInterface doesn't have to use the WpCliCommandTrait trait at all if not necessary. I think most commands would benefit from just being static functions, and in that case it's not needed.

[eddiesshop]

Makes sense. Can we use something like a docblock comment @see WpCliTrait if you need to use a singleton?

[naxoc]

Can we use something like a docblock comment @see WpCliTrait if you need to use a singleton?

Sure thing! You mean in the interface dockblock right?