-
Notifications
You must be signed in to change notification settings - Fork 669
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
feature(events): allows wider variety of callables to be unregistered #8071
Conversation
This adds a class usable for matching anonymous functions, dynamic method callbacks, and invokable objects without the need for an object reference. The matcher can be passed to the event/hook unregister functions to successfully remove such types of handlers. This also renames the event registration test so it’s actively tested by PHPUnit. Fixes Elgg#7750 Fixes Elgg#8069
This makes matching anon functions possible, but I don't know how we'd make it elegant or more reliable. The safest usage would be to specify the exact line with no margin for error, and loudly error if the unregister function failed to match. OTOH 3rd party plugins can always change their handler names, too. |
// ... elsewhere | ||
|
||
// we match a function that ends between lines 115 and 125: | ||
$matcher = new Elgg\CallableMatcher('function /mod/myplugin/start.php:120+-5'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would much prefer something like new Elgg\AnonymousFunctionMatcher('/file/path/start.php', 115, 125);
But even then... this is just a horrible, terrible hack. Can we please reconsider just leaving anonymous functions alone? This makes the line number of my function part of my plugin's API, which is just craziness to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It really is bad. One path I considered was making a NamedClosure, which is just a Closure wrapped in an invokable object with a readable name property. That would give the benefits of inline registration but making the handler searchable...also no one would ever go through the trouble of using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strawman
elgg_register_event_handler('login', 'user', [
'handler' => function ($e, $t, \ElggUser $user) {
// logic
},
'name' => 'myPlugin:user login',
'priority' => 100,
]);
elgg_unregister_event_handler('login', 'user', ['name' => 'myPlugin:user login']);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just make people use a class instead:
class MyPlugin\User\Login {
public function __invoke($e, $t, \ElggUser $user) {
// logic
}
}
elgg_register_event_handler('login', 'user', Login::class, 100);
elgg_unregister_event_handler('login', 'user', Login::class);
My basic understanding of the situation is:
- anonymous functions cannot be unregistered without adding extra API
- the possible APIs for anonymous functions are either:
- so painful that we can't justify introducing them, or
- require extra work on the part of the plugin dev such that they might as well not use anonymous functions in the first place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not particularly excited when folks use anonymous functions for
callbacks. It takes 5 seconds to come up with a function name or a class
method that can be properly referenced and unregistered. I would instead
enforce string/array callables only for hook/event handlers.
On Mar 17, 2015 7:00 PM, "Evan Winslow" notifications@github.com wrote:
In docs/design/events.rst
#8071 (comment):
- elgg_unregister_plugin_hook_handler('foo', 'bar', $matcher);
+Since anonymous functions have no identifiable names, you must match purely based on the file and line numbers.
+Since this may change over time, it's recommended to verify that the unregister function returnedtrue
.
+
+.. code:: php
+
- elgg_register_plugin_hook_handler('foo', 'bar', function ($hook, $type, $value, $params) {
return 42;
- }); // line 123 of mod/my_plugin/start.php
- // ... elsewhere
- // we match a function that ends between lines 115 and 125:
- $matcher = new Elgg\CallableMatcher('function /mod/myplugin/start.php:120+-5');
Just make people use a class instead:
class MyPlugin\User\Login { public function __invoke($e, $t, \ElggUser $user) { // logic }}elgg_register_event_handler('login', 'user', Login::class, 100);elgg_unregister_event_handler('login', 'user', MyPlugin\User\Login::class);
My basic understanding of the situation is:
- anonymous functions cannot be unregistered without adding extra API
- the possible APIs for anonymous functions are either:
- so painful that we can't justify introducing them, or
- require extra work on the part of the plugin dev such that they
might as well not use anonymous functions in the first place—
Reply to this email directly or view it on GitHub
https://github.com/Elgg/Elgg/pull/8071/files#r26598562.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ewinslow Using a classname looks nice, but we need to finish migrating to PHPDI or add support in our DI system. Particularly I like that code is lazy loaded and the IDE can reliably jump to the class. This is a problem when using syntax like __NAMESPACE__ . '\function_name'
.
If we choose not to support removing dynamic handlers (closures, dyn method callback, invokables) do we also issue a notice if one of them is registered to nudge devs away from this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I like this, seems that whatever benefits there may be to registering anonymous functions are completely offset by the difficulty of unregistering them. I would rather enforce it as a string function name or static function |
This adds a class usable for matching anonymous functions, dynamic method callbacks, and invokable objects without the need for an object reference. The matcher can be passed to the event/hook unregister functions to successfully remove such types of handlers.
This also renames the event registration test so it’s actively tested by PHPUnit.
Fixes #7750
Fixes #8069