contact form class

[the_cheat]

here is a class i made that creates and processes a contact form. I would appreciate it it you could look at it and critique it and give me any notes. Sorry it's not documented at all, I hope that's not a problem. thanks

Code: Select all

<?php
	ini_set('display_errors', 1);

	class ContactForm
	{
		var $form_block;
		var $name_err;
		var $address_err;
		var $message_err;
		var $name_min;
		var $name_max;
		var $address_min;
		var $address_max;
		var $message_min;
		var $message_max;
		var $send_status;
		var $receiver_address;
		
		function __construct($receiving_address)
		{
			$this->form_block = "
    			<form method=\"post\" action=\"$_SERVER[PHP_SELF]\">
    			<p><bold>Your name:</bold><br />
    			<input type=\"text\" name=\"txt_sender_name\" value=\"$_POST[txt_sender_name]\" /></p>
    			<p><bold>Your email address:</bold><br />
    			<input type=\"text\" name=\"txt_sender_address\" value=\"$_POST[txt_sender_address]\" /></p>
    			<p><bold>Message:</bold><br />
    			<textarea name=\"txta_message\" cols=30 rows=20></textarea></p>
    			<input type=\"hidden\" name=\"submit_pressed\" value=\"yes\" />
    			<p><input type=\"submit\" name=\"btn_submit\" value=\"Send\" /></p></form>";
    		$this->name_err = "";
			$this->address_err = "";
			$this->message_err = "";
			$this->name_min = 0;
			$this->name_max = 100;
			$this->address_min = 0;
			$this->address_max = 50;
			$this->message_min = 0;
			$this->message_max = 5000;
			$this->send_status = true;
			$this->receiver_address = $receiving_address;
    		
    			
    	}
    	
    	function perform() 
    	{
    		if ($_POST[submit_pressed] != "yes") {
    			echo $this->form_block;
    		} else if ($_POST[submit_pressed] == "yes") {
    			$this->validate_name($this->name_min, $this->name_max);
    			$this->validate_email($this->address_min, $this->address_max);
    			$this->validate_message($this->message_min, $this->message_max);
    			
    			if ($this->send_status == false) {
    				echo $this->name_err;
    				echo $this->address_err;
    				echo $this->message_err;
    				echo $this->form_block;
    			} else if ($this->send_status == true) {
    				$email_info = $this->build_email();
    				$this->send_form($email_info);
    			}
    		}
    	}
    	
    	function validate_name($min=0, $max=100) 
    	{
    		if ($_POST[txt_sender_name] == "") {
    			$this->name_err = "<font color=\"red\">Please enter your name</font><br />";
    		} else if ($_POST[txt_sender_name] < $min) {
    			$this->name_err = "<font color=\"red\">Your name must be larger than {$min} characters.</font><br />";
    		} else if ($_POST[txt_sender_name] > $max) {
    			$this->name_err = "<font color=\"red\">Your name cannot be longer than {$max} characters.</font><br />";
    		}
    		if ($this->name_err != "") {
    			$this->send_status = false;
    		}
    	}
    	
    	function validate_email($min=0, $max=50)
    	{
    		if ($_POST[txt_sender_address] == "") {
    			$this->address_err = "<font color=\"red\">Please enter your E-Mail Address.</font><br />";
    		} else if ($_POST[txt_sender_address] < $min) {
    			$this->address_err = "<font color=\"red\">Your E-Mail Address must be larger than {$min} characters.</font><br />";
    		} else if ($_POST[txt_sender_address] > $max) {
    			$this->address_err = "<font color=\"red\">Your E-Mail Address cannot be longer than {$max} characters.</font><br />";
    		} else if (!preg_match("^[_a-z0-9-]+(\.[_a-z0-9-]+)*@[a-z0-9-]+(\.[a-z0-9-]+)*(\.[a-z]{2,6})$^", $_POST[txt_sender_address])) {
    			$this->address_err = "<font color=\"red\">The E-Mail Address you entered is not valid, please enter a valid E-Mail Address.</font><br />";
    		}
    		if ($this->address_err != "") {
    			$this->send_status = false;
    		}
    	} 
    	
    	function validate_message($min=0, $max=5000) 
    	{
    		if ($_POST[txta_message] == "") {
    			$this->message_err = "<font color=\"red\">Please enter the message you would like to send.</font><br />";
    		} else if ($_POST[txta_message] < $min) {
    			$this->message_err = "<font color=\"red\">Your message must be at least {$min} characters long.</font><br />";
    		} else if ($_POST[txta_message] > $max) {
    			$this->message_err = "<font color=\"red\">Your message can be no longer than {$max} characters.</font><br />";
    		}
    		if ($this->message_err != "") {
    			$this->send_status = false;
    		}
    	}
    	
    	function build_email()
    	{
    		$msg = "Sender's name:		$_POST[txt_sender_name]\n";
    		$msg .= "Sender's address:	$_POST[txt_sender_address]\n";
    		$msg .= "Message:\n\n  \t$_POST[txta_message]\n\n";
    		$headers = "From:		My Web Site\n";
    		$headers .= "Reply-To:	$_POST[txt_sender_address]\n\n";
    		
    		return array($headers, $msg);
    	}
    	
    	function send_form($info)
    	{
    		$msg = $info[0];
    		$headers = $info[1];
    		$subject = "Contact Form From Website";
    		$result = mail($this->receiver_address, $subject, $msg, $headers);
    		if ($result) {
    			echo "Mailed successfully";
    		} else {
    			echo "Mailed unsuccessfully";
    		}
    	}
    }
?>

[Eran]

I'll be blunt - there's quite a few problems here. I'll go over what I see in a glance -
1. You mix PHP4 syntax with PHP5 syntax - using 'var' for properties (which is PHP4 - instead of PHP5 visibility keywords) but __construct() (which is PHP5). I suggest stick with one, preferably PHP5 since it has been out for over 6 years now.
2. You don't need to assign static values in the constructor, put those in the property list directly. Use the constructor to pass new values to the class properties. I suggest using an array for that.

Code: Select all

public function __construct($config = array())
{
    foreach( $config as $key => $val ) {
         if(isset($this -> $key)) {
              $this -> $key = $val;
         }
    }
}

3. You don't use array indexes properly. It should be either integers or strings. So this

Code: Select all

$_POST[submit_pressed]

Should really be this

Code: Select all

$_POST['submit_pressed']

4. Your final function, send_form(), outputs instead of just returning a result (boolean or string). If you returned instead of outputting you can send different output depending on the situation, or do other things with it (like logging silently, redirecting, etc)

[Christopher]

Eran has pointed out several more problems than I identified from a quick look at the code. Agree with all of it.

On the general design front, this seems like it should be multiple classes. I see at least a template script, but maybe a class to display the form and success/error messages. The you have the configuration and the perform() method which seem like the core form processing class. Then there are validators, which could each be a class to make the system more modular. And finally you send an email, which should probably be a separate class (or use something like SwiftMailer).