slimphp / Slim

Slim is a PHP micro framework that helps you quickly write simple yet powerful web applications and APIs.
http://slimframework.com
MIT License
11.94k stars 1.95k forks source link

Responses handling #1513

Closed designermonkey closed 8 years ago

designermonkey commented 8 years ago

As I build an app with Slim 3, I have been musing over how the response handling works, and I'd like to put forward an idea (or get my understanding sorted out).

Currently, we have the ability to let a route that is not found in the router return a 404 handler directly, but what about if we can't find relevant data in our data layer? Surely that too is a 404, and should be handled by Slim too?

This got me thinking about these handlers we have. Shouldn't they be partnered with Exceptions? NotFoundException is throwable inside my application, and is 'handled' by Slim to display the registered NotFound handler.

The same would apply to NotAllowed and the generic Error (at least I think Error is already right?)

Exceptions are the only thing that successfully bubble up the chain, and allow me to concentrate more on building the app, than worrying about having to instantiate the right handler.

jensscherbl commented 8 years ago

NotFoundException is throwable inside my application, and is 'handled' by Slim to display the registered NotFound handler.

I handled this by returning a notFoundHandler response in my controller action.

if (!$data) {

    return call_user_func($this->notFoundHandler, $request, $response);
}

This was pre-beta a few month ago and a bit ugly in my opinion, not sure if this is still up to date.

But yeah, what @designermonkey suggests is pretty much what I'd expect as well.

kminek commented 8 years ago

you can easily achieve what you want. set your custom exception handler via set_exception_handler ($this refers to some generic error class which holds reference to app object):

    set_exception_handler([$this, 'handleException']);

and then ALSO use this handler in Slim's callbacks:

    public function errorHandler()
    {
        $self = $this;
        return function ($request, $response, \Exception $e) use ($self) {
            return $self->handleException($e, $request, $response);
        };
    }

this way you'll have single place to handle exceptions and perform desired logic (based on custom exception class type for instance - like RecordNotFoundException or something like that)

designermonkey commented 8 years ago

I understand this, yes, but I am talking about the actual architecture of Slim itself. Error handling is already dealt with in Slim's execution, here: https://github.com/slimphp/Slim/blob/3.x/Slim/App.php#L306-L318

I'm aiming this discussion at having more Exception classes, and expanding the catch list for the relevant exception types, passing them to the right handler during App::runs operation.

Slim's App::__invoke method would also just be required to thrown a NotAllowedException (which would accept the correct allowed methods) or NotFoundException which would also be caught by the catch list and handled accordingly.

I know this could be achieved by hotwiring the errorHandler and doing the handling there, but this seems like it should be a part of Slim itself.

kminek commented 8 years ago

hmmm i like the idea. moreover instead of the need to define 3 handlers (error, notFound, notAllowed) it could be like single service - exceptionHandler which would return correct response :) btw current NotFoundException name is misleading as it suggests HTTP exception instead of DI container exception :P

designermonkey commented 8 years ago

Personally, I would still choose the more handlers over fewer, simply for a single responsibility principle that is in use.

The general errorhandler would have to do more work when overridden in an app of course, but it could farm work out to other handlers for specific use cases.

codeguy commented 8 years ago

All that is necessary in these situations is to return an appropriate response object. In which case, I'd suggest instantiating and returning an appropriate Response instance. This could be made easier with a factory method that simplifies this process. Or we could somehow recycle the existing notFound handler.

jensscherbl commented 8 years ago

All that is necessary in these situations is to return an appropriate response object. In which case, I'd suggest instantiating and returning an appropriate Response instance.

So my example is actually the recommended way?

codeguy commented 8 years ago

It's one way. Not necessarily the recommended way. If we can make this easier, I'm all for it.

jensscherbl commented 8 years ago

It's one way. Not necessarily the recommended way.

What would be the recommended way (what would your code look like for handling this situation)?

designermonkey commented 8 years ago

All that is necessary in these situations is to return an appropriate response object.

The problem I have here is that every aspect of my application needs to be aware of the container so it can get a copy of the relevant handler to create the same output consistently.

IMO, this is bad design practice (passing a container around much like a service locator), which is why I would opt for exceptions, that get caught and handled by the code I would otherwise have to be aware of at a deeper level.

As long as an exception tree has been defined correctly, they can be handled ok at the top of the stack.

As soon as I get a bit of free time this weekend, I will branch an example of what I mean in practice.