Is anything wrong with this code?

[garyed]

It seems to work fine but I would like to hear what you guys who are more versed in php think.

In my previous post I was trying find a way to hide the errors but I really want to get things right this time.

This is a sample code that I'm using as a test but on my site I'm going to be pulling my variables off a database & using a while loop to fill in all 50 states.   

<?php 
error_reporting(E_ALL);
ini_set('display_errors', '1');
session_start(); // Starts session & uses function to keep data  */
function get_value($var)
{
// this will check your session if it's not empty and you send an empty post
if ($_POST[$var]!="" || (!empty($_SESSION[$var]) && $_POST[$var] === '')) { 
   $_SESSION[$var]=$_POST[$var];   }
if (isset($_SESSION[$var])){ return $_SESSION[$var];} else{ return $_POST[$var];} 
}
if(!isset($_POST['submitter'])) {$_POST['submitter']="anything"; }  // initialize to avoid Undefined array key error
?>
<form name="test" action="" method="post"> 
<input type="text" style="width:255px;" name="owner" value="<?php if(isset($_POST['owner'])) {echo get_value('owner'); } else { if(isset($_SESSION['owner'])){ echo $_SESSION['owner'];} } ?>"> <br>
<input type="text" style="width:255px;" name="renter" value="<?php if(isset($_POST['renter'])) {echo get_value('renter');}else { if(isset($_SESSION['renter'])){ echo $_SESSION['renter'];} } ?>"> <br>
<select name="state" >  <option value=""> select state </option>
<?php
$state1="Maine" ;
$state2="Texas" ;
?>
<option value="<?php echo $state1; ?>"  <?php  if(isset($_POST['state'])) { if(get_value('state') == $state1) { echo 'selected="selected"'; } }  else { 
if(isset($_SESSION['state'])){ if ($_SESSION['state']==$state1) { echo 'selected="selected"'; } } }  ?>  > Maine </option>   
<option value="<?php echo $state2; ?>"  <?php if(isset($_POST['state'])) {if (get_value('state')==$state2) { echo 'selected="selected"'; }}  else { 
if(isset($_SESSION['state'])){ if ($_SESSION['state']==$state2) { echo 'selected="selected"'; } } }  ?>  > Texas </option>  
</select> <br> <br> <br>
<input type="submit" value="submit"> &nbsp; &nbsp; <input name="submitter" type="submit" value="clear session"><br>
</form>
<?php
if ($_POST['submitter']=="clear session") { echo $_POST['submitter']."<br>"; session_destroy(); };
print_r($_SESSION);
?>

 

[ginerjm]

Way too complicated.  I tried to rewrite this but got lost in your logic and your very convoluted coding methods.

Resolve your input issues before creating your html code.  Get the values you want to use by returning it from the function and then use that value in your html so that you don't have to use a php tag in the middle of your html.  Very bad coding IMHO.

And I strongly recommend using heredocs;  Here is an example of how it works for you:

$sel1 = $sel2 = '';
$value1 = get_value(???);
if ($value1 == $state1)
	$sel1 = "selected='selected'";
$value2 = get_value(???);
$dropdown=<<<heredocs
<select name='list'>
<option value=''>Select a State</option>
<option value='$value1' $sel1>$value1</option>
<option value='$value2' $sel2>$value2</option>
</select>
heredocs;

Now use the $dropdown var in your html output area.  You can see how I avoided leaving php mode and let the heredocs combine the assigned values with the html.  Not sure my code makes sense (as your code was confusing me too) but this shows you a cleaner way of coding.

If I read it correctly you have some session values that are to default your desired values when they are not in the POST'ed input.  You do know that POST values from text inputs are always set so you don't have to do an isset on those parts.  They may be blank but at least they are present as blanks.

[garyed]

1 hour ago, ginerjm said:

Way too complicated.  I tried to rewrite this but got lost in your logic and your very convoluted coding methods.

Resolve your input issues before creating your html code.  Get the values you want to use by returning it from the function and then use that value in your html so that you don't have to use a php tag in the middle of your html.  Very bad coding IMHO.

And I strongly recommend using heredocs;  Here is an example of how it works for you:

$sel1 = $sel2 = '';
$value1 = get_value(???);
if ($value1 == $state1)
	$sel1 = "selected='selected'";
$value2 = get_value(???);
$dropdown=<<<heredocs
<select name='list'>
<option value=''>Select a State</option>
<option value='$value1' $sel1>$value1</option>
<option value='$value2' $sel2>$value2</option>
</select>
heredocs;

Now use the $dropdown var in your html output area.  You can see how I avoided leaving php mode and let the heredocs combine the assgned values with the html.  Not sure my code makes sense (as your code was confusing me too) but this shows you a cleaner way of coding.

If I read it correctly you have some session values that are to default your desired values when they are not in the POST'ed input.  You do know that POST values from text inputs are always set so you don't have to do an isset on those parts.  They may be blank but at least they are present as blanks.

