Easily throw own exceptions? about command HOT 15 CLOSED

The event system is a little different in v4. One of the main differences is that the command events are at a level above the request events. Instead of listening to the request's error event, you'll want to listen to the command's error event, and apply your logic there.

// Barebones concrete service client
class Client extends GuzzleHttp\Command\AbstractClient {
    public function getCommand($name, array $args = []) {
        return new \GuzzleHttp\Command\Command($name, $args, clone $this->getEmitter());
    }
}

// Create HTTP client that will 404
$http = new GuzzleHttp\Client;
$http->getEmitter()->attach(new \GuzzleHttp\Subscriber\Mock([new \GuzzleHttp\Message\Response(404)]));

// Create the service client
$client = new Client($http);
// Attach simple request building logic
$client->getEmitter()->on('prepare', function (GuzzleHttp\Command\Event\PrepareEvent $event) {
    $event->setRequest($event->getClient()->getHttpClient()->createRequest('POST'));
});
// Attach error handling logic
$client->getEmitter()->on('error', function (GuzzleHttp\Command\Event\CommandErrorEvent $e) {
    throw new YourCustomException("FOO!");
});

// Execute a command
$result = $client->foo();

from command.

Hi,

IT does not seem to solve the issue (see here: https://github.com/zf-fr/zfr-mailchimp/pull/21/files)

Even if I listen to the client error event, it's still wrap around a CommandException :/.

from command.

The problem seems to come from here: https://github.com/guzzle/command/blob/master/src/AbstractClient.php#L87

No matter what, the client wrap it around its own extension :/.

from command.

Ah, yep. I see. Well, I'll talk to @mtdowling when he gets back from his vacation, and see how we want to handle this. guzzlehttp/command is still beta so we can make changes.

from command.

Thanks :).

from command.

The interface says that a CommandException is thrown from the method. Throwing something else would break the interface, so that's why it wraps any other thrown exception. Could you just extend CommandException with a subclass to throw a custom exception?

from command.

Hmmm that's really annoying to be honest. Extending CommandException is not convenient at all, it's too restrictive. Please bring back the Guzzle 3 behaviour here :). Not being able to trigger custom exception in listener is really a -1 from me. I used that all the times in all my API wrappers.

from command.

How is it annoying or restrictive? You already need to extend \Exception, so extending CommandException shouldn't be an issue. If that's not an option, then you could wrap the Guzzle service client and throw your own custom exception based on the CommandException.

I'm hesitant to change how this works because lots of the implementation expects CommandExceptions (e.g., the batch() method, the command event system, etc.). The current plan was that if you throw an exception in an event that does not extend from CommandException, then that exception will not be emitted as part of the event system, basically eliminating the ability to send commands in parallel. By emitting events for exceptions thrown during a command transfer, it allows you to handle the error using a CommandErrorEvent without actually throwing the exception up to the original caller (allowing for async error handling).

I'm open to suggestions on this stuff as it's under heavy development.

from command.

This is inconvenient because of that: https://github.com/guzzle/command/blob/master/src/Exception/CommandException.php#L32

Way too much parameters to give, and most of the time, I really don't care about all this when I catch exceptions. Here is an example of an exception class I have for MailChimp (built around Guzzle 3): https://github.com/zf-fr/zfr-mailchimp/blob/master/src/ZfrMailChimp/Exception/RequestTimedOutException.php

As you can see, I have TONS of exceptions: https://github.com/zf-fr/zfr-mailchimp/tree/master/src/ZfrMailChimp/Exception for all the exceptions that may be triggered by MailChimp.

I really want to keep my exceptions simple, and while in some cases I want to have the Response, in case of this integration for example, there are really an exception for every possible error so it's not necessary to have access to responses, commands...

I'd suggest that you create instead a simple CommandExceptionInterface that does not enforce anything. I could therefore implement the interface for my exceptions without having to implement the lengthy constructor.

from command.

I also realized this lengthy constructor does not allow to set a specific code in the constructor, that I often use.

from command.

Way too much parameters to give, and most of the time, I really don't care about all this when I catch exceptions.

The constructor is being trimmed down quite a bit in a branch I'm working on. I've added a "transaction" object that contains all of the dependencies of command events, exceptions, etc. You can now easily throw a CommandException from within an event by grabbing the transaction from the event and passing that to the constructor of a CommandException. So, assuming you want your exception to work its way through the Guzzle event system, you could throw an exception like this:

$command->getEmitter()->on('prepare', function (PrepareEvent $e) {
    throw new CommandException('Error!', $e->getCommandTransaction());
});

I think this change addresses the "way too many parameters" argument, and makes it much easier to mock CommandExceptions.

I also realized this lengthy constructor does not allow to set a specific code in the constructor, that I often use.

