Allow objects to be instantiated without hooks

[pdclark]

Recommended by @GaryJones.

afragen/git-updater#2 (comment)

[GaryJones]

Constructors should be simple - assign any injected dependencies, and that's it. If you want an object to do more than that, then call a method to do so. This generally makes the code more self-documenting, and unit testable, as the act of instantiating an object doesn't also then have a load of side-effects.

https://github.com/brainstormmedia/git-plugin-updates/blob/master/includes/class-updater.php#L40

That constructor:

• pulls in a global
• defines an unfilterable array of default values
• merges that array with the incoming data (and that $args variable name is awful - $plugin_data or $plugin_headers is far more revealing) with a WP function.
• makes a call to identify repo information from some of those headers
• hooks in to filter functions, with a WP function.

The use of WP functions in the constructor means this class can't even be instantiated outside of a WP environment, so non-WP unit testing to check the basics is out of the question.

Here's what it should look like:

protected $plugin_headers;
...
public function __construct( $plugin_headers ) {
    $this->plugin_headers = $plugin_headers;
}

That's it. If the controller wants to pass in the plugin headers as an object, it can do. All of the tidying up of those raw plugin headers, assignment to properties and hooking in of filters can be handled in one or more other methods (e.g. run() ), and that method called when the class is instantiated. That means it's possible to instantiate the class into an object without running that tidying up, perhaps needed when unit testing the tidying up methods themselves.

public function get_plugin_updater_object( $plugin_headers ) {
    if ( GPU_Updater_Github::updates_this_plugin( $plugin_headers ) ) {
        $updater = new GPU_Updater_Github( $plugin_headers );
    }

    if ( GPU_Updater_Bitbucket::updates_this_plugin( $args ) ) {
        $updater = new GPU_Updater_Bitbucket( $args );
    }

    if ( ! isset( $updater ) {
        return false;
    }

    $updater->run();
    return $updater;
}

[pdclark]

I think I understand the value of being able to use a non-WP unit testing framework (simplicity?). At the same time, I do like objects to be able to operate correctly without requiring additional method calls after instantiation. What do you think about this format?

protected $plugin_headers;
...
public function __construct( $plugin_headers, $initialize_hooks = true ) {
    if ( $initialize_hooks ) {
         // or setup, or run, or whatever... Just want it named as closely to what it does as possible
        $this->initialize_hooks();
    }
}

Then unit tests could call

new Class( $plugin_headers, false );

[GaryJones]

I do like objects to be able to operate correctly without requiring additional method calls after instantiation.

They will operate - when you tell them how to operate - and that's by calling the method that should do the operation.

You don't bake a cake and expect it to come out of the oven already chopped into slices (even though that's what is what you're going to want most of the time).

$dessert = new Cake();
$dessert->slice();
return $dessert;

That's still preparing the cake to be eaten, just the same as calling a run() method which facilitates merging the plugin headers, assigning properties and adding in hooked functions (aka stuff).

If I wanted to ice the cake before slicing, I could do that, because the slice action is not in the constructor. Similarly, one might want to do something else with a BitBucket updater object, that isn't done for the GitHub updater project, before doing the stuff, but if it's all in the constructor, then that's not possible without creating a constructor in the BitBucket object etc.

Creating a basic object, and doing anything else more with it, are two completely different things, so should be in different methods.

In terms of your suggestion, of using a new parameter - you've just made the code harder to read. The WP Coding Standards say about not using boolean params, because someone will have to go and lookup what are we saying true or false to.

$dessert = new Cake( true );

// versus

$dessert = new Cake();
$dessert->slice();

The former is nowhere near as self-documenting as the latter in that example.

Lastly, removing or deprecating arguments later on is a pain - whereas methods can just act as wrappers if necessary.

[pdclark]

I understand the concept and have seen the pattern before. Much of what you described is personal preference; how you like constructors to behave. There's nothing wrong with that — but I have a preference for simplifying instantiation as deep as your preference for explicit configuration. (And I think we can both agree classes like WP_Theme aren't a good example — jeez, its constructor is 121 lines!!!

I think we'll find common ground quickly by focusing on the useful differences.

Unit Testing

One great reason you pointed out is unit testing. Methods certainly need to be small, testable units.

It might seem a little bit odd to use a non-WordPress unit testing framework for a WordPress plugin... but when I think of setting up the full WP unit testing framework... the word "hassle" does come to mind.

Do you have another testing methodology in mind? Raw PHPUnit? Something else?

Generic use

If someone's using this class out of WordPress... well... It's already in some other project. I'm fine with them having to sub-class or modify.

Convenience

Because using the classes within WordPress (and within this plugin) are the most common use-case, I'd still like instantiation to be as convenient and succinct as possible. When I call new DateTime( '2000-01-01' ) in PHP or new Array(); in JavaScript, I don't have to call a further run() method to get it to serve its intended use -- setup logic is contained within the constructor.

I think it follows that since the intended use of these classes is to hook into WordPress, it's appropriate for the constructor to set up hooks and initial values.

Is there any case besides unit testing where this class may need to be instantiated in its raw form?

If total generic flexibility is needed for something, I'd compromise to:

protected $plugin_headers;
...
public function __construct( $plugin_headers ) {
    $this->plugin_headers = $plugin_headers;

    if ( !isset( $plugin_headers['disable_init'] ) ) {
        $this->init();
    }
}

// Logic contained in class, so deprecation of arguments no longer an issue.
static public function get_raw_instance( $plugin_headers ) { // or construct_without_init 
   $plugin_headers['disable_init'] = true;
}

public function init() {
    // hooks
}

Or, if we're really only talking about unit testing:

protected $plugin_headers;
...
public function __construct( $plugin_headers ) {
    $this->plugin_headers = $plugin_headers;

    if ( function_exists( 'add_action' ) ) {
        $this->init();
    }
}

public function init() {
    // hooks
}