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

Feature/Eventmanager #86

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ClemensSahs
Copy link
Collaborator

here is a simple EventManager to customize the SeoStats logic for the own business.

please give me some feedback to this idea, currently this is not tested.

// usage add listener
$em = \SEOstats\Helper\EventManager::getInstance();
$em->add('google::gCurl::pre_exec',function($event) {
  // some magic
});
$em->add('google::gCurl::post_exec',function($event) {
  // some magic
});
// logic in Services
$em = \SEOstats\Helper\EventManager::getInstance();

$e = new \SEOstats\Helper\Event();
$e->setProp('curlHandle', $ch);

$em->trigger(__METHODE__ . '::pre_exec', $e);

@jakubsacha
Copy link

it looks okey. but those singletons everywhere. not nice.

I think that whole package needs to be rebuild (remove statics etc, refactor API). for this what we have now your code looks okey.

It would be nice to use observer pattern in this case and inject dependencies instead of creating them inside of classes.

also curl "caller" should be separate class imho.

@ClemensSahs
Copy link
Collaborator Author

Yes, sure this will only the first step, to customize this library but it give some more abilities. After version 2.5.3 is released. I will create a 3.x branch and refactor the full API with a full new created Codebase.

Singleton is not the best "design pattern", I think its a "Anti-Pattern", too. But we use currently static::calls, too. So in that case Singleton is the right pattern.

In my mind this PR is not for 2.5.3 it will better for a 2.6.x. in the case it gives a 2.6 version.

@jakubsacha
Copy link

Dont you think this event manager will be just waste of time? After API refactoring you'd have to rebuild it again...

@ClemensSahs
Copy link
Collaborator Author

Don't understand me wrong, yes we need a big refactoring of this library.

But I think this feature can give user a new ability to customize this library.
If this is a Feature will come to 2.6.x or 3.x or never is not my decision.
For exactly this case we have this discussion.

And in case this will come never it cost me only 20 minutes code writing ;)

@jakubsacha
Copy link

you should definitely implement it!

what i wanted to do is to enable proxy support for google cals. this would allow me to do that. Problem would be with handling connection errors and retrying with different proxy.

@ClemensSahs ClemensSahs added this to the 2.6.0 milestone Jul 24, 2014
@ClemensSahs
Copy link
Collaborator Author

I set the millstone 2.6.x for this.

requirements for 2.6 is:

  • easy to implement in the current classes
  • php 5.3+

requirements for version 3:

  • php 5.4+
  • no singelton
  • no massive usage of static::calls

@mickaelandrieu
Copy link

hi,
I'm the creator of http://www.seo-tracker.net and I have a (little) dependency of this package.
Can I suggest the use of Symfony event dispatcher ? or @igorw's ? https://github.com/igorw/evenement

The point is "you don't need to make your own".

@ClemensSahs
Copy link
Collaborator Author

That is a good point. But if we add symfony we must care ZF2, too.

I will add Facade-API class to add both

@mickaelandrieu
Copy link

I'm not sure you need facade for it, maybe an interface can be sufficient if you want your users to extend your event dispatcher, but this is a good idea to provide connectors for the most popular frameworks (for Sf2 let me know if you need help :) )

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

Successfully merging this pull request may close these issues.

3 participants