[wp-trac] [WordPress Trac] #37699: Death to Globals Episode #1: A Registry, A Pattern

[WordPress Trac]

#37699: Death to Globals Episode #1: A Registry, A Pattern
----------------------------+------------------
 Reporter:  wonderboymusic  |       Owner:
     Type:  enhancement     |      Status:  new
 Priority:  normal          |   Milestone:  4.7
Component:  General         |     Version:
 Severity:  normal          |  Resolution:
 Keywords:                  |     Focuses:
----------------------------+------------------

Comment (by schlessera):

 Hell yes, it would be awesome to get rid of the globals.

 I'm fully aware that this is just a very rough first pass to get the
 discussions started, so I won't delve too much into technical details. I'd
 like to share some early observations, though, based on what you posted so
 far.

 '''1. Patch is huge'''

 The patch is very unwieldy when the goal is to discuss basic concepts
 first. I think it would be more practical to limit patches on the actual
 "Registry", and one or two use cases until the general direction is clear.

 '''2. Existing pattern'''

 What you call "Registry" here is commonly called a "Service Locator",
 which is a known pattern. The Service Locator is responsible for letting
 major subsystems interact with each other, by providing instances to each
 subsystem when requested.

 '''3. Still global'''

 The way you've implemented this Registry/Service Locator here means that
 you've replaced one global with another global. When we start to put more
 subsystems into that Service Locator, the number of globals will of course
 reduce, but it cannot be completely brought down this way. A reference to
 `WP::get()` means that the method `get()` is called on the static
 (=global) `WP` "instance". I put "instance" in brackets here, because it
 has not technically been instantiated (through the `new` keyword), but it
 has state, and this state is made globally accessible.

 '''4. Using WP as object'''

 Relying on the `WP` class name as the Service Locator is not ideal, as a
 future version of WordPress (PHP 5.3+) would most likely have `WP` as the
 root namespace. That future version would probably offer something like
 `WP\Service::get( 'wpdb' );`, so something like `WP_Service::get( 'wpdb'
 );` would be preferable. Keeping this as future-proof and flexible as
 possible should be a priority.

 '''5. Dependency injection'''

 WordPress is mostly procedural, but the places in the code where we do
 already have classes should try to start using dependency injection.
 As an example, consider the `WP_Comment_Query` class, for which you
 proposed something like the following:

 {{{#!php
 class WP_Comment_Query {

    protected $dbh;

    public function __construct( $query = '' ) {
       $this->dbh = WP::get( 'wpdb' );
       // [...]
    }
 }
 }}}

 There's a missed opportunity there to start using dependency injection to
 make it easier to do unit tests. Even worse: right now, with the globals,
 the unit tests can still set the global to a mock DB. With the above code,
 there's no proper way of injecting a mock DB for testing anymore.

 So, while we cannot yet have a proper injector decide what to inject in
 what context, we can at least let the constructor take an injected
 dependency, and provide a "Poka-yoke" for as long as we are not able to do
 real injection. This would look something like this:

 {{{#!php
 class WP_Comment_Query {

    protected $dbh;

    public function __construct( $query = '', $wpdb = null ) {
       $this->dbh = null !== $wpdb ? $wpdb : WP_service::get( 'wpdb' );
       // [...]
    }
 }
 }}}

 Improvements would be using the short-form ternary (PHP 5.3+) and
 typehinting against an interface. However, please, '''do not typehint
 against the WPDB class''', this defeats the whole point. This would just
 very tightly couple the class to the exact WPDB implementation, and any
 dependency injection would just be of esthetic nature.

 With the above constructor, the injected dependency is optional, so
 existing code will still work, but the unit tests can now inject a mocked
 WPDB instance into the constructor for real unit tests.

 '''6. What about the interface'''

 As mentioned under 5., typehinting against an interface would be a clear
 improvement. Let's imagine we had a `WPDB_Interface` and examine what
 would happen if we use it together with the idea of a Service Locator.

 First of all, any code making DB operations would need to be coded against
 that interface, not against the actual implementation. For code that does
 not using any typehinting or instance-checking, this should not make a
 difference.

 For our example code above, we would now have something like this:

 {{{#!php
 class WP_Comment_Query {

    /** @var WPDB_Interface */
    protected $dbh;

    public function __construct( $query = '', WPDB_Interface $wpdb = null )
 {
       $this->dbh = null !== $wpdb ? $wpdb : WP_Service::get(
 WPDB_Interface );
       // [...]
    }
 }
 }}}

 We have two changes here:
 A. The constructor argument is type-hinted. This means that, would we
 instantiate the `WP_Comment_Query` through a real Injector, the Injector
 could by itself figure out that it needs to pass the instance of the
 current DB handler into that constructor. So in that (wishful thinking)
 scenario, neither the `WP_Comment_Query` class, nor its surrounding code
 would need to know anything about the DB stack at all. `WP_Comment_Query`
 needs something that behaves like a WPDB, and it can just assume that when
 it gets instantiated, what it needs is automagically available within its
 constructor.
 B. The poka-yoke fallback that uses the Service Locator does not request
 an arbitrary string identifier, it requests an object that implements the
 interface it needs. The Service Locator, which should know more about the
 current environment than the `WP_Comment_Query` class, will then decide
 what exact object to pass around. Some objects, like the WPDB here, are
 globally shared, so everyone gets a reference to the same instance. Others
 might be freshly instantiated for each request, like a `WP_Query`. The
 Service Locator will deal with this behind the scenes, the ones requesting
 such interfaces should not need to care.

 Hope this all makes sense, and can't wait to see where this ticket will
 take us... :)

--
Ticket URL: <https://core.trac.wordpress.org/ticket/37699#comment:1>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform