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

Allow anonymous functions and dynamic methods to be unregistered #7750

Closed
mrclay opened this Issue Jan 4, 2015 · 10 comments

Comments

Projects
None yet
6 participants
@mrclay
Member

mrclay commented Jan 4, 2015

We all really love anon functions right up until you need to unregister one used as a handler in another plugin. I propose we allow a special syntax for matching an anonymous function in these unregister functions, e.g.

// matches any Closure defined in that file
elgg_unregister_blah('Closure in mod/foo/start.php');

// matches an [$barInstance, 'bing']
elgg_unregister_blah('\Bar->bing');

Since this requires Reflection it wouldn't be super fast, but it's not a common need anyway.

@mrclay mrclay added the dev usability label Jan 4, 2015

@mrclay mrclay referenced this issue Jan 4, 2015

Closed

Pr/7748 #7749

@Srokap

This comment has been minimized.

Show comment
Hide comment
@Srokap

Srokap Jan 4, 2015

Contributor

I actually imagined that at some point we could have registration api exposed by particular "core app" instance to the plugin and this way we could mark anonymous function by instance or just look at stacktrace (doesn't sound fast either).

Alternative would be to require for now additional parameter "function name" on registration which would be just a logical closure identifier for unregistering. This way we would at least not create global function, but that's not so cool solution.

Maybe some solution would be flipping whole thing around and marking events for particular plugin that are not supposed to be registered by it and just ignoring it during registration. That would make sense as plugin dependency similar to plugin order. It still requires knowing which plugin is just calling the api.

I'm not convinced entirely about any solution yet.

Contributor

Srokap commented Jan 4, 2015

I actually imagined that at some point we could have registration api exposed by particular "core app" instance to the plugin and this way we could mark anonymous function by instance or just look at stacktrace (doesn't sound fast either).

Alternative would be to require for now additional parameter "function name" on registration which would be just a logical closure identifier for unregistering. This way we would at least not create global function, but that's not so cool solution.

Maybe some solution would be flipping whole thing around and marking events for particular plugin that are not supposed to be registered by it and just ignoring it during registration. That would make sense as plugin dependency similar to plugin order. It still requires knowing which plugin is just calling the api.

I'm not convinced entirely about any solution yet.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Jan 4, 2015

Member

I think we should consider this a feature. If you want the function to be
unregisterable, that is extra API that you explicitly expose by naming the
function.

On Sun, Jan 4, 2015, 1:22 PM Paweł Sroka notifications@github.com wrote:

I actually imagined that at some point we could have registration api
exposed by particular "core app" instance to the plugin and this way we
could mark anonymous function by instance or just look at stacktrace
(doesn't sound fast either).

Alternative would be to require for now additional parameter "function
name" on registration which would be just a logical closure identifier for
unregistering. This way we would at least not create global function, but
that's not so cool solution.

Maybe some solution would be flipping whole thing around and marking
events for particular plugin that are not supposed to be registered by it
and just ignoring it during registration. That would make sense as plugin
dependency similar to plugin order. It still requires knowing which plugin
is just calling the api.

I'm not convinced entirely about any solution yet.


Reply to this email directly or view it on GitHub
#7750 (comment).

Member

ewinslow commented Jan 4, 2015

I think we should consider this a feature. If you want the function to be
unregisterable, that is extra API that you explicitly expose by naming the
function.

On Sun, Jan 4, 2015, 1:22 PM Paweł Sroka notifications@github.com wrote:

I actually imagined that at some point we could have registration api
exposed by particular "core app" instance to the plugin and this way we
could mark anonymous function by instance or just look at stacktrace
(doesn't sound fast either).

Alternative would be to require for now additional parameter "function
name" on registration which would be just a logical closure identifier for
unregistering. This way we would at least not create global function, but
that's not so cool solution.

Maybe some solution would be flipping whole thing around and marking
events for particular plugin that are not supposed to be registered by it
and just ignoring it during registration. That would make sense as plugin
dependency similar to plugin order. It still requires knowing which plugin
is just calling the api.

I'm not convinced entirely about any solution yet.


Reply to this email directly or view it on GitHub
#7750 (comment).

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Jan 5, 2015

Member

To most devs it's an unexpected outcome of using a nicer style, not a conscious decision to disable unregistering. In principle Elgg should nudge devs away from static code rather than imposing costs on those preferring OO/functional. I'll try to convince with a PR.

Member

mrclay commented Jan 5, 2015

To most devs it's an unexpected outcome of using a nicer style, not a conscious decision to disable unregistering. In principle Elgg should nudge devs away from static code rather than imposing costs on those preferring OO/functional. I'll try to convince with a PR.

@beck24

This comment has been minimized.

Show comment
Hide comment
@beck24

beck24 Jan 5, 2015

Member

Having something be unregisterable seems like a bad idea, you can't predict what future devs will need, I think it would just lead to people forking things unnecessarily.

Member

beck24 commented Jan 5, 2015

Having something be unregisterable seems like a bad idea, you can't predict what future devs will need, I think it would just lead to people forking things unnecessarily.

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Jan 7, 2015

Member

Personally would prefer to nudge toward registering classes instead of
functions. Then we could add di support. Closures still slightly nicer in
many cases, but I maintain that anonymity is a feature. You should not be
able to reference the function in a global way.

On Mon, Jan 5, 2015, 8:57 AM Matt Beckett notifications@github.com wrote:

Having something be unregisterable seems like a bad idea, you can't
predict what future devs will need, I think it would just lead to people
forking things unnecessarily.


Reply to this email directly or view it on GitHub
#7750 (comment).

Member

ewinslow commented Jan 7, 2015

Personally would prefer to nudge toward registering classes instead of
functions. Then we could add di support. Closures still slightly nicer in
many cases, but I maintain that anonymity is a feature. You should not be
able to reference the function in a global way.

On Mon, Jan 5, 2015, 8:57 AM Matt Beckett notifications@github.com wrote:

Having something be unregisterable seems like a bad idea, you can't
predict what future devs will need, I think it would just lead to people
forking things unnecessarily.


Reply to this email directly or view it on GitHub
#7750 (comment).

@brettp

This comment has been minimized.

Show comment
Hide comment
@brettp

brettp Jan 7, 2015

Member

Agree that anonymous functions should be able to be unregistered. Not
allowing them to be makes exceptions in the API, making it inconsistent. If
people can't unregister functions through the API, they'll fork or start
mucking around in the services to do it manually.

On Wed, Jan 7, 2015 at 3:05 AM, Evan Winslow notifications@github.com
wrote:

Personally would prefer to nudge toward registering classes instead of
functions. Then we could add di support. Closures still slightly nicer in
many cases, but I maintain that anonymity is a feature. You should not be
able to reference the function in a global way.

On Mon, Jan 5, 2015, 8:57 AM Matt Beckett notifications@github.com
wrote:

Having something be unregisterable seems like a bad idea, you can't
predict what future devs will need, I think it would just lead to people
forking things unnecessarily.


Reply to this email directly or view it on GitHub
#7750 (comment).


Reply to this email directly or view it on GitHub
#7750 (comment).

Brett Profitt
Elgg Lead Developer

Elgg: http://elgg.org/
Skype: brett.profitt
Twitter: http://twitter.com/brettprofitt

Member

brettp commented Jan 7, 2015

Agree that anonymous functions should be able to be unregistered. Not
allowing them to be makes exceptions in the API, making it inconsistent. If
people can't unregister functions through the API, they'll fork or start
mucking around in the services to do it manually.

On Wed, Jan 7, 2015 at 3:05 AM, Evan Winslow notifications@github.com
wrote:

Personally would prefer to nudge toward registering classes instead of
functions. Then we could add di support. Closures still slightly nicer in
many cases, but I maintain that anonymity is a feature. You should not be
able to reference the function in a global way.

On Mon, Jan 5, 2015, 8:57 AM Matt Beckett notifications@github.com
wrote:

Having something be unregisterable seems like a bad idea, you can't
predict what future devs will need, I think it would just lead to people
forking things unnecessarily.


Reply to this email directly or view it on GitHub
#7750 (comment).


Reply to this email directly or view it on GitHub
#7750 (comment).

Brett Profitt
Elgg Lead Developer

Elgg: http://elgg.org/
Skype: brett.profitt
Twitter: http://twitter.com/brettprofitt

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Mar 16, 2015

feature(events): allows wider variety of callables to be unregistered
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

@Srokap Srokap added the in progress label Mar 16, 2015

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay
Member

mrclay commented Mar 16, 2015

PR #8071

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Apr 11, 2015

Member

Consensus seems to be that unregistering anon functions is way too ugly. What do y'all think of allowing Foo::method to match [$foo, 'method'] where get_class($foo) === 'Foo'?

Member

mrclay commented Apr 11, 2015

Consensus seems to be that unregistering anon functions is way too ugly. What do y'all think of allowing Foo::method to match [$foo, 'method'] where get_class($foo) === 'Foo'?

@beck24

This comment has been minimized.

Show comment
Hide comment
@beck24

beck24 Apr 11, 2015

Member

I would be more in favor that

Member

beck24 commented Apr 11, 2015

I would be more in favor that

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 11, 2015

feature(events): allows dynamic method callbacks to be unregistered
This adds a class for matching dynamic method callbacks using static
method syntax.

Fixes #7750

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 11, 2015

feature(events): allows dynamic method callbacks to be unregistered
This adds a class for matching dynamic method callbacks using static
method syntax and uses it to allow unregistering dynamic method callbacks
from events/hooks.

Fixes #7750
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Apr 11, 2015

Member

More limited PR #8181

Member

mrclay commented Apr 11, 2015

More limited PR #8181

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Apr 12, 2015

feature(events): allows dynamic method callbacks to be unregistered
This adds a class for matching dynamic method callbacks using static
method syntax and uses it to allow unregistering dynamic method callbacks
from events/hooks.

Fixes #7750

@jdalsem jdalsem closed this Apr 28, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment