validation in form handlers

Moved validation code from classes to form handlers. Probably better
in the classes, but I can't quite grok the validate() method in
DB_DataObject, so for now I'm going to do it the old-fashioned way.

darcs-hash:20080521112707-84dde-38e27199b977ae81171b8391fbdb93ebb54494f9.gz

actions/newnotice.php

@@ -49,19 +49,22 @@ class NewnoticeAction extends Action {
 		$notice->profile_id = $user->id; # user id *is* profile id
 		$notice->created = DB_DataObject_Cast::dateTime();
 		# Default theme uses 'content' for something else
-		$notice->content = trim($this->arg('noticecontent'));
+		$notice->content = $this->trimmed('noticecontent');
+
+		if (!$notice->content) {
+			$this->show_form(_t('No content!'));
+		} else if (strlen($notice->content) > 140) {
+			$this->show_form(_t('Notice content too long.'));
+		}
 		
-		$val = $notice->validate();
-		if ($val === TRUE) {
 		return $notice->insert();
-		} else {
-			// XXX: display some info
-			return NULL;
-		}
 	}
 	
-	function show_form() {
+	function show_form($msg=NULL) {
 		common_show_header(_t('New notice'));
+		if ($msg) {
+			common_element('div', 'error', $msg);
+		}
 		common_notice_form();
 		common_show_footer();
 	}

actions/profilesettings.php

@@ -52,30 +52,54 @@ class ProfilesettingsAction extends SettingsAction {
 	}
 
 	function handle_post() {
-		$nickname = $this->arg('nickname');
+		
-		$fullname = $this->arg('fullname');
+		$nickname = $this->trimmed('nickname');
-		$email = $this->arg('email');
+		$fullname = $this->trimmed('fullname');
-		$homepage = $this->arg('homepage');
+		$email = $this->trimmed('email');
-		$bio = $this->arg('bio');
+		$homepage = $this->trimmed('homepage');
-		$location = $this->arg('location');
+		$bio = $this->trimmed('bio');
+		$location = $this->trimmed('location');
+
+		# Some validation
+		
+		if (!Validate::email($email, true)) {
+			$this->show_form(_t('Not a valid email address.'));
+			return;
+		} else if (!Validate::string($nickname, array('min_length' => 1,
+													  'max_length' => 64,
+													  'format' => VALIDATE_NUM . VALIDATE_ALPHA_LOWER))) {
+			$this->show_form(_t('Nickname must have only letters and numbers and no spaces.'));
+			return;
+		} else if (!is_null($homepage) && (strlen($homepage) > 0) &&
+				   !Validate::uri($homepage, array('allowed_schemes' => array('http', 'https')))) {
+			$this->show_form(_t('Homepage is not a valid URL.'));
+			return;
+		} else if (!is_null($fullname) && strlen($fullname) > 255) {
+			$this->show_form(_t('Fullname is too long (max 255 chars).'));
+			return;
+		} else if (!is_null($bio) && strlen($bio) > 140) {
+			$this->show_form(_t('Bio is too long (max 140 chars).'));
+			return;
+		} else if (!is_null($location) && strlen($location) > 255) {
+			$this->show_form(_t('Location is too long (max 255 chars).'));
+			return;
+		} else if ($this->nickname_exists($nickname)) {
+			$this->show_form(_t('Nickname already exists.'));
+			return;
+		} else if ($this->email_exists($email)) {
+			$this->show_form(_t('Email address already exists.'));
+			return;
+		}
 		
 		$user = common_current_user();
 		assert(!is_null($user)); # should already be checked
 
-		# FIXME: scrub input
 		# FIXME: transaction!
 
 		$original = clone($user);
 
-		$user->nickname = $this->arg('nickname');
+		$user->nickname = $nickname;
-		$user->email = $this->arg('email');
+		$user->email = $email;
-
-		$val = $user->validate();
-		if ($val !== TRUE) {
-			# XXX: better validation
-			$this->show_form(_t('Error saving user; invalid.'));
-			return;
-		}
 
 		if (!$user->update($original)) {
 			common_server_error(_t('Couldnt update user.'));
@@ -87,19 +111,12 @@ class ProfilesettingsAction extends SettingsAction {
 		$orig_profile = clone($profile);
 
 		$profile->nickname = $user->nickname;
-		$profile->fullname = $this->arg('fullname');
+		$profile->fullname = $fullname;
-		$profile->homepage = $this->arg('homepage');
+		$profile->homepage = $homepage;
-		$profile->bio = $this->arg('bio');
+		$profile->bio = $bio;
-		$profile->location = $this->arg('location');
+		$profile->location = $location;
 		$profile->profileurl = common_profile_url($nickname);
 
-		$val = $profile->validate();
-		if ($val !== TRUE) {
-			# XXX: some feedback here, please!
-			$this->show_form(_t('Error saving profile; invalid.'));
-			return;
-		}
-
 		if (!$profile->update($orig_profile)) {
 			common_server_error(_t('Couldnt save profile.'));
 			return;
@@ -107,4 +124,24 @@ class ProfilesettingsAction extends SettingsAction {
 
 		$this->show_form(_t('Settings saved.'), TRUE);
 	}
+	
+	function nickname_exists($nickname) {
+		$user = common_current_user();
+		$other = User::staticGet('nickname', $nickname);
+		if (!$other) {
+			return false;
+		} else {
+			return $other->id != $user->id;
+		}
+	}
+	
+	function email_exists($email) {
+		$user = common_current_user();
+		$other = User::staticGet('email', $email);
+		if (!$other) {
+			return false;
+		} else {
+			return $other->id != $user->id;
+		}
+	}
 }

actions/register.php

@@ -34,18 +34,27 @@ class RegisterAction extends Action {
 	}
 
 	function try_register() {
-		$nickname = $this->arg('nickname');
+		$nickname = $this->trimmed('nickname');
+		$email = $this->trimmed('email');
+		
+		# We don't trim these... whitespace is OK in a password!
+		
 		$password = $this->arg('password');
 		$confirm = $this->arg('confirm');
-		$email = $this->arg('email');
 
 		# Input scrubbing
 
 		$nickname = common_canonical_nickname($nickname);
 		$email = common_canonical_email($email);
 
-		if ($this->nickname_exists($nickname)) {
+		if (!Validate::email($email, true)) {
-			$this->show_form(_t('Username already exists.'));
+			$this->show_form(_t('Not a valid email address.'));
+		} else if (!Validate::string($nickname, array('min_length' => 1,
+													  'max_length' => 64,
+													  'format' => VALIDATE_NUM . VALIDATE_ALPHA_LOWER))) {
+			$this->show_form(_t('Nickname must have only letters and numbers and no spaces.'));
+		} else if ($this->nickname_exists($nickname)) {
+			$this->show_form(_t('Nickname already exists.'));
 		} else if ($this->email_exists($email)) {
 			$this->show_form(_t('Email address already exists.'));
 		} else if ($password != $confirm) {
@@ -84,11 +93,6 @@ class RegisterAction extends Action {
 		$profile->profileurl = common_profile_url($nickname);
 		$profile->created = DB_DataObject_Cast::dateTime(); # current time
 
-		$val = $profile->validate();
-		if ($val !== TRUE) {
-			# XXX: some feedback here, please!
-			return FALSE;
-		}
 		$id = $profile->insert();
 		if (!$id) {
 			return FALSE;
@@ -100,14 +104,6 @@ class RegisterAction extends Action {
 		$user->email = $email;
 		$user->created =  DB_DataObject_Cast::dateTime(); # current time
 
-		$val = $user->validate();
-		if ($val !== TRUE) {
-			# XXX: some feedback here, please!
-			# Try to clean up...
-			$profile->delete();
-			return FALSE;
-		}
-
 		$result = $user->insert();
 		if (!$result) {
 			# Try to clean up...

doc/TODO

@@ -38,10 +38,10 @@
 + save profile URL on registration
 + require valid nicknames
 + reject empty notices
-- validate registration form results
++ validate registration form results
-- validate profilesettings form results
++ validate profilesettings form results
-- validate newnotice form results
++ validate newnotice form results
-- remove validation code from classes
++ remove validation code from classes
 + use only canonical usernames
 - use only canonical email addresses
 - RSS 1.0 feeds of a user's notices
@@ -55,7 +55,7 @@
 - pretty URLs
 - instructions
 - deal with PHP quotes escaping
-- fix layout of textarea
++ fix layout of textarea
 + make notices into "big links"
 - fix spacing on notices
 - limit entry in textarea to 140 chars

lib/action.php

@@ -34,6 +34,11 @@ class Action { // lawsuit
 		}
 	}
 
+	function trimmed($key) {
+		$arg = $this->arg($key);
+		return (is_string($arg)) ? trim($arg) : $arg;
+	}
+	
 	function handle($argarray) {
 		$this->args = array();
 		foreach ($argarray as $k => $v) {