I have a program where users can log in. I have a User class and when they log in, all of their attributes (permissions, username, real name, etc) are saved as class properties. However I am having difficulty wrapping my mind around what the best way is to have the user update their own info due to the fact that all data needs to be validated.
Currently, when a user tries to update their own information, e.g. their email address, I do:
// Set new email
$try = $user->setEmail($_POST['new_email']);
if ($try !== true) {
$errors[] = $try;
}
...
if (isset($errors) {
// Tell user what went wrong
} else {
$user->saveValuesToDB();
}
It works fine, but seems rather ugly to me since User->setEmail() needs to have a mixed return type because the email can be invalid for a multiple number of reasons (domain not found, invalid format, empty string provided, already used by another user, etc) and the exact reason needs to be passed back to the user, but as I understand mixed returns are generally discouraged.
Another way I thought of doing it is just updating all of the properties without performing any sort of validation and then doing User->commitAllChanges() at the end which would perform all of the validations, which would take the multiple returns out of all the setter method and reduce it down to only the commit method, but I really don't like this either because I feel that any properties actually being set should be valid and it still doesn't completely get rid of the problem.
What are some other method(s) that I can use to allow the user to set object properties and also validate them, sending any error back to the user? Or is what I'm doing now fine?
Another way I thought of doing it is just updating all of the
properties without performing any sort of validation and then doing
User->commitAllChanges() at the end which would perform all of the
validations, which would take the multiple returns out of all the
setter method and reduce it down to only the commit method
This is generally a good approach. You can also break down the whole process into smaller pieces so that it's more versatile.
For example, the model might have a validate method that returns a single boolean (is everything OK, or is there one or more errors?), and a getValidationErrors method that returns an array which holds all the information you care about (would probably need to be multidimensional). It could have a commit method that automatically calls validate and only persists the changes if there are no errors at all.
but I really don't like this either because I feel that any properties
actually being set should be valid and it still doesn't completely get
rid of the problem.
Properties "that just won't be set" are IMO way more trouble than one might initially think. The major problem is that if a property objects to being set (either by returning some error code or, much better, by throwing an exception) then you have to wrap all access to it with "protective" code. This gets very tedious very fast.
In practice, it's much more convenient to massively assign values to all of the properties in one go through a helper method and then see what the result was (e.g. by calling a validate method as above).
Also, do not forget that in practice you 'd want to display validation errors next to the data input fields, and those fields are wired to be pre-populated with the values of your model's properties. So on the one hand the properties need to be valid, and on the other hand they need to match exactly what the user entered (otherwise the users will want to kill you). It's therefore better to relax the "must always be valid" constraint in favor of utility -- although these days with AJAX forms and client-side validation everywhere, this is becoming less relevant.
Use a try-catch construction.
// somewhere in your project
class UserValidationException extends \Exception {}
// In your form handler, controller, or whatever you have
try {
$user->setName($_POST['new_email']);
$user->setEmail($_POST['new_name']);
// etc.
}
catch (UserValidationException $e)
{
// tell user what went wrong using $e->getMessage();
}
// In your User class
class User
{
// ...
public function setName($newName)
{
if (strlen($newName) < 2)
throw new UserValidationException('User name is too short');
}
}
One way is to keep your solution (setEmail function) and make it more OOP.
Inside the setAction you can sett a list of errors
$this->errors[] = 'Domain invaild';
Then make 1 or 2 functions to get erros from the class
public function hasErrors() {return false == empty($this->errors);}
public function getErrors() {return $this->errors;}
Nothing wrong with your current method, but what you could do is use exceptions.
It might look like:
try
{
$user->setEmail($_POST['new_email']);
}
catch (Exception $e)
{
$errors[] = $e->getMessage();
}
And your setEmail method would be something like:
setEmail($email)
{
if ($tooShort) // validate length
throw new Exception("Email is too short"); // perhaps have custom exceptions
}
I use something of a MVC.NET approach on this. Basically the bad in the throwing an exception is performance and why would you only want to catch one? If both email and password are invalid just let them know at the same time rather then 'email is incorrect' - fix email - repost - 'oh password was also incorrect probably could have told you sooner but hey you only make me money'
Edited:
Also exceptions should not be thrown because of validation returning false, we expect to get invalid data whether it's intentional (hackers) or not. But lets see how this handles in a massive form, watch how slow the processing becomes. Validation errors should be stored and displayed to the user, it should only cause our programs to enter conditionals not exceptions. Exceptions should be reserved for coded errors, such as resources we require aren't there, someone deleted the library file or framework we need to execute code correctly, and so on.
<?php
class ChangePasswordModel
{
protected $OldPassword;
protected $NewPassword;
protected $ConfirmPassword;
protected $ValidationMessages;
protected $DisplayNames;
public function __construct()
{
$this->DisplayNames = array(
"OldPassword" => "Old password",
"NewPassword" => "New password",
"ConfirmPassword" => "Confirm new password",
);
}
public function LoadPost()
{
if ( !isset( $_POST["OldPassword"] ) || !isset( $_POST["NewPassword"] ) || !isset( $_POST["ConfirmPassword"] ) )
return;
$this->OldPassword = trim( $_POST["OldPassword"] );
$this->NewPassword = trim( $_POST["NewPassword"] );
$this->ConfirmPassword = trim( $_POST["ConfirmPassword"] );
if ( strlen( $this->OldPassword ) < 1 )
$this->ValidationMessages["OldPassword"] = "Old password is not set";
if ( strlen( $this->NewPassword ) < 6 )
$this->ValidationMessages["NewPassword"] = "Password must be at least 5 characters.";
if ( $this->NewPassword != $this->ConfirmPassword )
$this->ValidationMessages["ConfirmPassword"] = "Passwords do not match.";
}
public function ValidationMessageFor( $name )
{
if ( !isset( $this->ValidationMessages[$name] ) )
return "";
return $this->ValidationMessages[$name];
}
public function DisplayNameFor( $name )
{
// Throw exception if not set
return $this->DisplayNames[$name];
}
}
$Model = new ChangePasswordModel();
$Model->LoadPost();
?>
<form action="" method="post">
<div>
<?= $Model->DisplayNameFor( "OldPassword" ) ?>
</div>
<div>
<input type="password" name="OldPassword" />
<span><?= $Model->ValidationMessageFor( "OldPassword" ) ?></span>
</div>
<div>
<?= $Model->DisplayNameFor( "NewPassword" ) ?>
</div>
<div>
<input type="password" name="NewPassword" />
<span><?= $Model->ValidationMessageFor( "NewPassword" ) ?></span>
</div>
<div>
<?= $Model->DisplayNameFor( "ConfirmPassword" ) ?>
</div>
<div>
<input type="password" name="ConfirmPassword" />
<span><?= $Model->ValidationMessageFor( "ConfirmPassword" ) ?></span>
</div>
<p>
<input type="submit" value="Change Password" />
</p>
</form>