Thanks,

I'll check into "heredocs". I never even heard of it before you just mentioned it. 

I'm also going to study your code until I understand it better.

What I'm really trying to do is to havel all 50 states in the dropdown menu from a MYSQL database & put whichever one is selected into the session. Then change the page to perform another task then return to the page & have the state from the session be already selected upon return.     

[mac_gyver]

for the activity you have shown in this thread and using the previously given programming practices about how to organize, cleanup, and code your form processing and form,  a lot of this typing of code goes away, you would end up with the following -

<?php
/*
put any error relating settings in the php.ini on your system
you may have a need to store the result from some step in session variable(s), but by unconditionally storing each piece of post data, you are doubling the amount of code needed. only store the end result. Keep It Simple (KISS.)
to dynamically generate a select/option list, you would not use discrete variables for each value and write out logic for each option. you would instead have an array of the values, then loop to dynamically build the options.
in html5, an empty action='' attribute is not valid. to get a form to submit to the same page it is on, leave the action attribute completely out of the form tag.
you should validate your resulting web pages at validator.w3.org
*/

// initialization
session_start();

$post = []; // an array to hold a trimmed working copy of the form data
$errors = []; // an array to hold user/validation errors

// post method form processing
if($_SERVER['REQUEST_METHOD'] === 'POST')
{
	// trim all the data at once
	$post = array_map('trim',$_POST); // if any of the fields are arrays, use a recursive trim call-back function here instead of php's trim function

	// validate inputs
	if($post['owner'] === '')
	{
		$errors['owner'] = 'The owner is required.';
	}
	if($post['renter'] === '')
	{
		$errors['renter'] = 'The renter is required.';
	}
	if($post['state'] === '')
	{
		$errors['state'] = 'The state is required.';
	}
	
	// add validation for other inputs here...

	
	// if no errors, use the form data
	if(empty($errors))
	{
		// if this is a step in a multi-step process, store the now validated data in a specific session variable
		$_SESSION['step'][1] = $post;
	}

	// if no errors, success
	if(empty($errors))
	{
		// if you want to display a one-time success message, store it in a session variable here,
		// then test, display, and clear that session variable at the appropriate point in the html document
		$_SESSION['success_message'] = 'Some success message for this step in the process';
		// redirect to the exact same url of the current page to cause a get request for the page
		die(header("Refresh:0"));
	}
}

// get method business logic - get/produce data needed to display the page
// query to get the states in the order that you want them

// fake some values
$states = [];
$states[]="Maine";
$states[]="Texas";

// html document
?>
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
	<title>Form processing/form example</title>
</head>
<body>

<?php
// display and clear any success message
if(isset($_SESSION['success_message']))
{
	echo "<p>{$_SESSION['success_message']}</p>";
	unset($_SESSION['success_message']);
}
?>

<?php
// display any errors
if(!empty($errors))
{
	echo "<p>".implode('<br>',$errors)."</p>";
}
?>

<form method="post">
<label>Owner: <input type="text" style="width:255px;" name="owner" value="<?=htmlentities($post['owner']??'',ENT_QUOTES)?>"></label><br>
<label>Renter: <input type="text" style="width:255px;" name="renter" value="<?=htmlentities($post['renter']??'',ENT_QUOTES)?>"></label><br>
<label>State: <select name="state">
<option value="">select state</option>
<?php
foreach($states as $state)
{
	$sel = isset($post['state']) && $post['state'] === $state ? 'selected' : '';
	echo "<option value='$state'$sel>$state</option>";
}
?>
</select></label><br>
<input type="submit">
</form>
  
<?php
// note: if you have the need for a 'clear' session function, add that as a separeate post method form, with a hidden field with a value to indicate that action
// then test for that action value in the form processing code to clear the session data
?>

<?php
// display the content of the session
if(!empty($_SESSION))
{
	echo '<pre>'; print_r($_SESSION); echo '</pre>';
}
?>

</body>
</html>

and as already mentioned, if you have more than 2-3 form fields, you should use a data-driven design, where you have a data structure (database table,  array) that defines the expected form fields (for each step), validation rules, and processing for each field, then dynamically validate and process the form data, rather than to write out bespoke logic for each field.

[garyed]

2 hours ago, mac_gyver said:

for the activity you have shown in this thread and using the previously given programming practices about how to organize, cleanup, and code your form processing and form,  a lot of this typing of code goes away, you would end up with the following -

<?php
/*
put any error relating settings in the php.ini on your system
you may have a need to store the result from some step in session variable(s), but by unconditionally storing each piece of post data, you are doubling the amount of code needed. only store the end result. Keep It Simple (KISS.)
to dynamically generate a select/option list, you would not use discrete variables for each value and write out logic for each option. you would instead have an array of the values, then loop to dynamically build the options.
in html5, an empty action='' attribute is not valid. to get a form to submit to the same page it is on, leave the action attribute completely out of the form tag.
you should validate your resulting web pages at validator.w3.org
*/

