I’m in the process of building a routing system for learning purposes and have encountered an issue which I think is a bit in the grey area of best practices. Can you guys help me decide if this is wrong, okay or could be done in a different way.
When resolving an incoming request and a route resource has been matched the HTTP request method is validated against the route’s supported HTTP methods to ensure its capable of handling such requests. If this is not the case an exception of type UnsupportedMethodException is thrown.
The HTTP status code documentation states that if a request method is denied/unsupported an Allow header containing a list of supported methods should be returned in the response.
10.4.6 405 Method Not Allowed
The method specified in the Request-Line is not allowed for the
resource identified by the Request-URI. The response MUST include an
Allow header containing a list of valid methods for the requested
resource.
So to accommodate this I thought of extending the usage of the otherwise generic exception to require a sequential array of supported methods in its constructor.
class UnsupportedMethodException extends RuntimeException {
/**
* A sequential array of supported HTTP request methods.
*
* @var array $supported
*/
private $supported = [];
/**
* @param string $message The typical exception message.
* @param array $supportedMethods A sequential array of HTTP request methods the matched route supports.
* @param Exception $previous Any previous exceptions
*/
public function __construct($message, array $supportedMethods, Exception $previous = null)
{
parent::__construct($message, null, $previous);
$this->supported = $supportedMethods;
}
/**
* @return array
*/
public function getSupportedMethods()
{
return $this->supported;
}
}
With the usage of type hinting the catch block provides I now have a solid method for fetching the supported methods and generating a proper HTTP response.
So my question is, do you guys think this is a good practice? Something tells me the exception class shouldn’t contain such logic, but I have no proof/reference to back this up.
Storing information related to the exceptional circumstance in the exception is definitely a good thing, as it can allow a handler to fix & retry or fail gracefully. It’s not logic but data, so you don’t have to worry about the exception class being too complex. If the exception took a route and extracted the supported methods, that would be getting too coupled, but taking the methods as an argument is just dumb enough.
Other questions, “How to write a good exception message” and “Why do many exception messages not contain useful details?”, are closely related but focused on the message, rather than other exception fields.
1
If your exception did in fact contain any logic, I would take issue with it, but it does not, so I think it is fine. What it contains is the range (the set, actually) of accepted values, which is not a bad idea. (It might be unnecessary, because the range of accepted values is fixed and known in advance, but not bad at all.)
I think that it should also contain the name of the method that was not allowed, and possibly the URI that failed, since the “Method Not Allowed” response pertains to specific URIs. This information might not be useful for constructing the http response, but it should be useful for logging.
1
Using an exception to be part of program-flow is an anti-pattern, they should not be used for things that you expect to happen – hence the name!
I’d say that practicality trumps such rules however, but here I’d question why you are throwing such an exception and simply not returning the 405 result code – if the method succeeded you wouldn’t throw an exception to return a 200 code, so I don’t see throwing this exception is best practice here.
But, if you are throwing this, then I would call a function in the returning handler that fetches the allowed methods, not pass the data back via the exception.
4