Control panel module system

[hanakin]

minor versions not major releases

[mrgoldy]

Seeing I am an extension developer myself, I do take extensions into consideration.
That being said, it's major code rework and a lot of the core files are going to chance, so somethings gotta give.

However, as most of the extensions already use a admin_controller.php of sorts (and I know you're one of them), not that much will have to chance. Or even if the code is in the main_module.php, it can be copied and the global's turned into injected services (that's pretty much what's going to happen with the core modules).
Currently you have to add a migration, a main_info and a main_module file to register a module for a control panel.
The "registering" part of the module is going to chance, the actual code for what an extension module does not. So this will basically mean extensions will only have to move the main_info information to another location or perhaps even only register it as a service with a tag.
Then for the admin_controller it will probably look something like this:

Code: Select all

public function handle()
{
	$this->helper->build_cp('my_module_route');
	
	$this->u_action = $this->helper->route('my_module_route');
	
	// Original code
	
	return $this->helper->render('my_template.html, 'Page title');
}

Only differences in the original code will be that trigger_error's can (and should be) replaced by exceptions or messages and that the $u_action variable no longer exists and has to be replaced by the actual route for your module. Meaning that instead of the commonly set_page_url() you can use something like in the example above, or replace the occurrences of the variable. Perhaps it's even possible to assign it in the constructor, but I believe it's not recommended to assign routes (with SID's) in the constructor as they might get transfered.

Anyways, I would rather not have this specific topic turn into an "are extension sufficiently supported" discussion and keep it on point with actual approaches for the new control panel system. While I agree with your concerns, it might be better of in it's own topic.

P.S. Where did the syntax highlighting go??

[mrgoldy]

And about the approach/thought proposed by CHItA.
I've been looking into it and thinking about it some more.

This would mean we would have to register each menu item in a yml file, and if it's a menu category, create a service collection for it aswell. Example:

Code: Select all

services:
    phpbb.acp.collection:
        class: phpbb\di\cp_service_collection
        arguments:
            - '@service_container'
        tags:
            - { name: service_collection, tag: phpbb.acp }

    phpbb.acp.general:
        class: phpbb\acp\info\general
        tags:
            - { name: phpbb.acp, before: phpbb.acp.forums }

    phpbb.acp.general.collection:
        class: phpbb\di\cp_service_collection
        arguments:
            - '@service_container'
        tags:
            - { name: service_collection, tag: phpbb.acp.general }

Then create a new ordered service collection (cp_service_collection for example) which orders the services accordingly with its own compiler pass.
The main *cp collection (phpbb.acp.collection) can then be used to call the classes that hold the menu item's information (title, auth, route, etc), eg: \phpbb\acp\info\general::get_title().

All this to end up with a hard-coded array of menu items.
Then it seems to me we might aswell create the array immediatly, add an event to it which extensions can use to add their menu items and that's it. Perhaps even provide a few additional array manipulation functions (add_before(), add_after, delete() and edit()) so adjusting the multidimensional array becomes easier.

Code: Select all

$acp = array(
	'general'	=> array(
		'title'		=> $this->lang->lang('ACP_CAT_GENERAL'),
		'auth'		=> $this->auth->acl_get('a_'),
		'route'		=> $this->helper->route('phpbb_acp_index'),
		'children'	=> array(
			'index'		=> array(
				'title'		=> $this->lang->lang('ACP_INDEX'),
				'auth'		=> true,
				'route'		=> $this->helper->route('phpbb_acp_index'),
			),
			'board'		=> array(
				'title'		=> $this->lang->lang('ACP_BOARD_CONFIGURATION'),
				'auth'		=> true,
				'children'	=> array(
					'settings'	=> array(
						'title'		=> $this->lang->lang('ACP_BOARD_SETTINGS'),
						'auth'		=> $this->auth->acl_get('a_board'),
						'route'		=> $this->helper->route('phpbb_acp_settings_board'),
					),
					// etc
				),
			),
			// etc
		),
	),
	'forums'	=> array(
		// etc
	),
);

/**
 * Event to add or modify ACP menu items
 *
 * @event core.acp_menu
 * @var	array	acp
 */
$vars = array('acp');
extract($phpbb_dispatcher->trigger_event('core.acp_menu', compact($vars)));

It might look less fancy, but realistically speaking, it is the exact same result. Arguable even more "overviewable" (or accessible, w/e the word is) than through I don't know how many service declarations and collections.

[mrgoldy]

Okay, let's post an update.
There was a valid point made that with a yml approach, it might be possible to completely cache it. While the services are cached, the order is not.
So that together with the fact it's rather hard to make a water-proof before/after ordered collection (not to mention with how many array searches and loops), I think it's not the best approach.

So, what I am currently thinking is a good approach is building an array of service declaration names, add an event and cache that, as extension's menu items are added / removed upon (un)installation and then the cache is then already purched by default.
Then loop through the array and get the item from the unordered service collection and retrieve the information for the menu item.

This way, the services are cached. The order is cached. Only the actual calls to the information are not cached, which shouldn't be as they can be/are dependent on the current page/url, for example if a forum id is available for authentication or when creating the route.

To better illustrate the above mentioned method, some code:

Code: Select all

    phpbb.acp.collection:
        class: phpbb\di\service_collection
        arguments:
            - '@service_container'
        tags:
            - { name: service_collection, tag: phpbb.acp }

    phpbb.acp.general:
        class: phpbb\acp\info\general
        tags:
            - { name: phpbb.acp }
            
    phpbb.acp.board:
        class: phpbb\acp\info\board
        tags:
            - { name: phpbb.acp }

    phpbb.acp.forums:
        class: phpbb\acp\info\forums
        tags:
            - { name: phpbb.acp }

Code: Select all

if (!($acp = $this->cache->get('_acp_menu')))
{
	$acp = [
		'phpbb.acp.general'		=> [
			'phpbb.acp.board'		=> [
				'phpbb.acp.attachments.settings'	=> null,
				'phpbb.acp.board.settings'			=> null,
				'phpbb.acp.board.features'			=> null,
			],
			'phpbb.acp.client'		=> [

			],
		],
		'phpbb.acp.forums'		=> [

		],
		'phpbb.acp.usergroup'	=> [

		],
	];

	/**
	 * Event to add or modify ACP menu items
	 *
	 * @event core.acp_menu
	 * @var	array	acp
	 */
	$vars = array('acp');
	extract($phpbb_dispatcher->trigger_event('core.acp_menu', compact($vars)));

	$this->cache->put('_acp_menu', $menu);
}

foreach ($acp as $category_id => $items)
{
	$category = $this->acp_collection[$category_id];

	$this->template->assign_block_vars('acp_menu', [
		'TITLE'		=> $category->get_title(),
		'S_AUTH'	=> $category->get_auth(),
		'U_VIEW'	=> $category->get_route(),
	]);
}

Code: Select all

class board extends general
{
	public function get_title()
	{
		return $this->lang->lang('ACP_GENERAL');
	}
	
	public function get_auth()
	{
		return parent::get_auth() && $this->auth->acl_get('a_board');
	}
}

[DavidIQ]

I think the concerns about backwards compatibility are well founded here and should be considered whenever this work actually takes place. Inconveniencing extension developers is manageable/understandable; ticking them all off is not as easily manageable. Even though this topic is mostly pie in the sky stuff right now and planned for a future major version, if there is a complete change on how ACP modules are handled then you also need to consider how some of that could/should be incorporated into the 3.x line to ease the transition.

[CHItA]

Okay, let's post an update.
There was a valid point made that with a yml approach, it might be possible to completely cache it. While the services are cached, the order is not.
So that together with the fact it's rather hard to make a water-proof before/after ordered collection (not to mention with how many array searches and loops), I think it's not the best approach.

You can cache the ordering, e.g. how the text-formatter services are cached.

BC is probably should be solvable for most cases, and probably we should figure out the way to keep forward compatibility as long as possible as well.

[Nicofuma]

about the order and stuff like that, last time I discussed about it we kinda conclude it was easier to either not support ordering or only weight ordering (when you define a panel you give a weight to it between PHP_INT_MIN and PHP_INT_MAX. The greater the weight the higher in the list). This way it is static and can be cached easily but also you on't need to load everything in order to cache anything.

I don't think we need to be more dynamic than the service registration level. I mean that in order to enable or disable a module you will always need to add or remove a service. So if we can keep things in this area everything will be kept in sync out of the box.

EDIT: to precise one point: I think we are using events for too much things. Especially when we are building something that is static, because in this case we have to load everything every-time or add a dedicated cache layer which must be kept in sync. So when we can wok without events I think it is a good idea to do so.

[mrgoldy]

Bit stating the obvious here, but I think we can agree on the fact that we're all leaning towards the following structure:
Each control panel page (module) will have its own route.
Each control panel route will have its own controller.
Each control panel will have its own navigation declared with services.
Each control panel navigation item will lead to a control panel route.

The route and controllers are pretty straight-forward, nothing really there to discuss.
We are now determining how to efficiently register and load the panel's navigation menu.

As far as I can tell there currently are 2 approached brought forward:
1. Entirely through service declarations
2. Combination of service declarations and a PHP array.

While I agree that it is better to keep everything in one place, there are - in my opinion - quite a few disadvantages to doing everything within only service declarations. There are 3 options to order does services:
1. No ordering and completely rely on the order the services are fetched
2. "Weight" ordering
3. "Before/After" ordering

For "weight" ordering, I can only foresee weird and inconsistent code. As I presume they pretty much work the same as Symfony's native priority and phpBB's order. So then all native categories are ordered by a weight, with a step of (lets say) 100. So you will have categories with 100, 200, 300, 400 and 500. But then in the future, the decision is made a category has to be added in between. So now you have a categories with 100, 200, 250, 300, 400 and 500. Or you would have to refactor all weights for the categories, which will keep extension developers quite busy.
This is not only for categories, but also for subcategories and the actual items. In the end (not immediatly when it's introduced) you will end up with inconsistency.

For "before/after" ordering, I'm having trouble seeing and finding an "air-tight" way of achieving this, keeping in mind that services could be added / removed by phpBB and Extensions. But perhaps that's my lack of knowledge on ordering arrays this way, without examples. There are a few out there, but they all assume things that can not be assumed in this case.

For both these ordering methods, this will mean we will also have to order phpbb's native services. Meaning we have to loop through, depending on the method even perform (possible quite a few) array_searches, array_s(p)lices and foreach()s. I understand that they will be cached, but so in the other method, I'm talking about when they're not and have to be build.

Not to mention that for every category and subcategory, a service collection has to be created.
Now in the ACP alone, there are 9 categories and 23 subcactegories.

---

Now the method I propose, is register all the panel's navigation items to a single collection, eg phpbb.acp, and do the ordering through a PHP array. This array is put through an event, indeed, but only when it's not already cached. This means that no ordering has to be done on phpBB's native menu items, no limitation in added items in the future and easier access for extensions which can then simply use a function similar to this to add their items.

The benefits I see to this is everything is still cached, there has to be no ordering done on phpBB's native items, less service collections and easier to provide a solid ordering.

To better illustrate the approach I created a sample PR ([ticket/15977] Move control panels to controllers), where you can see most of it in action. It's just an example and only the old acp_main and acp_board are some what done (both controllers still need a lot of work) but that should be enough to provide a working example on how I would approach handling the navigation aspect.

[mrgoldy]

Well, I am stuck with this.
All options I have proposed thusfar have been 'rejected' or refuted (lack of better words), all with good reasons.
However, this does mean that I am out of ideas on possible approaches.
I am still willing to take this on, but I would need an actual example/set up of how it should look like.
Otherwise, I will have to pass this up.

[hanakin]

not sure whee you are stuck but the whole concept of control panel needs to go away in my opion. you should have admin route for all the acp stuff and everyrthing related the mcp/ucp needs routed to the main model. ie topics controller should have the ability to perfrom mcp actions for topics etc...