// initialization
session_start();

$post = []; // an array to hold a trimmed working copy of the form data
$errors = []; // an array to hold user/validation errors

// post method form processing
if($_SERVER['REQUEST_METHOD'] === 'POST')
{
	// trim all the data at once
	$post = array_map('trim',$_POST); // if any of the fields are arrays, use a recursive trim call-back function here instead of php's trim function

	// validate inputs
	if($post['owner'] === '')
	{
		$errors['owner'] = 'The owner is required.';
	}
	if($post['renter'] === '')
	{
		$errors['renter'] = 'The renter is required.';
	}
	if($post['state'] === '')
	{
		$errors['state'] = 'The state is required.';
	}
	
	// add validation for other inputs here...

	
	// if no errors, use the form data
	if(empty($errors))
	{
		// if this is a step in a multi-step process, store the now validated data in a specific session variable
		$_SESSION['step'][1] = $post;
	}

	// if no errors, success
	if(empty($errors))
	{
		// if you want to display a one-time success message, store it in a session variable here,
		// then test, display, and clear that session variable at the appropriate point in the html document
		$_SESSION['success_message'] = 'Some success message for this step in the process';
		// redirect to the exact same url of the current page to cause a get request for the page
		die(header("Refresh:0"));
	}
}

// get method business logic - get/produce data needed to display the page
// query to get the states in the order that you want them

// fake some values
$states = [];
$states[]="Maine";
$states[]="Texas";

// html document
?>
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
	<title>Form processing/form example</title>
</head>
<body>

<?php
// display and clear any success message
if(isset($_SESSION['success_message']))
{
	echo "<p>{$_SESSION['success_message']}</p>";
	unset($_SESSION['success_message']);
}
?>

<?php
// display any errors
if(!empty($errors))
{
	echo "<p>".implode('<br>',$errors)."</p>";
}
?>

<form method="post">
<label>Owner: <input type="text" style="width:255px;" name="owner" value="<?=htmlentities($post['owner']??'',ENT_QUOTES)?>"></label><br>
<label>Renter: <input type="text" style="width:255px;" name="renter" value="<?=htmlentities($post['renter']??'',ENT_QUOTES)?>"></label><br>
<label>State: <select name="state">
<option value="">select state</option>
<?php
foreach($states as $state)
{
	$sel = isset($post['state']) && $post['state'] === $state ? 'selected' : '';
	echo "<option value='$state'$sel>$state</option>";
}
?>
</select></label><br>
<input type="submit">
</form>
  
<?php
// note: if you have the need for a 'clear' session function, add that as a separeate post method form, with a hidden field with a value to indicate that action
// then test for that action value in the form processing code to clear the session data
?>

<?php
// display the content of the session
if(!empty($_SESSION))
{
	echo '<pre>'; print_r($_SESSION); echo '</pre>';
}
?>

</body>
</html>

and as already mentioned, if you have more than 2-3 form fields, you should use a data-driven design, where you have a data structure (database table,  array) that defines the expected form fields (for each step), validation rules, and processing for each field, then dynamically validate and process the form data, rather than to write out bespoke logic for each field.

I'm tried your code but it still doesn't insert the session data in the fileds & dropdown when I leave & return. 

I'm not sure what I'm missing but the only thing I know to do is to add something similar to what I already have to get it to work. 

[mac_gyver]

i was able to make the code repopulate the fields/select-option, when navigating around, using 3 lines of code and one conditional test, added to the get method business logic section. i won't post the code i came up with because it is probably not how your application is determining which step/page it is on. you could also just merge the successful $post data into the $_SESSION['step'] array inside the post method form processing code, then when the page is visited without any $post data, copy the whole $_SESSION['step'] array back into $post (which i just tested and it works as well.)

if you want help with anything your application is or is not doing, you must post enough of the code so that someone here can reproduce what it is doing.

[garyed]

1 hour ago, mac_gyver said:

i was able to make the code repopulate the fields/select-option, when navigating around, using 3 lines of code and one conditional test, added to the get method business logic section. i won't post the code i came up with because it is probably not how your application is determining which step/page it is on. you could also just merge the successful $post data into the $_SESSION['step'] array inside the post method form processing code, then when the page is visited without any $post data, copy the whole $_SESSION['step'] array back into $post (which i just tested and it works as well.)

if you want help with anything your application is or is not doing, you must post enough of the code so that someone here can reproduce what it is doing.

I appreciate the help. It's a lot of good information that I've got to process. 

 I'm a little slow at times but i'll eventually get it. 

[mac_gyver]

now that we know a bit more about what you are doing, a multi-page form, collecting data that eventually gets used for some purpose, now would be a good time to switch to using a data-driven design, to eliminate all the repetitive code for the data collection, rather than to spend time fixing each page of it.