I should add this to the constructor I suppose.

I really want to keep my exceptions simple, and while in some cases I want to have the Response, in case of this integration for example, there are really an exception for every possible error so it's not necessary to have access to responses, commands...

Maybe I should explain why I designed the exceptions this way. Guzzle allows you to recover from exceptions using an event system so that you can send requests in parallel. It also allows you to batch commands and then work with the results of each command execution after the batch of commands has been sent. If you don't handle exceptions in an event or using batches, then the same exception type is thrown, allowing you to recover from an error using exceptions. In order to make it possible to actually handle and recover from web service errors, the CommandException provides access to the client, command, request, and response. This allows you gracefully handle the exception: retry the command, log errors as needed, etc.

With that said, maybe trimming down the CommandException could make sense. If we did this, we could utilize an interface (as you suggested) to tell the event system what should be caught and sent through the event system vs what should be thrown immediately. In this case, you'd still be able to handle command errors using events and you'd have access to all of the transactional data (request, response, command, client, etc.). The interface of ServiceClient could then just have a ``@throws CommandExceptionInterface`. If someone chose not to throw a CommandExceptionInterface, then it wouldn't be wrapped (but they'd be breaking the contract of the interface). If we went with this approach, then in AWS we'd likely extend this method with a more explicit exception type that provides AWS specific error information.

You don't think a CommandException interface would need any methods?

Regardless of whether or not the exception class is changed, I think it would be beneficial to add an exception factory method to clients that would be able to create custom exceptions (or subclasses of CommandException) when an error occurs during a request transfer. It would be a new method on ServiceClientInterface that accepts a CommandTransaction and returns an Exception.

from command.

I think erring on the side of flexibility is the right way to go here, so I've pushed a commit to the WIP branch that forces CommandExceptionInterface rather than CommandException (it's just a blank interface).

d2cc372

1. If an exception other than CommandExceptionInterface is thrown, then Guzzle will still wrap the exception in a CommandException so that the interface is adhered to.
2. Added a new method on ServiceClientInterface that is responsible for creating CommandExceptionInterface exceptions when an error occurs while transferring the command over the wire (this can be overridden to throw exceptions specific to your library).
3. If an exception is thrown at any other point during the transfer of a command that implements CommandExceptionInterface, then it will be emitted through the event system. Other exceptions will just be thrown immediately.

It's still a WIP and completely untested. What do you think about each item I've listed above?

from command.

That looks better to me and it goes to the right direction!

I'm still unsure about the concept though. You told me that:

If you don't handle exceptions in an event or using batches, then the same exception type is thrown, allowing you to recover from an error using exceptions. In order to make it possible to actually handle and recover from web service errors, the CommandException provides access to the client, command, request, and response.

However, CommandExceptionInterface cannot guarantee that because it is an empty interface (which is good as it prevents me from creating complex exceptions in my case). Therefore your point that justify the need of CommandException is not valid anymore (because most of my exceptions, even if I implement CommandExceptionInterface, won't give you the client, command, request...).

I'd say that you should go the easy path: remove the ExceptionInterface, let anyone trigger their own exception WITHOUT wrapping them around CommandException, and it appears that you encounter a CommandException explicitly, then you have your contract to perform the additional logic you're talking about. But as it is now, I can't get the point of the added complexity :/

from command.

The interface is there so you can know what to try/catch when making a call.

On Jul 10, 2014, at 5:10 AM, Michaël Gallego [email protected] wrote:

That looks better to me and it goes to the right direction!

I'm still unsure about the concept though. You told me that:

If you don't handle exceptions in an event or using batches, then the same exception type is thrown, allowing you to recover from an error using exceptions. In order to make it possible to actually handle and recover from web service errors, the CommandException provides access to the client, command, request, and response.

However, CommandExceptionInterface cannot guarantee that because it is an empty interface (which is good as it prevents me from creating complex exceptions in my case). Therefore your point that justify the need of CommandException is not valid anymore (because most of my exceptions, even if I implement CommandExceptionInterface, won't give you the client, command, request...).

I'd say that you should go the easy path: remove the ExceptionInterface, let anyone trigger their own exception WITHOUT wrapping them around CommandException, and it appears that you encounter a CommandException explicitly, then you have your contract to perform the additional logic you're talking about. But as it is now, I can't get the point of the added complexity :/

—
Reply to this email directly or view it on GitHub.

from command.

@bakura10 I've updated my update to the command layer to not require a CommandInterface or wrap exceptions. This basically pushed the burden onto consumers of the library to ensure that any exception emitted by their library are consistent (e.g., in AWS we would wrap any uncaught random exception to provide predictable error handling). I think this makes for a good solution and cleans up the implementation a bit. I know it's a huge PR, but how does #12 look to you?

from command.