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

Allow to bind closures to the EventDispatcher #1951

Closed
wants to merge 10 commits into from
Closed

Allow to bind closures to the EventDispatcher #1951

wants to merge 10 commits into from

Conversation

nicoschoenmaker
Copy link

There are right now two ways that I know of that are possible to allow to get a trigger in a custom installer

  • in the __destruct() (ugly)
  • in the supports function (example). Less ugly, but still not nice.

I would like to be able to subscribe to events because I'm building a custom installer that does some code generation based on the contents of the to-be-installed package. Part of the code that I generate depends on the combination of the installed packages.

i.e. I generate some "glue" between them. So I need a hook where I can get the complete list of packages, after they have all been installed.

My gut feeling told me to bind to the EventDispatcher, but there was no possibility for that. Hence this pull request + unit tests :)

@RobLoach
Copy link
Contributor

Interesting... Would love to see this develop.

if ($this->isPhpScript($callable)) {
if ($this->isCallable($callable)) {
try {
$callable($event);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not the right way to call a callable on PHP 5.3

* @param unknown_type $callable Something we will decide whether it's a callable
* @return boolean
*/
protected function isCallable($callable)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you wrap the simple native call into a method?

Copy link
Author

Choose a reason for hiding this comment

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

ah, heh. That used to be more complicated :) that can be inlined by now

@Seldaek
Copy link
Member

Seldaek commented Sep 25, 2013

I think a better fix now that we have plugins would be to modify the plugin system to allow hooking into any existing "script" event from plugins. Any comments on this @naderman?

@nicoschoenmaker
Copy link
Author

Seems the same / very similar thing has been done in 3960edd

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

Successfully merging this pull request may close these issues.

None yet

5 participants