Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP Design Patterns #4

Open
tlovett1 opened this issue Feb 4, 2021 · 2 comments
Open

PHP Design Patterns #4

tlovett1 opened this issue Feb 4, 2021 · 2 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@tlovett1
Copy link
Member

tlovett1 commented Feb 4, 2021

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.

@tlovett1 tlovett1 added enhancement New feature or request help wanted Extra attention is needed labels Feb 4, 2021
@nicholasio
Copy link
Member

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
Copy link
Collaborator

@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: #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:

  • It keeps the same structure to the code that engineers are already familiar with.
  • It removes a required step from the setup of a new class.
  • It gives engineers an easy way to use the standard setup (via InstantiableModule) and allows them to implement custom loading via the constructor should they want that control.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants