Feature/admin pages menu

[Camwyn]

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!

[juanfra]

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.

[lucatume]

I would decomplicate the stack by removing interfaces and traits to have a menu and sub-menu abstract class.

[lucatume]

This comment applies here and to other properties: if a property is meant to be overridden and provided a non-empty value to make sense, do not make it a property. Make it a method, abstract in this class, that extending classes will have to implement (e.g. here it would be get_page_title) the method should be protected (it's not part of the public API); if a public method by the same name already exists, just use that.
This would allow someone to come in, extend the abstract class and get a complete checklist of the methods to fill.

[Camwyn]

I'm not sure I agree with this logic...
You are saying this:

	/**
	 * {@inheritDoc}
	 */
	public function get_icon_url() : string {
		return $this->get_menu_icon();
	}

	/**
	 * {@inheritDoc}
	 */
	public function get_page_title() : string {
		return _x( 'The Events Calendar', 'The title for the admin page', 'the-events-calendar');
	}

	/**
	 * {@inheritDoc}
	 */
	public function get_menu_title() : string {
		return _x( 'The Events Calendar', 'The title for the admin menu link', 'the-events-
	}

is simpler than this:

	public function init() : void {
		$this->page_title = _x( 'The Events Calendar', 'The title for the admin page', 'the-events-calendar');
		$this->menu_title = _x( 'The Events Calendar', 'The title for the admin menu link', 'the-events-calendar');
		$this->icon_url   = $this->get_menu_icon();

		parent::init();
	}

[lucatume]

This is where I see the noise coming up: we have Menu pages and sub-menu pages. They are different things requiring different logic. I think the extension/inheritance/implementation stack here could be cut down having a base abstract class for menu pages and a base abstract class for sub-menu pages.

[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 - yes. the idea behind this is something that is less opinionated, less tied to TEC/ET, does not include admin pages in it automatically, and can be separated out and shared both with Stellar and with the rest of the world.

[Camwyn]

Not sure why GitHub decided to attach that to this comment 🤦🏼

[juanfra]

@Camwyn I don't understand the opinionated part. Could you please expand?

I feel that menus and submenus are just pages, thinking abstractly. Creating whole new classes for something that was already done a couple months ago (and that's ~300 lines) would be maybe taking valuable time from us. To add up, we are still using tec_ prefixes, so I don't understand how this would be less tied to TEC/ET.

With the current admin pages implementation there's no page generated, that was moved to the plugins, and it was kind of one of the goals, because for ET as standalone the TEC page was created automatically from common.

[Camwyn]

The final home for this would be a public Stellar library - so not tec_ prefixed (just convenient for now) and not built with TEC/ET's needs specifically in mind.

Note it uses no tribe() functionality (although that would be convenient - it won't, come time to separate it)

[lucatume]

Used only by one class, I would make this part of an abstract sub-menu class.

[Camwyn]

The traits are designed to be used when extending the abstract menu class - a la carte menu pages.
See the way they were implemented in the TEC PR.

I can see where it might make more sense to make Submenu the default though 🤔

[lucatume]

This hook should live in a service provider, could this code be refactored to have this live there?

[Camwyn]

I'd rather not. I want the (Abstract_Menu) class to be all the user has to deal with (remember - this is intended to become a standalone) so all they have to do is extend Abstract_Menu and use any needed Traits...and not instantiate a Service Provider

[Camwyn]

I actually have some ideas about this so...maybe 😉

[lucatume]

If this trait will be used only in menu, or sub-menu, pages, then I would just include it in the competent abstract class.

[Camwyn]

It will - but not all of them.

[lucatume]

Too generic a method name for a trait, conflicts will arise.

[Camwyn]

I agree (and have had said collisions 😜) - I am working on that

[lucatume]

On the same note as the Admin Bar one, if this one will be used only by menu or sub-menu pages, then I would just include it there.

[Camwyn]

Why? Why add all the code for settings (which isn't working yet BTW) when all I want is a simple top-level menu?

[lucatume]

Too generic a method name for a trait.

[lucatume]

Furthermore, this looks like something that should live in a __construct method, not in a method that implementers have to remember to call.

[lucatume]

Some changes with build configuration files.

[lucatume]

I know we used to ignore the tric configuration files in Common, but I think we should not: TEC and other plugins are including tric configuration files in VCS.

[lucatume]

This looks like a good chance to move Common build away from Travis CI, as is the case for the other plugins, and just remove any configuration file related to it.

[lucatume]

Update this to 3.1.6 (latest version).

[lucatume]

Minimum required version of PHP in the plugins is 7.3; the platform.php value should stay on 7.3.33.

[lucatume]

Remove excess empty line before, add class doc-block.

[lucatume]

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.