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

Custom responseClass creation through the operation.parse_class event #380

Closed
wants to merge 2 commits into from

Conversation

mtdowling
Copy link
Member

This PR attempts to address #375 in a way that does not require custom domain specific objects to be implemented in any specific way. If you need to create custom domain specific objects to create a custom responseClass object from a command result, you can listen to the operation.parse_class event of a client object. The listener should call the setResult() method of the event to set the create object and automatically stop event propagation. This gives the added flexibility of registering multiple listeners that can defer creation to the next listener.

So here's an example of how you'd register a listener to create custom domain specific objects:

$client = new Client();
$client->setDescription(ServiceDescription::factory(array(
    'operations' => array('test' => array('responseClass' => 'FooBazBar'))
)));
$foo = new \stdClass();
$client->getEventDispatcher()->addListener('operation.parse_class', function ($e) {
     // Determine what to actually create for the command
     if ($e['command']->getName() == 'test') {
         $e->setResult(createTheObject($e['command']));
     }
});

Any thoughts on this vs #375?

@Danack
Copy link
Contributor

Danack commented Jul 13, 2013

Well it certainly looks simpler, and by not listing the factory it also avoids exposing the implementation of the client in the service description which is a good thing.

I'll switch my project across to use this branch to see if it's as easy to use, or if there's any hidden gotchas when actually using this.

@mtdowling
Copy link
Member Author

Thanks for trying it out, Dan!

@Danack
Copy link
Contributor

Danack commented Jul 14, 2013

Yeah, that works just fine, it only took me a few minutes to move my code across to doing it that way.

My only comments would be, and I realise these are opinions rather than absolutes:

i) It'd be better to define the event name as a const in one of the Guzzle classes rather than having it be just a string.

ii) The event name is probably wrong at the moment as it's not 'parsing a class', but is 'creating a object from the response'. So maybe just 'response_parse' ?

But yeah it works just as well as my pull request but with less code and less coupling, so I'll close that pull request.

cheers
Dan

@mtdowling
Copy link
Member Author

Good points, Dan. I'll add a constant and look into renaming the event name to something more appropriate.

@mtdowling mtdowling closed this Jul 14, 2013
@Danack
Copy link
Contributor

Danack commented Jul 23, 2013

Hi,

Erm - it looks like the branch you did this work in hasn't been merged into master yet. Any chance you can do that, and define the constant so I can avoid the magic string for the event name?

cheers
Dan

@mtdowling
Copy link
Member Author

It's merged: https://github.com/guzzle/guzzle/blob/master/src/Guzzle/Service/Command/OperationResponseParser.php#L95. I rebased first though so it doesn't look like it is. I don't think I will define a one-off constant for the event because no other event constants are defined in the Service namespace. I think it would be better to address this in a more generic way to collect event names, so I'm going to sit on that till I think of something.

@Danack
Copy link
Contributor

Danack commented Jul 23, 2013

Ah sorry, it's merged just not tagged.

Hmm, btw dev-master has some changes that re-break the Oauth for Flickr, so I'll try and send a pull-request for that and maybe we can have a tag after that?

@mtdowling
Copy link
Member Author

Thanks for letting me know. It would be awesome if you could send a PR that also includes a test case to make sure this doesn't happen again. I'll tag soon. I was hoping to resolve #349 first if possible.

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.

2 participants