As we start to make use of exceptions in more things, I was wondering
what people thought about this idea:
Should we make class SS_UserException extends Exception, designed to
be used to throw exceptions where the message on that exception is
acceptable to be sent back to end-users?
It would mean that you could do things like this in a form action
handler, without risking that sensitive diagnostic information for
developers is sent back to end users:
function doMyAction($data, $form) {
try {
// something
} catch(SS_UserException $e) {
$form->sessionMessage($e->getMessage(), "error");
$this->redirectBack();
}
// redirect to the success page;
Excellent idea. It would be nice to have a fixed way to pass messages
back to the user that are not necessarily PHP errors. I make good use
of the Forms feature to throw Session Messages of good/bad/warn when
possible, and think that is a good starting model. Having a way to
send "flash" messages back in ajax scenarios would also be great.
One item to consider is baking group security levels in this system.
At the very minimum the ability to message an Admin differently than
Registerd Users vs Anonymous Users would be useful.
Also if discussing a move towards exception based coding would it make
sense to have a way to consistently wrap method calls in try{} blocks
via magic methods?
On Feb 28, 3:50 am, Sam Minnee <sam.min...@gmail.com> wrote:
> As we start to make use of exceptions in more things, I was wondering
> what people thought about this idea:
> Should we make class SS_UserException extends Exception, designed to
> be used to throw exceptions where the message on that exception is
> acceptable to be sent back to end-users?
> It would mean that you could do things like this in a form action
> handler, without risking that sensitive diagnostic information for
> developers is sent back to end users:
> function doMyAction($data, $form) {
> try {
> // something
> } catch(SS_UserException $e) {
> $form->sessionMessage($e->getMessage(), "error");
> $this->redirectBack();
> }
> // redirect to the success page;
I'm not sure I agree. An exception is supposed to occur when something
unforseen or unexpected (...exceptional) occurs. While I think that
better Exception handling would be a good feature in 3.0, I don't
think that routinely using Exceptions to report messages to the user
is a good idea. In the case of form validation, receiving non valid
form data is an entirely expected result so handling with an Exception
is inappropriate.
That is, if an Exception occurs, there is something that needs to be
fixed.
I've also read (but haven't confirmed this) that using a try..catch
block to, for example, catch a form validation exception is
considerably less performant than simply returning a validation
result.
Hamish Campbell
On Feb 28, 10:50 pm, Sam Minnee <sam.min...@gmail.com> wrote:
> As we start to make use of exceptions in more things, I was wondering
> what people thought about this idea:
> Should we make class SS_UserException extends Exception, designed to
> be used to throw exceptions where the message on that exception is
> acceptable to be sent back to end-users?
> It would mean that you could do things like this in a form action
> handler, without risking that sensitive diagnostic information for
> developers is sent back to end users:
> function doMyAction($data, $form) {
> try {
> // something
> } catch(SS_UserException $e) {
> $form->sessionMessage($e->getMessage(), "error");
> $this->redirectBack();
> }
> // redirect to the success page;
> An exception is supposed to occur when something > unforseen or unexpected (...exceptional) occurs. ... > That is, if an Exception occurs, there is something that needs to be > fixed.
These are two different statements. To be honest, I don't know whether user-input error counts as the flow-control-via-exceptions anti-pattern. But your second statement is very restrictive - it suggests that a 404s (indeed any 400-level error) wouldn't be allowed to be handled via exceptions. I've done a bit of googling and can't find any specific guidance. When people are raise the anti-pattern it's usually about much more banal stuff, like using exceptions to handle inappropriate passing of nulls.
> I've also read (but haven't confirmed this) that using a try..catch > block to, for example, catch a form validation exception is > considerably less performant than simply returning a validation > result.
Time without exception: 0.44 microseconds / call Time with try but never throwing exceptions: 0.48 microseconds / call Time throwing exceptions half the time: 7.08 microseconds / call Time throwing exceptions all the time: 14.51 microseconds / call
So, yes, it looks like we'll be adding 14 microseconds to a form processing request. I think I can live with that. ;-) (For comparison, simply constructing an Exception object hand not throwing it takes about 4.4 microseconds.)
The key is that we'll only be throwing & catch a single exception for a request, whereas the really bad exception anti-patterns might use Exceptions 100s or 1000s of times within a single request, which would start to bloat out code.
I'd like to see most user_error calls replaced with exceptions (where correct - that is, when there's an unresolvable error, not a warning or notice). This would let you suppress errors when desired a lot easier than currently, and all the other things which exceptions provide.
Makes sense that if we do this we extend Exception to be able to also specify a user-readable error message, so we can give users a better message than "something went wrong" without exposing them to technical issues.
I don't think exceptions are the right method for providing non-failure information though. Every raised exception should either be caught & recovered from by some method in the call chain, or cause a failure for the user.
I imagine there are plenty of situations where that failure could be presented to a user better than an "it broke" page - for instance, form actions could be handled with a redirectBack + a flash message. This could either be in the framework or via user code. Either way, it's not something that's easily do-able unless we replace user_errors.
Hamish Friedlander SilverStripe
On 1 March 2011 10:20, Sam Minnée <s...@silverstripe.com> wrote:
> > An exception is supposed to occur when something > > unforseen or unexpected (...exceptional) occurs. > ... > > That is, if an Exception occurs, there is something that needs to be > > fixed.
> These are two different statements. To be honest, I don't know whether > user-input error counts as the flow-control-via-exceptions anti-pattern. > But your second statement is very restrictive - it suggests that a 404s > (indeed any 400-level error) wouldn't be allowed to be handled via > exceptions. I've done a bit of googling and can't find any specific > guidance. When people are raise the anti-pattern it's usually about much > more banal stuff, like using exceptions to handle inappropriate passing of > nulls.
> > I've also read (but haven't confirmed this) that using a try..catch > > block to, for example, catch a form validation exception is > > considerably less performant than simply returning a validation > > result.
> Time without exception: 0.44 microseconds / call > Time with try but never throwing exceptions: 0.48 microseconds / call > Time throwing exceptions half the time: 7.08 microseconds / call > Time throwing exceptions all the time: 14.51 microseconds / call
> So, yes, it looks like we'll be adding 14 microseconds to a form processing > request. I think I can live with that. ;-) > (For comparison, simply constructing an Exception object hand not throwing > it takes about 4.4 microseconds.)
> The key is that we'll only be throwing & catch a single exception for a > request, whereas the really bad exception anti-patterns might use Exceptions > 100s or 1000s of times within a single request, which would start to bloat > out code.
> -- > You received this message because you are subscribed to the Google Groups > "SilverStripe Core Development" group. > To post to this group, send email to silverstripe-dev@googlegroups.com. > To unsubscribe from this group, send email to > silverstripe-dev+unsubscribe@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/silverstripe-dev?hl=en.
OK, so I did a bit more research and as far as I can work out there are no problems with using Exceptions for handling user-errors such validation errors and 400-level errors.
There are two ways in which we could allow for "user-readable exceptions":
1) SS_UserException - which has a single message
2) SS_Exception - which lets you define two messages as two separate parameters: - message - userMessage (defaulting to something innocuous, like "Sorry, something went wrong.")
The advantage of the SS_UserException class is that you can easily trap *only* those exceptions that have user-readable messages, and, for example, show them in the error message part of a form.
However, upon further reflection (and discussion with Hamish F), I'm not sure that this is such a problem. If, for example, there was a database error on a form submission, it would make sense to put "Sorry, we couldn't talk to our database." in the form's error message, rather than showing a generic error page.
That said, it may make logging difficult - we would want to log some exceptions but not others. Exceptions generated by user actions needn't be logged. Right now, the best way of logging an exception is to leave it uncaught, but that's a little crude.
Perhaps we need a separate class, not for user *readable* exceptions, but for user *caused* exceptions. Something, SS_UserGeneratedException or SS_UnloggedException.
Coming full-circle, instead of SS_UnloggedException we could have an $isUserGenerated flag on SS_Exception, and let the logging code inspect that when deciding whether or not to log a message.
On 1/03/2011, at 11:01 AM, Hamish Friedlander wrote:
> I'd like to see most user_error calls replaced with exceptions (where correct - that is, when there's an unresolvable error, not a warning or notice). This would let you suppress errors when desired a lot easier than currently, and all the other things which exceptions provide.
> Makes sense that if we do this we extend Exception to be able to also specify a user-readable error message, so we can give users a better message than "something went wrong" without exposing them to technical issues.
> I don't think exceptions are the right method for providing non-failure information though. Every raised exception should either be caught & recovered from by some method in the call chain, or cause a failure for the user.
> I imagine there are plenty of situations where that failure could be presented to a user better than an "it broke" page - for instance, form actions could be handled with a redirectBack + a flash message. This could either be in the framework or via user code. Either way, it's not something that's easily do-able unless we replace user_errors.
Another variation is to use SS_UserException only to deliver user-centric messages to the user, but where a system exception occurred, hold a reference to it in SS_UserException. Logging inspects all exceptions, and logs all non-user exceptions, and those that are contained in SS_UserException objects. SS_UserException could be raised as a result of user error, and these are only shown to the user. Essentially, SS_UserException uses the exception mechanism to deliver errors in a consistent way, but doesn't have the pure semantics of exceptions as unanticipated events.
When a system exception occurs, this could be wrapped in a SS_UserException, either at the framework level or the application level. I'm doing something similar to this in a project at the moment. The default exception handler would handle unwrapped system exceptions by showing a generic "system had a problem". This gives the opportunity to tailor messages to the user based on the exception and the context it was raised in.
On Wed, Mar 2, 2011 at 1:30 PM, Sam Minnée <s...@silverstripe.com> wrote: > OK, so I did a bit more research and as far as I can work out there are no > problems with using Exceptions for handling user-errors such validation > errors and 400-level errors.
> There are two ways in which we could allow for "user-readable exceptions":
> 1) SS_UserException - which has a single message
> 2) SS_Exception - which lets you define two messages as two separate > parameters: > - message > - userMessage (defaulting to something innocuous, like "Sorry, something > went wrong.")
> The advantage of the SS_UserException class is that you can easily trap > *only* those exceptions that have user-readable messages, and, for example, > show them in the error message part of a form.
> However, upon further reflection (and discussion with Hamish F), I'm not > sure that this is such a problem. If, for example, there was a database > error on a form submission, it would make sense to put "Sorry, we couldn't > talk to our database." in the form's error message, rather than showing a > generic error page.
> That said, it may make logging difficult - we would want to log some > exceptions but not others. Exceptions generated by user actions needn't be > logged. Right now, the best way of logging an exception is to leave it > uncaught, but that's a little crude.
> Perhaps we need a separate class, not for user *readable* exceptions, but > for user *caused* exceptions. Something, SS_UserGeneratedException or > SS_UnloggedException.
> Coming full-circle, instead of SS_UnloggedException we could have an > $isUserGenerated flag on SS_Exception, and let the logging code inspect that > when deciding whether or not to log a message.
> On 1/03/2011, at 11:01 AM, Hamish Friedlander wrote:
> > I'd like to see most user_error calls replaced with exceptions (where > correct - that is, when there's an unresolvable error, not a warning or > notice). This would let you suppress errors when desired a lot easier than > currently, and all the other things which exceptions provide.
> > Makes sense that if we do this we extend Exception to be able to also > specify a user-readable error message, so we can give users a better message > than "something went wrong" without exposing them to technical issues.
> > I don't think exceptions are the right method for providing non-failure > information though. Every raised exception should either be caught & > recovered from by some method in the call chain, or cause a failure for the > user.
> > I imagine there are plenty of situations where that failure could be > presented to a user better than an "it broke" page - for instance, form > actions could be handled with a redirectBack + a flash message. This could > either be in the framework or via user code. Either way, it's not something > that's easily do-able unless we replace user_errors.
> -- > You received this message because you are subscribed to the Google Groups > "SilverStripe Core Development" group. > To post to this group, send email to silverstripe-dev@googlegroups.com. > To unsubscribe from this group, send email to > silverstripe-dev+unsubscribe@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/silverstripe-dev?hl=en.
I'm pretty sure that exception wrapping is considered an anti-pattern, because it buries the call stack. You could always modify the user-message in the existing exception object as it bubbles up the call stack?
> Another variation is to use SS_UserException only to deliver user-centric messages to the user, but where a system exception occurred, hold a reference to it in SS_UserException. Logging inspects all exceptions, and logs all non-user exceptions, and those that are contained in SS_UserException objects. SS_UserException could be raised as a result of user error, and these are only shown to the user. Essentially, SS_UserException uses the exception mechanism to deliver errors in a consistent way, but doesn't have the pure semantics of exceptions as unanticipated events.
> When a system exception occurs, this could be wrapped in a SS_UserException, either at the framework level or the application level. I'm doing something similar to this in a project at the moment. The default exception handler would handle unwrapped system exceptions by showing a generic "system had a problem". This gives the opportunity to tailor messages to the user based on the exception and the context it was raised in.
