callable on exception shows an error.
Closed this issue · 13 comments
When we use a callable that throws an exception we have an error, that occurs because we are running it inside the __toString(); method.
Oops that's a big one.
The error you are referring to is the toString must return a string, am I correct?
toString cannot throw exceptions!
public function __toString()
{
throw new Exception('Fatal error'); // PHP Fatal Error
}But where would it go in any case?
Isn't this where we want to translate it into a 5xx type HTTP header and let the user know about it.
I was under the impression that was what we were going for here.
It should best be dealt with before it gets to toString I would imagine. I'd hate to see a try catch in there for whatever reason. What do you think @wesleyvicthor ?
That is what the documentation suggests:
Respect\Rest provides two special ways to handle errors. The first one is using Exception Routes:
$r3->exceptionRoute('InvalidArgumentException', function (InvalidArgumentException $e) {
return 'Sorry, this error happened: '.$e->getMessage();
});Whenever an uncaught exception appears on any route, it will be caught and forwarded to this side route.
The exception should be caught and you get to respond about it. This is what you are referring to right?
Sorry, I forgot to reference the commit to this issue.
here you can see it.
When using the router as a controller $router->any('/', array('MyController')) we have the liberty to do whatever we want, even throw exceptions.
But if we throw an exception in the controller we have a Fatal Error because the method that dispatch the execution is __toString();
I like the idea of translate it to 5** HTTP error.
But I don't like to print the object instead of just call the right method.
I'me not following, if you are geting the error from that change it is not from an exception but because you are not returning a string anymore. __toString requires that you return a string.
__toString is the correct method but we're not calling it PHP is calling it for us.
@nickl- doesn't matter if my method returns a string, if before it throws a exception I will get the error anyway.
did you get the point ?
The __toString is only a syntatic sugar, the alternative way of running the route was always there to be used. Why we removed the entire __toString() method instead of just changing it's callee? Exceptions are optional in PHP, there can be a lot of simple routers that doesn't handle exceptions and can benefit of the __toString() straightforwardness.
We also removed a critical test that ensured that our README instructions are up-to-date, now our user manual is broken.
Well, I vote for restoring the __toString() method (for both the API cleaness and BC with our previous users), review our internal code to make sure that internally we only use run() instead of type juggling and create a special note regarding exceptions on our README. What do you guys think?
+1 for sugar... I'm also open to spice =)
If the exceptions are ours we have a responsibility to deal with them appropriately.
Even though we are not obliged to deal with exceptions from the route handlers, this onus lies on the developer implementing the behaviour.
That said the possibility remains that this may happen and allowing errors or exceptions to slip past is not useful in any way, in context of a REST service considering.
This however, I regard as part and purpose of the #41 implementation and responding appropriately in HTTP what PHP may do differently if unattended.
@wesleyvicthor am I understanding the point correctly?
It may be fair to add:
As per the manual the purpose of __toString
The __toString() method allows a class to decide how it will react when it is treated like a string. For example, what echo $obj; will print. This method must return a string, as otherwise a fatal E_RECOVERABLE_ERROR level error is emitted.
So if we were to claim, as we do, that we don't attempt to change the way PHP is meant to be then we are probably abusing this functionality and it would make more sense to return a printout (string representation) of the RouteInspector, would you agree? Executing the process is by no means a string representation of the instance, which is what I would expect to see if I treat an object as a string, agree?
Yet "deciding how it will react" is still our choice so maybe we are not abusing the purpose. Then it remains to be a question practice. I was not prive to the conversations leading up to the strategy to make these changes, can someone fill me in please or is there somewhere I can reference the discussion online? I do somehow feel I am missing the point as @wesleyvicthor points out which may be due to lack of information.
This is a case only for during development. The exception inside the __toString() isn't traceable and it seems ugly, but no one with conscience would publish an application that displays uncaught exceptions, neither PHP errors, and for that purpose the previous __toString() implementation was OK.
Considering people can use the Router as a component, it's obviously better to use ->run() directly. We should mention this right in the beginning of our docs.
In previous issues we discussed something that would mitigate this problem: a default error handler that shows up friendly errors and spit out pretty logs before PHP explodes. I would go for that, even implemented the error and exception handlers already =D