Code works but is it safe? Good form?

cal

Code works but is it safe? Good practice?

Attached code is long (sorry - 62 lines) but well commented. I edited an existing script that is maybe 4 years old. I am just starting to do real projects and have not yet absorbed all the information I have read.

We have the form on our server and are redirecting folks to a secure server for payment. In the meantime, we are writing contact info to a text file for our use.

<?php
$fname = $_POST ['fname'];
$lname = $_POST ['lname'];
...snip....
	$form_status = ""; // initialization
	if( count($_POST) ) { // if there has been a form submittal, this will be > 0 (interpreted as true)
		// this first block creates the file & the variable name row. 		
		if(!file_exists("reg.txt")) { // if the reg.txt file does not exist~
			$fp = fopen("reg.txt", "wb") or die("fopen failed!<br>\n"); // create and open the file for binary write (wb)
			chmod("reg.txt", 0777) or die("chmod failed!<br>\n"); // chmod the file so that other participants can alter the file
			$j = 0; // counter for the foreach command, since foreach uses the array's native element names and not a number
			foreach($_POST as $i => $val) { // foreach loop in form elements, each array element name is set to $i, its value set to $val
				if( $j != count($_POST) - 1 ) // if we are not on the last element of the array,
					fwrite($fp, $i."\t"); // write data with a tab
				else // if we ~are~ on the last element of the array,
					fwrite($fp, $i."\n"); // write data with a new line
				$j = $j + 1; // increment the counter
			}
			fclose($fp); // close the file
			$form_status = "disabled"; // file write success, so we disable the form	
			header("refresh:5; url=https:someplace");  //create link using form variables & redirect 
		}
		// this block appends the file with a single line of all the responses in the same order as the variable list 		
		if($fp = fopen("reg.txt", "ab")) { // open the file appending in binary mode
			$j = 0; // counter set to zero
			foreach($_POST as $i => $val) { // foreach loop in form elements, each arry element name is set to $i, its value set to $val
				if( $j != count($_POST) - 1 ) // if we are not on the last element of the arry,
					fwrite($fp, $val."\t"); // write data with a tab
				else // if we ~are~ on the last element of the array,
					fwrite($fp, $val."\n"); // write data with a new line
				$j = $j + 1; // increment the counter
			}
			fclose($fp); // close the file
			$form_status = "disabled"; // file write success, so we disable the form
	//create link using form variables & redirect	
	header("refresh:5; url=https:someplace");
		}
		else echo "Failed to open file! (check to make sure it's chmod'd 777)<br>\n"; // failed to open the file on the append command
	} 
?>   

        <? if( $form_status == "" ) { ?>

  <form action="<? echo $PHP_SELF; ?>" method="post" name="form" id="form" onsubmit="return confirm('Are you ready to submit this form?')">
    <p><label for="fname">*First Name:</label>
    <input <? echo $form_status; ?> name="fname" type="text" id="fname" size="20" />
    </p>
    <p >
      <label for="lname">*Last Name:</label>
      <input <? echo $form_status; ?> name="lname" type="text" id="lname" size="20" />
    </p>     
  ...snip...
      <input <? echo $form_status; ?> name="submit" type="submit" value="Submit" />
      <input <? echo $form_status; ?> type="reset" name="Reset" value="Reset" />
    </font>(After you click submit, you will be given payment information)</span></p>
  </form>
       <? } else { ?>
       <h1 >Thank you! </h1>
  <h2 >Redirecting to payment site...</h2>
  <p >If you have trouble paying for the conference, use the following link:<a href="https:someplace"></a></p>
  <? } ?>
  ...snip...
  </html>

Thank you for your time and expertise.
Cal

bradgrafelman

Thread moved to Code Critique forum. Also, 62 lines is what I would call a very, very short script... short enough that you can just include it in the body of your post (wrapping it with [noparse]

..

[/noparse] bbcode tags, of course!) as I've done for you.

As for a general code critique/review...

Code such as:
```
$fname = $_POST ['fname'];
$lname = $_POST ['lname'];
```
will cause E_NOTICE level error messages if those array indexes don't already exist. As such, I like to define such variables like so:
```
$foo = (isset($_POST['foo']) ? $_POST['foo'] : NULL);
```
since that a) allows me to define a default value (NULL) if the array index isn't defined, b) allows me to do any generic casting/transforming of the data before it's used anywhere in the script, and c) does (a) and (b) cleanly without generating any PHP error messages.

This entire code:

			$j = 0; // counter for the foreach command, since foreach uses the array's native element names and not a number
			foreach($_POST as $i => $val) { // foreach loop in form elements, each array element name is set to $i, its value set to $val
				if( $j != count($_POST) - 1 ) // if we are not on the last element of the array,
					fwrite($fp, $i."\t"); // write data with a tab
				else // if we ~are~ on the last element of the array,
					fwrite($fp, $i."\n"); // write data with a new line
				$j = $j + 1; // increment the counter
			}

could be more efficiently (and more correctly) written using a single function call to [man]fputcsv/man.

NOTE: This applies for the second occurrence of this same code structure later on as well.

You could minimize file I/O by not opening and closing the same file twice - just leave it open for the subsequent write as well and do both at once.
Here:
```
$form_status = "disabled"; // file write success, so we disable the form
```
you're defining an HTML attribute that doesn't have a value - that should probably instead add disabled="disabled" so that you're outputting valid HTML.

Speaking of valid HTML, have you validated yours?
Note that since PHP's short open tags ("<?" and "<?=") can be disabled (and often are), I find that the "best practice" that will guarantee portability is to never rely on these tags being enabled. In other words, you should always use the full "<?php" opening tag when writing PHP code.
Where is $PHP_SELF ever defined? This is likely generating an error about an undefined variable - perhaps you meant $_SERVER['PHP_SELF'] instead?
While it might work okay for smaller projects, note that once you move onto "real" projects you'll find it more and more beneficial (or even required) to separate design/display code from server-side executable code.

In other words, you'll want to separate all of the HTML from your PHP scripts (i.e. via template engines and whatnot).

EDIT: Also, welcome to PHPBuilder! 🙂