Various improvements to HTML forms classes; Updated login to use form object; Added forgotten password reset mechanism

10 files changed, 248 insertions, 102 deletions

diff --git a/frontend/classes/form.php b/frontend/classes/form.php
deleted file mode 100644
index 51f548f..0000000
--- a/frontend/classes/form.php
+++ /dev/null
@@ -1,53 +0,0 @@
-<?php
-class form {
-	protected $elements=array();
-	function output($rw=true, $vals=array()) {
-		foreach ($this->elements as $name => &$el) {
-			if (is_object($el)) {
-				if (!$el->status)
-					echo print_warning('Please complete this field:');
-				$el->output($rw, isset($vals[$name])?$vals[$name]:false);
-			} else {
-				echo $el;
-			}
-		}
-	}
-	function process() {
-		$vals=array();
-		foreach ($this->elements as $name => &$el) {
-			if (!is_object($el)) continue;
-			$vals[$name]=$el->process();
-			$el->status=$vals[$name] !== false;
-		}
-		return $vals;
-	}
-	function verify($vals) {
-		foreach ($this->elements as $name => &$el) {
-			if (!is_object($el)) continue;
-			if (!isset($vals[$name]))
-				return null;
-			elseif (!($el->status=$el->verify($vals[$name])))
-				return false;
-		}
-		return true;
-	}
-	public function text($text) {
-		$this->elements[]=$text;
-	}
-	public function text_input($optname, $htmlname, $label) {
-		$this->elements[$optname]=new text_input($htmlname, $label);
-	}
-	public function select($optname, $htmlname, $label, $options) {
-		$this->elements[$optname]=new select($htmlname, $label, $options);
-	}
-	public function radio_array($optname, $htmlname, $label, $options) {
-		$this->elements[$optname]=new radio_array($htmlname, $label, $options);
-	}
-	public function checkbox_array($optname, $htmlname, $label, $array, $delim=' ') {
-		$this->elements[$optname]=new checkbox_array($htmlname, $label, $array, $delim=' ');
-	}
-	public function layered_checkbox_array($optname, $htmlname, $label, &$array, $delim=' ', $metadata) {
-		$this->elements[$optname]=new layered_checkbox_array($htmlname, $label, $array, $delim, $metadata);
-	}
-}
-?>
diff --git a/frontend/classes/form_elements.php b/frontend/classes/forms.php
index 6e5b43e..602c743 100644
--- a/frontend/classes/form_elements.php
+++ b/frontend/classes/forms.php
@@ -1,4 +1,73 @@
 <?php
