Should exceptions be thrown based on user input?

[NotionCommotion]

Consider the following.  Also, please comment on my use of USERException.  Thanks

<?php

try {
    $validate=new Validate();
    $data=$validate->getValidatedData($_POST);
    //insert record
}
catch (USERException $e) {
    //Deal with the error
}

//New file
class USERException extends Exception {}    //Should this exception be defined here? Should another exception type be used?
class Validate{

    public function getValidatedData($data)
    {
        if(!isset($data['city'])){throw new USERException("City field is missing.");}
        if(!$this->isACity($data['city'])){throw new USERException("$data[city] is not a valid city.");}
        // ...
        return ['city'=>$data['city'],'etc'=>123];
    }

}
?>

[requinix]

Whether you use exceptions or not has nothing to do with where data comes from. It's about the architecture of the code and execution flow of the application.

 

I strayed a bit from the exception stuff because the sun is coming up and I'm tired and I ramble when I'm tired.

 

- I'm feeling meh about your use of exceptions here. "getValidatedData" sounds like it's doing two jobs: validation and getting data. Which is really only one job because you told it the data to begin with. Instead I would use a method to validate what's in $_POST and return a list of problems (if any).

- USERException, which should really be called UserException, which should really be called something more useful than that, would go in its own file to be autoloaded. Like every other class.

- WhateverItGetsCalledException would benefit from storing which field was invalid too.

- Validate class, which again should be called something more useful, shouldn't be a class if all you're doing is putting one method on it like that. If you're not going to reuse the logic anywhere then it should just go alongside the rest of the input processing code. If you would reuse it then it would be better served as just a regular function - there is nothing "object oriented" about it so it doesn't make sense as a class. If you would reuse it as a sort of validator that might get passed around to other places in an arbitrary manner that isn't hard-coded into whatever processing logic is at work, then it should observe some sort of interface and you better have at least two more classes like it to do other types of validation.

- Throwing an exception as a way of returning error information is... less than optimal, but IMO acceptable. It would make more sense if you skipped the validation at this point, blindly passed it along to whatever will perform the action (the //insert record), and that performs the validation and throws the exception if invalid.

- Exceptions are about exceptional behavior, that is to say behavior that is unexpected or abnormal or otherwise not what typically should happen when the code executes. Validation is a normal step, getting errors is normal enough to not be considered "unexpected" or "abnormal", and so I wouldn't use exceptions for that. A simple "getValidationErrors(array $data): array" would suffice.

- So then if //insert record calls the validation, what happens is the validation returns normally (validation failure is not unexpected behavior) whatever information to it, and then if there's a validation error it throws the exception itself (as the failure is "not what typically should happen"). Again, there's wiggle room there about whether exceptions could be considered appropriate or not.

[Jacques1]

Abusing exceptions to direct the normal control flow of the program is poor style and very confusing.

 

Exceptions are for exceptional situations. Most of those problems cannot be recovered from, which is why exceptions are fatal by default and make PHP abort the entire program. You, on the other hand, use exceptions even when there's no problem at all, giving a nasty surprise to anybody has hasn't carefully read your documentation (if the exceptions are documented at all).

 

Getting information from a method or function is what the return value is for. Exceptions are for (mostly serious) internal errors.

[Psycho]

Also, there is a usability problem in that code. Form validations should not stop at the first validation error found. It should check for all possible errors and then notify the user of all the problems they need to resolve.

[NotionCommotion]

Also, there is a usability problem in that code. Form validations should not stop at the first validation error found. It should check for all possible errors and then notify the user of all the problems they need to resolve.

That is what I was implying under:

catch (USERException $e) {
    //Deal with the error
}

Thank you all three for your replies.

 

Okay, I concede using exceptions for validation isn't best.

 

I heard the word "exceptional" behavior and situations a couple of times.  I thought exceptions are used to deal with user behavior, and errors are used to deal with bugs in the script.

 

What do you consider is exceptional?

[Psycho]

That is what I was implying under:

catch (USERException $e) {
    //Deal with the error
}

 

Right, but . . . you also have this

    public function getValidatedData($data)
    {
        if(!isset($data['city'])){throw new USERException("City field is missing.");}
        if(!$this->isACity($data['city'])){throw new USERException("$data[city] is not a valid city.");}
        // ...
        return ['city'=>$data['city'],'etc'=>123];

    }

The FIRST "error" will throw an exception and go strait to the catch condition. In the specific two conditions you gave in the example it is not a problem, because if the value is empty then there is no need to check if the value if valid. But, you follow that with the // ... which I assume to be other form validations. That is where the problem exists. Let's say there was another validation to check the state value. If a user submitted a form with no city and no state, the check for the city field would throw the exception and go to the catch logic. The logic would never validate the state field. What would result is you would provide the user an error message that the city field is required. They then enter a value and submit the form only to then be told that the state is required. Check form submissions for all such validations. Then if any fail, provide the user with a response that shows all the errors to be resolved.

 

Example

$errors = array();
 
$city = trim($_POST['city']);
if($city == '') {
    $errors[] = "City is required";
} elseif(!$this->isACity($city)) {
    $errors[] = "The selected city is not valid";
}
 
$state = trim($_POST['state']);
if($state =='') {
    $errors[] = "State is required";
} elseif(!$this->isAState($state)) {
    $errors[] = "The selected state is not valid";
}
 
if(!empty{$errors))
{
    //Provide all the errors to the user to resolve
}
else
{
    //All form validations passed, process the data
}

Edited June 9, 2016 by Psycho

[Jacques1]

I thought exceptions are used to deal with user behavior, and errors are used to deal with bugs in the script.

 

You mean errors in the sense of the E_* constants? No. Exceptions and classical PHP errors are two unrelated concepts. The reason why they both exist is historical: In the early days, there were no exceptions, just errors. Errors are primitive and difficult to handle (they cannot be caught), so later PHP versions introduced the more modern concept of exceptions known from many other languages. Now we have both. Classical errors are mostly seen in legacy code (as well as the PHP core), exceptions are mostly used in modern applications.

 

 

 

What do you consider is exceptional?

 

Bugs, network problems, database problems, missing files, ...

 

Anything that happens unexpectedly and is worth suspending the normal control flow of the program. Wrong user input does not fall into this category, because it's perfectly normal for the user to make a mistake. This should be handled by the standard processing routine (print a warning and emit an appropriate HTTP code), not by putting PHP into “panic mode”.

Edited June 9, 2016 by Jacques1