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

Subaction interface #8164

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

Conversation

live627
Copy link
Contributor

@live627 live627 commented Apr 1, 2024

Motivation

To cut down on the use of static properties as much as possible and implement well-defined methods for adding sub-actions. The goal here would be to hook into one entry point probably in SMF\Forum where new functions could be implemented that return the instance of the action class.

Definition

This is the interface for future reference (I suck at naming things):

/**
 * Interface for all action classes to register their sub-actions.
 *
 * Any action class that delegates tasks to different sub-actions should
 * implement this interface and use  ProvidesSubActionTrait.  Use the public
 * methods defined in this interface to aad sub-actions.
 */
interface ProvidesSubActionInterface
{
	/****************
	 * Public methods
	 ****************/

	/**
	 * Set a default sub-action.
	 *
	 * The default sub-action is the one executed if no sub-action was requested.
	 *
	 * @param string $sa
	 */
	public function setDefaultSubAction(string $sa): void;

	/**
	 * Add a sub-action.
	 *
	 * A sub-action is another term for a route where a URL parameter, usually "sa", has
	 * its value mapped to some function.
	 *
	 * @param string $sa
	 * @param callable $cb
	 */
	public function addSubAction(string $sa, callable $cb): void;

	/**
	 * Determines whether the provided sub-action exists.
	 *
	 * @param string $sa
	 *
	 * @return bool
	 */
	public function hasSubAction(string $sa): bool;

	/**
	 * Finds a sub-action that is associated with the given keyword.
	 *
	 * If no sub-actions match or nothing was provided, the internal variable is set to
	 * either the previously specified default or the first registered sub-action.
	 *
	 * @param string|null $sa
	 */
	public function findRequestedSubAction(?string $sa = null): void;

	/**
	 * @return string|null
	 */
	public function getSubAction(): ?string;

	/**
	 * Executes the sub-action.
	 *
	 * @uses findRequestedSubAction() if $sa is null.
	 *
	 * @param string|null $sa
	 *
	 * @return mixed
	 */
	public function callSubAction(?string $sa = null): mixed;
}

Action objects that use sub-actions are encouraged to implement this.

@live627 live627 added the Housekeeping SMF code reorganization label Apr 1, 2024
@live627 live627 self-assigned this Apr 1, 2024
@live627 live627 force-pushed the subaction-interface branch from 74b3920 to 10e6949 Compare April 2, 2024 14:17
@live627 live627 force-pushed the subaction-interface branch from 10e6949 to 99321ad Compare May 7, 2024 03:29
@live627
Copy link
Contributor Author

live627 commented May 7, 2024

@tyrsson I would like your thoughts on this implementation, how it might work with PSR-14. Should we stay with the current way off plastering hooks everywhere (nearly one for each sub-action implementation) or would a single entry point be best, where a conditional check for the right class instance must be made? Or perhaps this approach is bad and should be scrapped?

@tyrsson
Copy link
Collaborator

tyrsson commented May 7, 2024

Will try to look into it tomorrow. Here is some general thoughts on it though.

Your psr 14 events/listeners can become first class actors rather than simply a means to get arrays passed around.

The subactions could actually be executed as a psr 14 event. Mod authors then just attach listeners to said event. If they have a reason to do so.

ie
$eventIdentifier = 'some.subaction'; // fires at priority = 0
Given two listeners, one at priority = -1 and one at priority = 1 we can then run code before and after some.subaction with ZERO other modifications to the code based solely on the priority of the attached listener. Works great for stuff like caching and such.

You are introducing a SubActionInterface paired with an expressing trait. Composition over inheritance. I like it. The application should only care that the expressing class honors the contract defined by the interface. That should be the only required check. Now, to get more specific than that you would need more than a single interface. Where subaction interface would be an extension of other interfaces. Say a ConfigVarAwareInterface, which means it's capable of returning a configvar array for consumption. That the application can depend on. It's the contract defined by that interface. Only an instanceof check is required to determine if this action/subaction provides any.

As you are doing. SMF needs to be built to the interface, mod authors should be the only ones (generally speaking) typing more specific than the interface.

As a general rule of thumb. SMF classes do about a 1000 more things than they should. SRP is a thing for a reason ;)

@tyrsson
Copy link
Collaborator

tyrsson commented May 10, 2024

Another minor thought in regards to naming. Its not very SMF but I would suggest either.

SubActionProviderInterface
or
SubActionAwareInterface

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

@tyrsson tyrsson May 10, 2024

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.

@live627
Copy link
Contributor Author

live627 commented Jul 23, 2024

@Sesquipedalian can I have your thoughts on this please? Do you think this could be a good change, having an interface to explicitly build sub-actions with?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Housekeeping SMF code reorganization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants