10up / wp-scaffold

10up WordPress project scaffold.
MIT License
184 stars 47 forks source link

PHP Design Patterns #4

Open tlovett1 opened 3 years ago

tlovett1 commented 3 years ago

Currently, the scaffold is very light on PHP structure. I think we want to move to more class based approaches. It would be great to get some code in place to start people in the right direction.

nicholasio commented 3 years ago

IMO if we're thinking of giving more structure to the PHP side of things and potentially introducing helper classes and utilities it would make sense to make anything reusable in a composer PHP package.

darylldoyle commented 2 years ago

@tlovett1 I've been thinking about this recently; the way we do this in most projects I've seen so far is the same as this PR: https://github.com/10up/wp-scaffold/pull/13.

Whilst most engineers at 10up will be used to this; I wonder if now is an opportune time to re-think the structure.

For example, one of the things I dislike about the current method is that we use Plugin.php to initialise all of our classes. This seems like an unnecessary step since we're using a PSR-4 autoloader.

Instead, we could have an abstract class used by classes to initialise them. E.G.

/**
 * Class InstantiableModule
 *
 * Used to help register classes that should be instantiated on plugin load.
 */
abstract class InstantiableModule {

    /**
     * Set up the class and whether the module should be registered or not.
     */
    function __construct() {
        if ( $this->can_register() ) {
            add_action( 'tenup_plugin_loaded', [ $this, 'register' ], 9 );
        }
    }

    /**
     * Can the module be registered?
     * 
     * @return boolean
     */
    abstract public function can_register();

    /**
     * Register any hooks/functions for the module.
     */
    abstract public function register();
}

It would then be used like this:

/**
 * Add a test settings page to the admin area.
 */
class TestSettings extends InstantiableModule {

    /**
     * Can the module be registered?
     * 
     * @return boolean
     */
    public function can_register() {
        return is_admin();
    }

    /**
     * Register any hooks/functions for the module.
     */
    public function register() {
        add_action( 'init', [ $this, 'add_setting_page' ] );
    }

}

There would be a few benefits of changing the system to work this way:

If we were to implement this pattern of instantiating modules, it would also allow us to create a 10up CLI script to help engineers create common classes quickly. For example, say an engineer wanted to add a new settings page, we could have a stub set up that would allow them to do something like:

wp 10up make:setting Settings/SiteSettings --admin=true --fm=true

This CLI command would do something like:

  1. Create a new class using a 10up stub with the namespace Settings\SiteSettings
  2. Make sure that FieldManager is required via Composer.
  3. Set the can_register() method to return to is_admin() && function_exists( 'fm_register_submenu_page' )
  4. Set the register() method to call fm_register_submenu_page() and all other required functions.

The engineer would only need to set up the fields they want to render, reducing the amount of scaffolding they need to do significantly.

The CLI script is secondary to this issue, but I thought it was worth raising since it'd be significantly harder to implement if we stick with the current loading method via Plugin.php.