+class form {
+	protected $action, $method, $enctype, $elements=array();
+	function __construct($action=null, $method='post', $enctype=null) {
+		global $S;
+		$this->action=$action?$action:url($S['request']);
+		$this->method=$method;
+		$this->enctype=$enctype;
+	}
+	public function output($vals=array(), $rw=true) {
+		if ($rw)
+			echo '<form action="'.htmlentities($this->action).'" method="'.$this->method.'"'.($this->enctype?'enctype="'.$this->enctype.'"':'').'>';
+		foreach ($this->elements as $name => &$el) {
+			if (!$el->status)
+				echo print_warning('Please complete this field:');
+			$el->output(isset($vals[$name])?$vals[$name]:false, $rw);
+		}
+		if ($rw)
+			echo '</form>';
+	}
+	public function process() {
+		$vals=array();
+		foreach ($this->elements as $name => &$el) {
+			$vals[$name]=$el->process();
+			$el->status=$vals[$name] !== false;
+		}
+		return $vals;
+	}
+	public function verify($vals) {
+		foreach ($this->elements as $name => &$el) {
+			if (!($el->status=$el->verify(isset($vals[$name])?$vals[$name]:false))) {
+				return $el->status;
+			}
+		}
+		return true;
+	}
+	public function text($text) {
+		$this->elements[]=new form_text($text);
+	}
+	public function rw_text($text) {
+		$this->elements[]=new form_rw_text($text);
+	}
+	public function ro_text($text) {
+		$this->elements[]=new form_ro_text($text);
+	}
+	public function text_input($optname, $htmlname, $label) {
+		$this->elements[$optname]=new text_input($htmlname, $label);
+	}
+	public function password($optname, $htmlname, $label) {
+		$this->elements[$optname]=new form_password($htmlname, $label);
+	}
+	public function hidden($optname, $htmlname, $value) {
+		$this->elements[$optname]=new form_hidden_input($htmlname, $value);
+	}
+	public function select($optname, $htmlname, $label, $options) {
+		$this->elements[$optname]=new select($htmlname, $label, $options);
+	}
+	public function radio_array($optname, $htmlname, $label, $options) {
+		$this->elements[$optname]=new radio_array($htmlname, $label, $options);
+	}
+	public function checkbox_array($optname, $htmlname, $label, $array, $delim=' ') {
+		$this->elements[$optname]=new checkbox_array($htmlname, $label, $array, $delim=' ');
+	}
+	public function layered_checkbox_array($optname, $htmlname, $label, &$array, $delim=' ', $metadata) {
+		$this->elements[$optname]=new layered_checkbox_array($htmlname, $label, $array, $delim, $metadata);
+	}
+	public function submit($text=null) {
+		$this->rw_text('<input type="submit" value="'.($text?htmlentities($text):'Submit').'" />');
+	}
+}
 abstract class form_element {
 	protected $htmlname, $label;
 	public $status=true;
@@ -6,7 +75,7 @@ abstract class form_element {
 		$this->htmlname=htmlentities($htmlname);
 		$this->label=htmlentities($label);
 	}
-	public function output($rw=true, $val=false) {
+	public function output($val=false, $rw=true) {
 		echo "<b>$this->label:</b> ";
 	}
 	public function process() {
@@ -16,21 +85,66 @@ abstract class form_element {
 		return $val !== false;
 	}
 }
+class form_text extends form_element {
+	protected $text, $rw;
+	function __construct($text, $rw=null) {
+		$this->text=$text;
+		$this->rw=$rw;
+	}
+	public function output($val=null, $rw=true) {
+		if (!isset($this->rw) || $this->rw == $rw) echo $this->text;
+	}
+	public function process() {
+		return null;
+	}
+	public function verify() {
+		return true;
+	}
+}
+class form_rw_text extends form_text {
+	function __construct($text) {
+		parent::__construct($text, true);
+	}
+}
+class form_ro_text extends form_text {
+	function __construct($text) {
+		parent::__construct($text, false);
+	}
+}
 class text_input extends form_element {
-	public function output($rw=true, $val=false) {
-		parent::output($rw, $val);
+	public function output($val=false, $rw=true) {
+		parent::output($val, $rw);
 		echo $rw?"<input name=\"$this->htmlname\"".($val===false?'':'value="'.htmlentities($val).'"').' />':($val===false?'':htmlentities($val));
 		echo "<br/>\n";
 	}
 }
+class form_password extends form_element {
+	public function output($val=false, $rw=true) {
+		parent::output($val, $rw);
+		echo $rw?"<input name=\"$this->htmlname\" type=\"password\" />":($val?'*******':'');
+		echo "<br/>\n";
+	}
+}
+class form_hidden_input extends form_element {
+	private $value;
+	function __construct($htmlname, $value) {
+		$this->htmlname=$htmlname;
+		$this->value=$value;
+	}
+	public function output($val=false, $rw=true) {
+		if ($rw) {
+			echo '<input type="hidden" name="'.$this->htmlname.'" value="'.htmlentities($this->value).'" />';
+		}
+	}
+}
 class select extends form_element {
 	private $options;
 	function __construct($htmlname, $label, $options) {
 		parent::__construct($htmlname, $label);
 		$this->options=$options;
 	}
-	public function output($rw=true, $val=false) {
-		parent::output($rw, $val);
+	public function output($val=false, $rw=true) {
+		parent::output($val, $rw);
 		if ($rw) {
 			echo '<select name="'.$this->htmlname.'">'."\n";
 			$i=0;
@@ -56,8 +170,8 @@ class select extends form_element {
 	}
 }
 class radio_array extends select {
-	public function output($rw=true, $val=false) {
-		if (!$rw) return parent::output($rw, $val);
+	public function output($val=false, $rw=true) {
+		if (!$rw) return parent::output($val, $rw);
 		echo "$this->label:<br/>\n";
 		$i=0;
 		foreach ($this->options as $value => $label) {
@@ -73,7 +187,7 @@ class checkbox_array extends form_element {
 		$this->array=$array;
 		$this->delim=$delim;
 	}
-	public function output($rw=true, $val=false) {
+	public function output($val=false, $rw=true) {
 		$this->set_val($val);
 		if (strlen($this->label))
 			echo "<b>$this->label:</b><br/>\n";
@@ -127,7 +241,7 @@ class layered_checkbox_array extends checkbox_array {
 			$S['scripts'][]='lca';
 		}
 	}
-	public function output($rw=true, $val=false) {
+	public function output($val=false, $rw=true) {
 		$this->set_val($val);
 		if ($this->label) {
 			echo '<h4>'.htmlentities($this->label).'</h4>';
diff --git a/frontend/classes/wizard.php b/frontend/classes/wizard_step.php
index 31d02e9..c2b5c20 100644
--- a/frontend/classes/wizard.php
+++ b/frontend/classes/wizard_step.php
@@ -2,9 +2,15 @@
 class wizard_step extends form {
 	public $configuration, $module, $step, $title, $next;
 	function __construct(&$c, $step, $noload=false) {
+		global $S;
+		parent::__construct(url('config/'.$c->id));
 		$this->configuration=&$c;
 		$this->module=new module($c->module);
 		$this->step=$step;
+		$this->title=$this->module->steps[$step-1];
+		$scale=$S['conf']['progressbar_width']/$this->module->numsteps;
+		$this->rw_text('<a style="float: right" href="'.url('config/'.$this->configuration->id.'/status').'">Status</a><h3>Step '.$this->step.': '.$this->title."</h3>\n".'<img src="'.url('images/full.gif').'" style="border-left: 1px solid black; border-top: 1px solid black; border-bottom: 1px solid black; width: '.$this->step*$scale.'px; height: 15px" /><img src="'.url('images/empty.gif').'" style="border-right: 1px solid black; border-top: 1px solid black; border-bottom: 1px solid black; width: '.(count($this->module->steps)-$this->step)*$scale.'px; height: 15px" /><br/>'."\n");
+		$this->buttons();
 		if (!$noload) {
 			$file=$this->module->dir."/step$step.php";
 			if (!is_readable($file)) {
@@ -12,25 +18,15 @@ class wizard_step extends form {
 			}
 			require($file);
 		}
-		$this->title=$this->module->steps[$step-1];
 		$this->next=isset($next)?$next:($this->step == $this->module->numsteps?null:$step+1);
 	}
 	public function output($rw=true) {
-		global $S;
-		echo "<div class=\"wizard\" id=\"step$this->step\">";
-		if ($rw)
-			echo '<form action="'.url('config/'.$this->configuration->id).'" method="post"><a style="float: right" href="'.url('config/'.$this->configuration->id.'/status').'">Status</a>';
-		if ($rw) {
-			echo '<h3>Step '.$this->step.': '.$this->title."</h3>\n";
-			$scale=$S['conf']['progressbar_width']/$this->module->numsteps;
-			echo '<img src="'.url('images/full.gif').'" style="border-left: 1px solid black; border-top: 1px solid black; border-bottom: 1px solid black; width: '.$this->step*$scale.'px; height: 15px" /><img src="'.url('images/empty.gif').'" style="border-right: 1px solid black; border-top: 1px solid black; border-bottom: 1px solid black; width: '.(count($this->module->steps)-$this->step)*$scale.'px; height: 15px" /><br/>'."\n";
-			$this->echo_buttons();
-		}
-		parent::output($rw, $this->get_opts());
-		if ($rw) {
-			echo '<br/>';
-			$this->echo_buttons();
+		if ($rw) { // We're assuming that one page never gets output rw twice in one page load
+			$this->rw_text('<br/>');
+			$this->buttons();
 		}
+		echo "<div class=\"wizard\" id=\"step$this->step\">";
+		parent::output($this->get_opts(), $rw);
 		echo '</div>'."\n";
 	}
 	public function process() {
@@ -54,7 +50,6 @@ class wizard_step extends form {
 	private function get_opts() {
 		$vals=array();
 		foreach ($this->elements as $name => &$el) {
-			if (!is_object($el)) continue;
 			$vals[$name]=$this->get_opt($name);
 		}
 		return $vals;
@@ -68,8 +63,8 @@ class wizard_step extends form {
 	private function delete_opt($name) {
 		return $this->configuration->delete_opt($name);
 	}
-	private function echo_buttons() {
-		echo ($this->step > 1?'<input type="button" onclick="window.location=\''.url('config/'.$this->configuration->id.'/'.($this->step-1)).'\'" value="Back" /> ':'&nbsp;').'<input style="float: right" type="submit" name="wizard_submit['.$this->step.']" value="'.($this->step == $this->module->numsteps?'Finish':'Next').'" /><br/>';
+	private function buttons() {
+		$this->rw_text(($this->step > 1?'<input type="button" onclick="window.location=\''.url('config/'.$this->configuration->id.'/'.($this->step-1)).'\'" value="Back" /> ':'&nbsp;').'<input style="float: right" type="submit" name="wizard_submit['.$this->step.']" value="'.($this->step == $this->module->numsteps?'Finish':'Next').'" /><br/>');
 	}
 }
 ?>
diff --git a/frontend/pages/login.php b/frontend/pages/login.php
index 953d2c4..d821396 100644
--- a/frontend/pages/login.php
+++ b/frontend/pages/login.php
@@ -1,24 +1,35 @@
 <?php
 function init_login(&$S) {
 	if (isset($S['user'])) {
+		if (isset($_REQUEST['go']))
+			header('Location: '.url($_REQUEST['go']));
 		// Should we let you continue to $_REQUEST['go'] instead?
 		return 'welcome';
-	} else {
-		if (isset($_REQUEST['email']) && isset($_REQUEST['password'])) {
-			$r=query('SELECT * FROM `users` WHERE `email`='.$S['pdo']->quote($_REQUEST['email']).' AND `passhash`="'.sha1($_REQUEST['password']).'"');
-			if ($r->rowCount()) {
-				$S['user']=new sql_user($r->fetch(PDO::FETCH_ASSOC));
-				$S['login.result']=sql_session::create();
-			} else {
-				$S['login.result']=false;
-			}
+	}
+	if (substr($S['request'], 0, 5) != 'login')
+		$_REQUEST['go']=$S['request'];
+	$S['login']['form']=new form(url('login'));
+	$form=&$S['login']['form'];
+	if (isset($_REQUEST['go']))
+		$form->hidden('go', 'go', $_REQUEST['go']);
+	$form->text_input('email', 'email', 'Email');
+	$form->password('password', 'password', 'Password');
+	$form->submit();
+	$S['login']['data']=isset($_REQUEST['email'])?$form->process():array();
+	$data=&$S['login']['data'];
+	if (isset($data['email'], $data['password'])) {
+		$r=query('SELECT * FROM `users` WHERE `email`='.$S['pdo']->quote($data['email']).' AND `passhash`="'.sha1($data['password']).'"');
+		if ($r->rowCount()) {
+			$S['user']=new sql_user($r->fetch(PDO::FETCH_ASSOC));
+			$S['login.result']=sql_session::create();
+		} else {
+			$S['login.result']=false;
 		}
-		return array('title' => 'Login');
 	}
+	$S['title']='Login';
 }
 function body_login(&$S) {
-	if (substr($S['request'], 0, 5) != 'login') {
-		$_REQUEST['go']=$S['request'];
+	if (isset($_REQUEST['go']) && $_REQUEST['go'] == $S['request']) {
 		echo print_warning('Please sign in to access this page.');
 	}
 	if (isset($S['login.result'])) {
@@ -27,15 +38,13 @@ function body_login(&$S) {
 		} elseif ($S['login.result']) {
 			echo print_success('Welcome, '.$S['user']->name);
 			echo '<a href="'.url(isset($_REQUEST['go'])?$_REQUEST['go']:'').'">Continue</a>';
-die;
+			return;
 		} else {
 			echo print_error('Your email and password combination was not recognized.');
 		}
 	}
-	echo '<h3>Login</h3><form action="'.url('login').'" method="post">';
-	if (isset($_REQUEST['go'])) {
-		echo '<input type="hidden" name="go" value="'.htmlentities($_REQUEST['go']).'" />';
-	}
-	echo 'Email: <input name="email" /><br/>Password: <input type="password" name="password" /><br/><input type="submit" value="Submit" /></form>';
+	echo '<h3>Login</h3>';
+	echo $S['login']['form']->output($S['login']['data']);
+	echo '<a href="'.url('forgot').'">Forgot password?</a>';
 }
 ?>
diff --git a/frontend/pages/register.php b/frontend/pages/register.php
index 441269c..9f33e8b 100644
--- a/frontend/pages/register.php
+++ b/frontend/pages/register.php
@@ -4,8 +4,8 @@ function init_register(&$S) {
 		header('Location: '.url());
 		return 'welcome';
 	}
-	if (isset($_REQUEST['token']) && preg_match('/^[a-zA-Z0-9]{30}$/', $_REQUEST['token'])) {
-		$r=query('SELECT * FROM `registrationtokens` WHERE `id`=\''.$_REQUEST['token'].'\'');
+	if (isset($_REQUEST['token']) && strlen($_REQUEST['token']) == 30 && ctype_alnum($_REQUEST['token'])) {
+		$r=query('SELECT * FROM `registrationtokens` WHERE `id`="'.$_REQUEST['token'].'" AND `expire` > '.time());
 		if ($r->rowCount()) {
 			$S['register.token']=new sql_registrationtoken($r->fetch(PDO::FETCH_ASSOC));
 			if (isset($_REQUEST['password'])) {
@@ -37,7 +37,7 @@ function body_register(&$S) {
 			if (query('SELECT COUNT(*) FROM `users` WHERE `email`='.$S['pdo']->quote($_REQUEST['email']))->fetch(PDO::FETCH_COLUMN))
 				echo print_warning('An account already exists with this email address.').'<a href="'.url('login').'">Login</a>';
 			else {
-				if ($token=query('SELECT * FROM `registrationtokens` WHERE `email`='.$S['pdo']->quote($_REQUEST['email']))->fetch(PDO::FETCH_ASSOC)) {
+				if ($token=query('SELECT * FROM `registrationtokens` WHERE `expire` > '.time().' AND `email`='.$S['pdo']->quote($_REQUEST['email']))->fetch(PDO::FETCH_ASSOC)) {
 					echo print_warning('A confirmation email has already been sent to this email address... sending another email.');
 					$token=new sql_registrationtoken($token);
 				} else {
diff --git a/frontend/pages/users/forgot-password.php b/frontend/pages/users/forgot-password.php
new file mode 100644
index 0000000..b781cec
--- /dev/null
+++ b/frontend/pages/users/forgot-password.php
@@ -0,0 +1,29 @@
+<?php
+function init_users_forgot_password(&$S) {
+	if (isset($S['user'])) return 'login';
+	$S['title']='Forgot Password';
+}
+function body_users_forgot_password(&$S) {
+	$form=new form();
+	$form->text('<h3>Reset password</h3>');
+	$form->text_input('email', 'email', 'Email');
+	$form->submit();
+	if (isset($_REQUEST['email'])) {
+		$data=$form->process();
+		$r=query('SELECT * FROM `users` WHERE `email`='.$S['pdo']->quote($data['email']));
+		if ($r->rowCount()) {
+			$user=new sql_user($r->fetch(PDO::FETCH_ASSOC));
+			$token=sql_registrationtoken::create();
+			$token->owner=$user->id;
+			$token->email=$user->email;
+			$token->expire=time()+24*3600;
+			$token->write();
+			$url=url('reset?email='.urlencode($token->email).'&token='.$token->id);
+			xhtmlemail($user->email, $S['conf']['emailfrom'], $S['conf']['title'].' password', 'You requested to reset your '.$S['conf']['title'].' password.  You may do so by going to <a href="'.$url.'">'.$url.'</a> or by entering your email and the reset key "'.$token->id.'" at '.url('reset').'.  This link will expire in twenty-four hours.  If you did not request to reset your password, you may safely ignore this message.');
+		}
+		echo print_success('Success.', 'You have been sent an email (if you have an '.$S['conf']['title'].' account) with further instructions to reset your password.');
+	} else {
+		$form->output();
+	}
+}
+?>
diff --git a/frontend/pages/users/reset-password.php b/frontend/pages/users/reset-password.php
new file mode 100644
index 0000000..683a67a
--- /dev/null
+++ b/frontend/pages/users/reset-password.php
@@ -0,0 +1,49 @@
+<?php
+function init_users_reset_password(&$S) {
+	if (isset($S['user'])) return 'login';
+	$S['title']='Forgot Password';
+}
+function body_users_reset_password(&$S) {
+	$form1=new form();
+	$form1->text('<h3>Reset password</h3>');
+	$form1->text_input('email', 'email', 'Email');
+	$form1->text_input('token', 'token', 'Reset key');
+	$form1->submit();
+	$data=array();
+	if (isset($_REQUEST['email']) && ($data=$form1->process()) && $form1->verify($data)) {
+		$user=new sql_user($data['email']);
+		$token=new sql_registrationtoken(query('SELECT * FROM `registrationtokens` WHERE `expire` > '.time().' AND `id`='.$S['pdo']->quote($data['token']))->fetch(PDO::FETCH_ASSOC));
+		if ($token->email != $user->email) {
+			echo print_warning('Your email/key combination is invalid.');
+			$form1->output($data);
+		}
+		$form2=new form();
+		$form2->text('<h3>Reset password</h3>');
+		$form2->hidden('email', 'email', $data['email']);
+		$form2->hidden('token', 'token', $data['token']);
+		$form2->password('pass', 'pass', 'New password');
+		$form2->password('repeat', 'repeat', 'Repeat new password');
+		$form2->submit();
+		if (isset($_REQUEST['pass'])) {
+			$data=$form2->process();
+			if ($form2->verify($data)) {
+				if ($data['pass'] == $data['repeat']) {
+					$user->passhash=sha1($data['pass']);
+					$user->write();
+					$token->delete();
+					echo print_success('Password changed.', '<a href="'.url('login').'">Login</a>');
+				} else {
+					echo print_warning('The passwords you entered do not match.');
+					$form2->output($data);
+				}
+			} else {
+				$form2->output($data);
+			}
+		} else {
+			$form2->output($data);
+		}
+	} else {
+		$form1->output($data);
+	}
+}
+?>
diff --git a/frontend/routing.csv b/frontend/routing.csv
index 7183a43..06447b6 100644
--- a/frontend/routing.csv
+++ b/frontend/routing.csv
@@ -36,6 +36,8 @@ logout/(.+)	logout	go
 register	register
 register/([a-zA-Z0-9]{30})	register	token
 invite	invite
+forgot	users/forgot-password
+reset	users/reset-password
 # Pass through
 (js)/([0-9a-zA-Z-_]+\.(js))	passthrough	dir	file	ext
 (images)/([0-9a-zA-Z-_]+\.(gif|jpg|jpeg|ico|png))	passthrough	dir	file	ext
diff --git a/shared/functions/xhtmlemail.php b/shared/functions/xhtmlemail.php
index d4924f7..796a97c 100644
--- a/shared/functions/xhtmlemail.php
+++ b/shared/functions/xhtmlemail.php
@@ -12,7 +12,7 @@ function xhtmlemail($to,$from,$subj,$cont,$inheads=null) {
 	if ($inheads!==null) {
 		$heads.=$inheads."\r\n";
 	}
-	$cont='<?xml version="1.0" encoding="utf-8"?>'."\n".'<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">'."\n".'<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">'.$cont.'</html>'."\n";
+	$cont='<?xml version="1.0" encoding="utf-8"?>'."\n".'<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">'."\n".'<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en"><body>'.$cont.'</body></html>'."\n";
 	$heads.='Content-length: '.strlen($cont)."\r\n";
 	debug('mail', $heads.$cont);
 	return mail($to,$subj,$cont,$heads);
diff --git a/todo b/todo
index 7354cd0..1158b17 100644
--- a/todo
+++ b/todo
@@ -23,3 +23,4 @@ Ask someone to add the necessary USE flags to php on tinderbox
 Add rollback to backend so it can resume after a partial task
 Offer option in frontend to submit a failed build for resume
 Change builds->display() to handle `failed` column
+Find out what on earth is wrong with error reporting