Skip to content

added addSubscriber method because guzzle plugins are subscribers #124

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

Merged
merged 2 commits into from
Mar 19, 2014

Conversation

cordoval
Copy link
Contributor

added addSubscriber method because guzzle plugins are subscribers

@@ -82,6 +82,11 @@ public function addListener($eventName, $listener)
$this->client->getEventDispatcher()->addListener($eventName, $listener);
}

public function addSubscriber($subscriber)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be typehinted IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not do it because of the typehint missing in the listener, should i do it, i think is reasonable

Copy link
Contributor

Choose a reason for hiding this comment

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

a listener is a callable, which cannot be typehinted on PHP 5.3. A subscriber is an object implementing EventSubscriberInterface

@cordoval
Copy link
Contributor Author

done @stof
@cursedcoder i know you guys are in difficult times so i will not pressure
but if it is good then thanks so we can work on spreading Gush to the enterprise world

Thanks, if not i will use my own fork on composer

cursedcoder added a commit that referenced this pull request Mar 19, 2014
added addSubscriber method because guzzle plugins are subscribers
@cursedcoder cursedcoder merged commit d328b43 into KnpLabs:master Mar 19, 2014
@cordoval cordoval deleted the addSubscriber branch March 19, 2014 15:23
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.

3 participants