tribe-common icon indicating copy to clipboard operation
tribe-common copied to clipboard

Feature/admin pages menu

Open Camwyn opened this issue 1 year ago • 2 comments

Draft so folks can see and comment on where I am with the new admin page/menu architecture.

Note: one premise I have been working on is that all pages are handled by their respective plugins - None of them are contained in common.

This is for reasons:

  1. It reduces the code in common to just the core functionality.
  2. It makes the code a potential for a removable, sharable library.
  3. No cross-contamination. (looking at you, Troubleshooting, App Shop, and Help pages...)

NOTE: tests will fail - this uses Composer 2 and our workflows aren't set up for that!

Camwyn avatar Sep 19 '22 14:09 Camwyn

Are we re-doing what we recently introduced here? https://github.com/the-events-calendar/tribe-common/blob/master/src/Tribe/Admin/Pages.php

We introduced that as part of the Tickets menu, and all plugins are using now that "framework" to already register and manage admin pages.

juanfra avatar Sep 19 '22 14:09 juanfra

Adding a catch-all comment here to address my previous comments regarding the abstract class and trait architecture.

To frame my comment, I'm thinking about my experience implementing Views v2 and the ORM/Repository; my guiding light there was to keep the code clean and strive for good architecture that would make sense. In actual, day-to-day use, it turned out both implementations had missed the mark putting a knowledge requirement on the developers willing to use and extend them. In more than one dev meeting we discussed how to make one or the other simpler and @borkweb created an adapter API on top of Views v2 code to address the issue when he found himself struggling to come to terms with some non-obvious implementation choices.

This PR deals with code to be used by developers to base more code upon, and I think there is merit in it. If this refactoring aims at making something standalone and general purpose, then it's easy for me to think of something like ACF. Specifically, the part of ACF that ends up producing a schema (in the shape of a PHP array), that is easy to understand and would well reflect the nature of what this library is trying to do (in my understanding): create the business logic that, when fed a configuration, would manage and set up settings and related menus. The way I see this being adopted outside of a crowd of TEC developers with a lot of knowledge and context of how this is used in TEC products, does not go through a class, but through a PHP (or JSON) schema easy to write and understand. Developers using this library outside of TEC will not have easy access to other implementations built on the library and will have to work it out from what they see.

If I start from the end of the path, the one where a developer pulls the library, copies and pastes a JSON schema from the documentation and runs with it, then I ask myself how the PHP class consuming it would look. It would have to use all traits since any one might be required (I'm excluding use of eval for creation of dynamic trait-based classes, although it would be fun), it would then need to have a number of methods wrapping the trait ones to "dud" trait methods that are not required by the schema (e.g. the schema is not for a post type, then the post type trait methods should be no-ops). This class would be messy to maintain and debug for users. If a developer is more hands-on and wants to use a class directly, she will have to extend the base abstract class and then try and tinker until she ended up including all the required traits, by either exploring the code, reading the documentation and then exploring the code, or by copying from existing code from TEC uses. Frustration could easily take over.

Beside ACF, there are other notable examples of schema-based, general purpose, libraries like johnbillion/extended-cpts.

I propose the use of two, possibly one, abstract class as, I think, it would solve the pain points of both scenarios above. In the first, schema-driven, scenario, the inheritance tree would look like this (-> means "extends"):

JSON_Schema_Settings -> Schema_Settings -> Abstract_Settings

In the second scenario:

Acme\Project\BookCPTSettings -> Abstract_Settings
Acme\Project\PaymentGatewaySettings -> Abstract_Settings
Acme\Project\HomepagePreferences -> Abstract_Settings

The Abstract_Settings class would have a number of "flag" methods that would enable business logic methods like this:

<?php

abstract class Abstract_Settings {
	abstract protected function is_menu(): bool;

	abstract protected function is_cpt(): bool;

	protected function set_up_menu(): void {
		if ( ! $this->is_menu() ) {
			return;
		}

		// do stuff
	}

	protected function set_up_cpt(): void {
		if ( ! $this->is_cpt() ) {
			return;
		}

		// do stuff
	}
}

The use of abstract will guide the implementation of the class in scenario 2 on any modern IDE (e.g. PHPStorm will suggest the implementation of all abstract methods when extending an abstract class); with ablative and self-explanatory method names, the developer will not require to read any documentation. Since properties cannot be declared abstract, I suggest the use of methods to leverage the guided development I mention above; methods returning fixed scalar values will become properties at the op-cache level, and performance would not be meaningfully impacted. Moreover, methods can be made as dynamic as required (e.g. "this will be a menu if A is true, else it will be a sub-menu).

How would it play out in scenario 1? Example code follows (with reference to the code above):

class Schema_Settings extends Abstract_Settings {
	protected $schema = [];

	public function __construct( array $schema ) {
		$this->schema = $schema;
	}

	protected function is_menu(): bool {
		return isset($this->schema['menu']);
	}

	protected function is_cpt(): bool {
		return isset($this->schema['cpt']);
	}
}

class JSON_Schema_Settings extends Schema_Settings {
	public function __construct( string $json_file ) {
		$schema = json_decode( file_get_contents( $json_file ), true, 512, JSON_THROW_ON_ERROR );
		parent::__construct( $schema );
	}
}

A schema-based implementation would, then, make testing very easy. That would include bug reports.

I know this was a long comment, thanks for taking the time to read it.

lucatume avatar Sep 29 '22 09:09 lucatume