ResponseFactory alternative to response objects needing Guzzle interface

[Danack]

Hi Michael,

Here is a pull request to add a 'responseFactory' option to both the service description and individual operations. There's a couple of things which probably need reviewing before merging.

1. Should the option be called be responseFactory or responseFactoryClass?

I went with responseFactory as it's shorter but it looks slightly odd when calling setResponseFactory and passing in a string rather than a class instance.

2 .There's a couple of bits of code that were already a bit 'hairy' which have gotten even more complicated.

Operation::inferResponseType() is one and in Operation::__construct() the lines:
if (!$this->responseClass && !$this->responseFactory) {

and below are definitely quite complicated. Should I attempt to refactor them?

1. Non-trivial getter. The function Operation::getResponseFactory() isn't a simple getter, but looks at whether the service description for the Operation has a default ResponseFactory set. Is that within the Guzzle coding standards or should it either be called something else, or coded differently?

I've added some unit tests for the added functionality, but I'm not a unit tester expert so if you think there's any other tests that are missing please let me know.

cheers
Dan

[mtdowling]

It looks like this part and the section of code below this are very similar. Can you take a crack at refactoring this to remove some of the code duplication? I think it would be fine to remove the ResponseFactoryException and just throw a ResponsClassException for both error cases.

[mtdowling]

Thanks for the PR! This looks like a really good start. I have a couple requests:

1. Can you remove the default responseFactory setting from the ServiceDescription object please? We already have a way to extend one operation from another, so I don't think we need to add this Guzzle's API as it adds unneeded complexity.
2. Can you remove the closing PHP tags and the extra new lines please?

Should the option be called be responseFactory or responseFactoryClass?

responseFactory is good.

There's a couple of bits of code that were already a bit 'hairy' which have gotten even more complicated.

I can take a look at the inferResponseType() method after the PR is merged to attempt to clean it up a bit. I'd like the most common cases checked first, followed by the least common.

I've added some unit tests for the added functionality, but I'm not a unit tester expert so if you think there's any other tests that are missing please let me know.

The unit tests look good. I'll take a closer look before merging and after these changes are completed.

[mtdowling]

@Danack I've been thinking about this implementation and had a different train of thought. This implementation still enforces tight restrictions on how domain specific objects are created. I am going to submit a PR that implements custom response class creation using an event based system rather than adding a new custom attribute to the service description. Let me know what you think.

[Danack]

Moot.