Added Response Factory #73
Conversation
|
It took me a while, but I think I understand what you are trying to achieve now. GoalsFrom the description and the code I can see two objectives:
Is this correct? CommentsNow, assuming it is, I have a few comments about the general approach. First, a lot is hard coded here, which I believe will prevent achieving the second objective. This has especially to do with the fact that:
Secondly, the response class to be used for creating a specific response is set on the message object. I may be wrong, but I think that you try to achieve that a command of type Alternative suggestionGiven these comments, I suggest the following approach:
Example interfaceAs an example of the interface, I would suggest the following. interface IResponseFactory
{
/**
* @param string $commandClass The class name of the command
* @param string $responseClass The class name of the response
*
* @return void
* @throws InvalidArgumentException when the response class is not found.
*/
public function registerReponseClassForCommand($commandClass, $responseClass);
/**
* @param OutgoingMessage $originalMessage The message that was originally sent
* @param string $rawResponseMessage The raw array received from Asterisk
*
* @return ResponseMessage
*/
public function createReponseForCommand($originalMessage, $rawResponseMessage);
}Please do not bother with commenting on the naming of things. I named things to make this suggestion as clear as possible, not to be how PAMI does things now. |
|
Hi Jacob, Without the example uses of the change it might be a little harder to see the benefit. Goals:Basically yes. It will look for a Response Handler with the the same name as the Action first, if not found it will use the GenericReponse handler. If need be you can override it by setting the ResponseHandler manually when creating the action. For example to point multiple Actions to one ResponseHandler like for example SCCPGenericResponse.php. Comments:Which parts are hardcoded according to you ? I followed the '/Event/Factory/Impl/EventFactoryImpl.php which was already there. This should not really have to be different. I will try to match an Action with a Response and FallBack to GenericResponse.php if not found. You can override this behaviour by setting a ResponseHandler Manually, for example to point multiple Actions to a MinimalResponse or something. I thought this would make things pretty flexible. Alternative Suggestion:I guess that would work as well, and could be benefitial. I hope this does not mean multiple ReponseFactory implementations (although that would be possible using the same interface). I did not want to change to much of the original design and only suggest a possible implementation (was mostly focussed on the response results). There are many ways to reach the same goal. At the moment ClientImpl seems to be able to handle only one outgoing Action message (well) at a time. And my impletation might be reinforing that (which can be seen as bad). (You have the same problem as I do when typing Response , somehow we skip over the first 's' :-) |
|
I've created a pull request on your branch (dkgroot#2), showing what I meant. |
|
But let's keep the discussion going here, and not at the PR on your branch, OK? |
|
I forgot to say what changes. First off, it is still compatible with @dkgroot's approach. But it also give the user a little more flexibility. With my example code, it is possible register actions for a specific message object and for all messages of the same class, using the factory provided in PAMI. If someone still needs more flexibility, it is possible to inject a fully customized Response Factory, as long as it adheres to the IResponseFactory interface. EDIT: typo's |
|
Jacob, thanks for taking the trouble to create an example implementation, that always aids in understanding. As far as i can see it (only) adds a layer of indirection which can/might be a good thing as it will give the opportunity to replace the responseFactory by the end user of the library. The actual original question remains however, which is do 'we' want:
I think these question should be answered before choosing any implementation. I would be absolutely fine with your solution to the problem. You might even want to duplicate this factory interface to the EventFactory as well. There are some events for which it would make sense to handle them using one EventHandler (The ....CloseEvents are all the same, for example). (ie: For the chan-sccp-b there are quite a number of Aktion commands the will generate Events that need to be closed by these ...CloseEvents). Let's keep this for the next PR. |
|
Hi Diederik, Let's start with an even more fundamental question: do we want specific Response messages in the first place? Personally, I would welcome it. At the very least it makes it easier to develop against a specific type, which aids in development (for example, IDE's can provide autocompletion, which is a huge win for me). Given that we do, I would answer your questions as follows:
Let's first see what @marcelog thinks, and whether he has any ideas with regards to implementation. |
|
Waiting for @marcelog to see how we can make progress on this. I have a whole slew of action/repsonses in waiting. There is even someone (@pnlarsson) who created a PR against my repository with PJSIP related actions, events and responses, using the same response factory :-) |
|
@marcelog: Any progress / viewpoints, or just really really busy ? |
|
Any chance of revisiting this MR ? |
|
@marcelog I'd like to see this one get in as well. Could you check this out and respond please? It's waiting for your feedback in order to move forward. Thanks! |
|
@dkgroot @jacobkiers I'm not sure I like how it is right now. Also, there are no further comments from others to have something like this. Since I'm the only one that can merge, you will have to be patient guys and wait for me to have enough time to sit and think about it for a bit (so far, I couldn't). Perhaps I'd like to have a different implementation, or maybe a few adjustments would do the job. It's not necessary to insist by adding more comments here: it's in my queue, and that's all I can say right now. I know I'm being the bottleneck, but this is actually how open source works sometimes: out of the free time and good will of others (I did too have pull requests of mine open for months, some of them got merged, some are not even reviewed yet, and I trust that someone will eventually have the time to review them). Unfortunately, PAMI will not pay my bills (or anything, since it's just a hobby and a way to contribute something to others) so other things get my attention sometimes. Thanks for understanding! |
|
Hi Marcelo, On 24-08-15 13:11, Marcelo Gornstein wrote:
Regards, Diederik
|
|
Thanks, and sorry if I seemed to be a little "loud" there. I didn't reply before because I don't actually have a better thing to say other than "I'm not convinced", so I wanted to sit and look at the specific problem of how you are getting the responses (the forest) instead of the tree (the response factory implementation). I always try to point out specific things and improvements. So far I don't have them. Do you know if this might be needed for other things than chan-sccp-b? Can you give some specific examples of the issues you want to solve when dealing with chan-sccp-b with this pull? (I understand the format of the responses change, but how exactly?) Thanks! |
|
No problem at all. pnlarsson used the extended implementation to implement some PJSIP parsers also using the Response Factory, see: By having the factory you can move some of the post message processing into PAMI, making it easier to use PAMI as a library, understanding the differences between series of AMI output. PJSIP and SCCP have partially digressed/extended the sometimes minimal output from an AMI command. For example in the case of SCCP a command like 'SCCPShowDevice' will return a message header with multiple sub lists, which standard AMI does not really have a natural format for. We used what was there, but the post-processer has to know and expect what we are sending. Example:Response: Success Event: SCCPShowDevice Event: TableStart Event: SCCPDeviceButtonEntry Event: SCCPDeviceButtonEntry Event: TableEnd Event: TableStart Event: SCCPDeviceLineEntry Event: TableEnd Event: TableStart Event: SCCPDeviceSpeeddialEntry Event: TableEnd Event: TableStart Event: SCCPDeviceFeatureEntry Event: TableEnd Event: TableStart Event: TableEnd Event: TableStart Event: SCCPDeviceStatisticsEntry Event: SCCPDeviceStatisticsEntry Event: TableEnd Event: SCCPShowDeviceComplete ListTableItems: 6Because a little extended knowledge is expected about the returned information, it will be easier for me to add the post processing into PAMI, than it might be for someone else to do it in their source code using PAMI. PAMI as is will be able to parse and pass-on the information provided, but the enduser(s) will have to implement their own methods to use the information. I tried adding the factory in such a way that non of the other messages/events would have to be touched/changed. I hope this somewhat long winded explanation helps in understanding my objective(s) for the change. |
| @@ -402,6 +416,10 @@ public function send(OutgoingMessage $message) | |||
| ); | |||
| } | |||
| $this->_lastActionId = $message->getActionId(); | |||
| // If there are multiple outgoing messages in flight, we might have to add this information to a queue instead, like: | |||
There was a problem hiding this comment.
Let's remove the commented code if it's not used
|
Would you like me to make these corrections before discussing any further progression, or leave it until you have had a change to deliberate if you would like to see something like this in your source base ? No stress, take your time. |
Added ResponseFactory
Added logging to this factory classes, so we can see what it produces. Might be usefull in the Event Factory as well.