eyecatchup / SEOstats

SEOstats is a powerful open source PHP library to request a bunch of SEO relevant metrics.
MIT License
1.46k stars 384 forks source link

Feature/Eventmanager #86

Open ClemensSahs opened 9 years ago

ClemensSahs commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

ClemensSahs commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

I set the millstone 2.6.x for this.

requirements for 2.6 is:

requirements for version 3:

mickaelandrieu commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 :) )