Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Subaction interface #8164

Draft
wants to merge 1 commit into
base: release-3.0
Choose a base branch
from
Draft

Subaction interface #8164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 11 additions & 36 deletions Sources/Actions/Activate.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
use SMF\Lang;
use SMF\Logging;
use SMF\Mail;
use SMF\ProvidesSubActionInterface;
use SMF\ProvidesSubActionTrait;
use SMF\Routable;
use SMF\Security;
use SMF\Theme;
Expand All @@ -34,35 +36,11 @@
/**
* Activates a user's account.
*/
class Activate implements ActionInterface, Routable
class Activate implements ActionInterface, ProvidesSubActionInterface, Routable
{
use ActionRouter;
use ActionTrait;

/*******************
* Public properties
*******************/

/**
* @var string
*
* The sub-action to call.
*/
public string $subaction = '';

/**************************
* Public static properties
**************************/

/**
* @var array
*
* Available sub-actions.
*/
public static array $subactions = [
'activate' => 'activate',
'resend' => 'resend',
];
use ProvidesSubActionTrait;

/*********************
* Internal properties
Expand Down Expand Up @@ -106,17 +84,15 @@ public function execute(): void
return;
}

if (empty($this->subaction)) {
$this->findRequestedSubAction($_REQUEST['sa'] ?? null);

if (empty($this->sub_action)) {
$this->showResendRequest();

return;
}

$call = method_exists($this, self::$subactions[$this->subaction]) ? [$this, self::$subactions[$this->subaction]] : Utils::getCallable(self::$subactions[$this->subaction]);

if (!empty($call)) {
call_user_func($call);
}
$this->callSubAction($_REQUEST['sa'] ?? null);
}

/**
Expand Down Expand Up @@ -234,6 +210,9 @@ public static function parseRoute(array $route, array $params = []): array
*/
protected function __construct()
{
$this->addSubAction('activate', [$this, 'activate']);
$this->addSubAction('resend', [$this, 'resend']);

// Logged in users should not bother to activate their accounts
if (!empty(User::$me->id)) {
Utils::redirectexit('action=profile');
Expand All @@ -259,10 +238,6 @@ protected function __construct()
if (!empty($_REQUEST['code'])) {
$_REQUEST['sa'] = 'activate';
}

if (!empty($_REQUEST['sa']) && isset(self::$subactions[$_REQUEST['sa']])) {
$this->subaction = $_REQUEST['sa'];
}
}

/**
Expand Down
68 changes: 19 additions & 49 deletions Sources/Actions/Admin/Attachments.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
use SMF\ItemList;
use SMF\Lang;
use SMF\Menu;
use SMF\ProvidesSubActionInterface;
use SMF\ProvidesSubActionTrait;
use SMF\Sapi;
use SMF\SecurityToken;
use SMF\Theme;
Expand All @@ -41,47 +43,12 @@
/**
* Maintains and manages attachments and avatars.
*/
class Attachments implements ActionInterface
class Attachments implements ActionInterface, ProvidesSubActionInterface
{
use ActionTrait;

use ProvidesSubActionTrait;
use BackwardCompatibility;

/*******************
* Public properties
*******************/

/**
* @var string
*
* The requested sub-action.
* This should be set by the constructor.
*/
public string $subaction = 'browse';

/**************************
* Public static properties
**************************/

/**
* @var array
*
* Available sub-actions.
*/
public static array $subactions = [
'attachments' => 'attachmentSettings',
'avatars' => 'avatarSettings',
'browse' => 'browse',
'maintenance' => 'maintain',
'remove' => 'remove',
'byage' => 'removeByAge',
'bysize' => 'removeBySize',
'removeall' => 'removeAll',
'repair' => 'repair',
'attachpaths' => 'paths',
'transfer' => 'transfer',
];

/****************
* Public methods
****************/
Expand All @@ -91,11 +58,9 @@ class Attachments implements ActionInterface
*/
public function execute(): void
Copy link
Collaborator

@tyrsson tyrsson May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[3.x.x relevant]
This is a great place to see where PSR-14 provides a simplified workflow. Bare with me its been a long day.

Instead of needing to call both IntergrationHook::call(....) then $this->callSubAction(....)

You would only need to call the implementers dispatch method usually something like dispatch or trigger.
Where $_REQUEST['sa'] === event identifier.
We do not care if there are zero listeners or 10 listeners (usually).

Instead of having integration hooks littered throughout the code you end up simply triggering/dispatching events. Which, at that point is just normal application execution.

The event is just the message between the dispatcher and the listener. The event can have nearly any state that is required.
It could be a SubActionEvent.

What this does is it decouples the dispatch from the listener. All that matters is that you are dispatching a registered event. The listener is what determines the typing for the Event. So, at that point all that we require is that we are being passed a SubActionEventInterface instance. Which insures our contract is honored. SubActionEvent should be open for extension.

Currently you can view the IntegrationHook::call as the dispatcher. All of the "hooks" registered to act on that call are your listeners. We get passed an array by reference of sub_actions.

A side effect of this which is a positive once the min php req jumps to 8.1 is that you can have a backed Enum expose all known Events. You could then pass $_REQUEST['sa'] to the tryFrom method and insure its a known event type or set the expected type. Null safe and null coalescing operator usage here greatly simplifies your syntax.

{
$call = method_exists($this, self::$subactions[$this->subaction]) ? [$this, self::$subactions[$this->subaction]] : Utils::getCallable(self::$subactions[$this->subaction]);
IntegrationHook::call('integrate_manage_attachments', [&$this->sub_actions]);

if (!empty($call)) {
call_user_func($call);
}
$this->callSubAction($_REQUEST['sa'] ?? null);
}

/**
Expand Down Expand Up @@ -2572,6 +2537,19 @@ public static function attachDirStatus(string $dir, int $expected_files): array
*/
protected function __construct()
{
$this->setDefaultSubAction('browse');
$this->addSubAction('attachments', [$this, 'attachmentSettings']);
$this->addSubAction('avatars', [$this, 'avatarSettings']);
$this->addSubAction('browse', [$this, 'browse']);
$this->addSubAction('maintenance', [$this, 'maintain']);
$this->addSubAction('remove', [$this, 'remove']);
$this->addSubAction('byage', [$this, 'removeByAge']);
$this->addSubAction('bysize', [$this, 'removeBySize']);
$this->addSubAction('removeall', [$this, 'removeAll']);
$this->addSubAction('repair', [$this, 'repair']);
$this->addSubAction('attachpaths', [$this, 'paths']);
$this->addSubAction('transfer', [$this, 'transfer']);

// You have to be able to moderate the forum to do this.
User::$me->isAllowedTo('manage_attachments');

Expand All @@ -2585,14 +2563,6 @@ protected function __construct()
'description' => Lang::$txt['attachments_desc'],
];

IntegrationHook::call('integrate_manage_attachments', [&self::$subactions]);

if (!empty($_REQUEST['sa']) && isset(self::$subactions[strtolower($_REQUEST['sa'])])) {
$this->subaction = strtolower($_REQUEST['sa']);
}

Utils::$context['sub_action'] = &$this->subaction;

// Default page title is good.
Utils::$context['page_title'] = Lang::$txt['attachments_avatars'];
}
Expand Down
71 changes: 22 additions & 49 deletions Sources/Actions/Admin/Bans.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
use SMF\Lang;
use SMF\Logging;
use SMF\Menu;
use SMF\ProvidesSubActionInterface;
use SMF\ProvidesSubActionTrait;
use SMF\SecurityToken;
use SMF\Theme;
use SMF\Time;
Expand All @@ -40,42 +42,12 @@
/**
* This class contains all the methods used for the ban center.
*/
class Bans implements ActionInterface
class Bans implements ActionInterface, ProvidesSubActionInterface
{
use ActionTrait;

use ProvidesSubActionTrait;
use BackwardCompatibility;

/*******************
* Public properties
*******************/

/**
* @var string
*
* The requested sub-action.
* This should be set by the constructor.
*/
public string $subaction = 'list';

/**************************
* Public static properties
**************************/

/**
* @var array
*
* Available sub-actions.
*/
public static array $subactions = [
'list' => 'list',
'edit' => 'edit',
'add' => 'edit',
'browse' => 'browseTriggers',
'edittrigger' => 'editTrigger',
'log' => 'log',
];

/****************
* Public methods
****************/
Expand All @@ -87,11 +59,19 @@ public function execute(): void
{
User::$me->isAllowedTo('manage_bans');

$call = method_exists($this, self::$subactions[$this->subaction]) ? [$this, self::$subactions[$this->subaction]] : Utils::getCallable(self::$subactions[$this->subaction]);
IntegrationHook::call('integrate_manage_bans', [&$this->sub_actions]);

$this->findRequestedSubAction($_REQUEST['sa'] ?? null);

if (!empty($call)) {
call_user_func($call);
// Mark the appropriate menu entry as selected
if (array_key_exists($this->sub_action, Menu::$loaded['admin']->tab_data['tabs'])) {
Menu::$loaded['admin']->tab_data['tabs'][$this->sub_action]['is_selected'] = true;
}

Utils::$context['page_title'] = Lang::$txt['ban_title'];
Utils::$context['sub_action'] = $this->sub_action;

$this->callSubAction();
}

/**
Expand Down Expand Up @@ -1472,6 +1452,13 @@ public static function list_getNumBanLogEntries(): int
*/
protected function __construct()
{
$this->addSubAction('list', [$this, 'list']);
$this->addSubAction('edit', [$this, 'edit']);
$this->addSubAction('add', [$this, 'edit']);
$this->addSubAction('browse', [$this, 'browseTriggers']);
$this->addSubAction('edittrigger', [$this, 'editTrigger']);
$this->addSubAction('log', [$this, 'log']);

Theme::loadTemplate('ManageBans');

// Tab data might already be set if this was called from Logs::execute().
Expand Down Expand Up @@ -1502,20 +1489,6 @@ protected function __construct()
],
];
}

IntegrationHook::call('integrate_manage_bans', [&self::$subactions]);

if (!empty($_REQUEST['sa']) && isset(self::$subactions[$_REQUEST['sa']])) {
$this->subaction = $_REQUEST['sa'];
}

// Mark the appropriate menu entry as selected
if (array_key_exists($this->subaction, Menu::$loaded['admin']->tab_data['tabs'])) {
Menu::$loaded['admin']->tab_data['tabs'][$this->subaction]['is_selected'] = true;
}

Utils::$context['page_title'] = Lang::$txt['ban_title'];
Utils::$context['sub_action'] = $this->subaction;
}

/**
Expand Down
Loading
Loading