Closed luislard closed 4 days ago
While I maybe agree with having a "generic" hookName to hook into all Packages, this could also introduce some side effects like hooking into a package which shouldn't be extended.
So instead of doing:
add_action(
MyPackage\plugin()->hookName(Package::ACTION_INIT),
static function(Package $plugin): void {
$plugin->addModule( ... );
}
);
you simply could loop over your packages by doing:
$packagesToExtend = [
MyPackage\plugin()
];
foreach($packagesToExtend as $package) {
add_action(
$package->hookName(Package::ACTION_INIT),
static function(Package $plugin): void {
$plugin->addModule( ... );
}
);
}
you simply could loop over your packages by doing:
The problem here is that you don't know whether MyPackage\plugin()
exists. If the plugin is deactivated, you get fatal. This means we should check it with function_exists
, which is pretty verbose.
I think the hook would be good to have.
Looping packages, besides using function_exists
(which is also not the fastest function in town) has the problem that if you want to target all/most packages, then you might need to edit the function very frequently, e.g. every time a new plugin is added or removed.
Moreover, one issue of calling MyPackage\plugin()
upfront, is that you might cause that plugin to boot earlier than expected. And if you "wait" using hooks, it might be too late.
To act as soon as the package is ready without side effects is impossible right now, because you need a package instance to add hooks, and obtaining that instance might have side effects.
To help avoid possible unwanted effects, we could fire the hook similarly to:
do_action(
'inpsyde.modularity.init',
$package->properties()->basename(),
$package
);
This way, whoever listens to the event could easily filter on the basename. For example:
add_action(
'inpsyde.modularity.init',
static function ($basename, Package $package): void {
if (str_start_with($basename, 'acme-')) {
$package->addModule(/* ...*/);
}
},
10,
2
);
This could also be used to build a flexible allow-list that you don't need to update often:
const PACKAGES_TO_EXTEND = [
'^(inp)?syde-.+$',
'^my-client-.+$',
'something-else',
];
add_action(
'inpsyde.modularity.init',
static function ($basename, Package $package): void {
foreach (PACKAGES_TO_EXTEND as $package) {
if (preg_match("#$package#", $basename)) {
$package->addModule(/* ...*/);
return;
}
}
},
10,
2
);
If a developer decides to do something with a Package
instance without checking the package name... that's on them. I mean, we have to trust people to know what they are doing, and if they don't, there are so many different ways they can cause issues...
@Chrico it seems you agreed on having this hook.
Let's agree on :
I was thinking of adding this constant to the class:
public const ACTION_MODULARITY_INIT = self::HOOK_PREFIX . self::ACTION_INIT;
The constant evaluates to "inpsyde.modularity.init"
.
And then do:
do_action(
self::ACTION_MODULARITY_INIT,
$this->properties()->baseName(),
$this
);
Immediately after the current package-specific "init" hook.
That said I was also thinking that we don't have any hook after the package is "built". In 1.7.0 we introduced built()
but still if we want to hook at that point, which is the first point where it is safe to get a container, we can't do it.
The first point where it is safe to call $package->container()
is here but there's no hook there.
If you call container()
in the current "init" hook or in the new "generic" init hook, you get a fatal error. That is why I was thinking of adding this additional constant to the class:
public const ACTION_INITIALIZED = 'initialized';
And then, after the status is "initialized" do something like:
do_action(
$this->hookName(self::ACTION_INITIALIZED),
$this
);
I don't think we need a "generic" hook also there. But having a package-specific hook the first moment it is safe to call $package->container()
I think it is useful.
What you (and everyone) think about this? If you agree, can we have a single PR that address both these two new hooks?
I totally agree with the generic Package::ACTION_MODULARITY_INIT
-hook. ✔️
But, by writing the text below, maybe the name is wrong. The status of the Package is STATUS_IDLE
( STATUS_INIT
does not exists), so the action should reflect that as well? 😅
That said I was also thinking that we don't have any hook after the package is "built". In 1.7.0 we introduced built() but still if we want to hook at that point, which is the first point where it is safe to get a container, we can't do it.
I'm wondering what the use case for accessing the container via this hook at this time could be. Maybe we should revisit the workflow of registration, executing, resolving, extending and when which hook makes sense.
v1) We probably could even use the Package::progress()
to trigger those hooks (we already have STATUS_READY
and ACTION_READY
) so we could have a mapping and automatically trigger those hooks. This could also have the downside that some context (like for "failure") is missing. So we need to ensure that all information is accessible through $this
.
const STATUS_TO_ACTION = [
self::STATUS_READY => self::ACTION_READY,
self::STATUS_INITIALIZED => self::ACTION_INITIALIZED,
];
private function progress(int $status): void
{
$action = self::STATUS_TO_ACTION[$status] ?? null;
if($action !== null && !did_action($action)){
do_action(
$action,
$this->properties()->baseName(),
$this
);
}
$this->status = $status;
}
v2) Or we also could implement a similar logic as WordPress has with post_status changes where they have an action which contains "from-to":
self::HOOK_PREFIX . ".{$oldStatus}_to_{$newStatus}"
v3) Or - probably not that performant - we implement a
do_action( self::HOOK_PREFIX . '.progress', $oldStatus, $newStatus, $baseName, $this);
@Chrico I like the idea of emitting a hook on progress
and I like the idea of having a map between statuses and hooks, even if we might need more statuses than hooks.
Statuses help us determine internally what happened and what is happening, while hooks open the class to the extern and we don't need to expose the class externally at any status change.
For the extern, we only need to expose:
So we have just 3 hooks but more statuses.
I think we could go with:
STATUS_IDLE
statusSTATUS_INIT
-- This would be newACTION_INIT
hook is fired (package specific + global) -- The global hook would be newSTATUS_INITIALIZED
ACTION_INITIALIZED
hook is fired. -- This would be newSTATUS_BOOTING
-- This would be newSTATUS_READY
ACTION_READY
hook is fired.STATUS_DONE
-- _This would be renamed from "STATUSBOOTED"This could be done in a 100% backward compatible way.
So we would have statuses like:
public const STATUS_IDLE = 2;
public const STATUS_INIT = 3;
public const STATUS_INITIALIZED = 4;
public const STATUS_BOOTING = 5;
public const STATUS_READY = 7;
public const STATUS_DONE = 8;
And a "map" like:
private const STATUSES_ACTIONS_MAP = [
self::STATUS_INIT => self::ACTION_INIT,
self::STATUS_INITIALIZED => self::ACTION_INITIALIZED,
self::STATUS_READY => self::ACTION_READY,
];
There's IMO no need for an action for self::STATUS_BOOTING
because, at that point, the container is already locked, so it can be used to add services, and to access services there're already two hooks, one before and one after, so I don't think this additional hook would be useful at all.
If you agree with the above I can send a PR.
Is your feature request related to a problem?
Currently, the only way to add modules from other places is via adding an action to this hook
The hook name is created dynamically based on the package name.
Since #44 was merged it would be very nice to have a way to add a module with an extension to all packages automatically.
So, in a project with many packages we could do:
Describe the desired solution
Maybe the solution is to add a hook.
Describe the alternatives that you have considered
Option 1: Adding a generic hook
Option 2: Using current hookName is also an option but you should hook in another hook like
Additional context
No response
Code of Conduct