Calculating values returned from checkboxes using array_sum()

[terungwa]

I have built a dynamic list of Check Boxes with the query below.

while($row = mysqli_fetch_assoc($result))
{
	extract($row);
	$nospaces = str_replace(' ', '_', $life_event);	
	echo "<tr>
		<td style='width:30px'>$life_event_id</td>
		<td style='width:300px'>$life_event</td>
		<td><input type='checkbox' name='$nospaces' value='$Value' /></td>
	</tr>";
}

Each of the checkboxes has a numerical value which i want to add up and echo back to the user when he submits the form (please see code below).

I have gotten this far in attempting to extract values when the submit (calculate) button is clicked into an array.

Thereafter, i applied array_sum()  to return the sum of values in the $stress_score array.

 

However the array_sum($Stress_score) returns zero (0).

$Death_of_spouse = 'unchecked';
$Divorce = 'unchecked';
if (isset($_POST['calculate'])) 
{
        $Stress_score=array();
	if (isset($_POST['Death_of_spouse']) ) 
	{
		if ($Death_of_spouse == 'checked') 
		{
			$Stress_score[] = '100';
		}
	}
	if (isset($_POST['Divorce']) ) 
	{
		if ($Divorce == 'checked') 
		{
			$Stress_score[]= '20';
		}
	}
        echo "Your Stress Score = " . array_sum($Stress_score) . "\n";
}

I shall appreciate any thoughts on how to resolve this.

 

Thanks.

[Psycho]

You are giving all of the checkboxes the same name. You can't do that. Typically in those situations only the last field will be passed. You need to give each field a unique name or make the name an array. In this case, you shoudl make the field an array and use the life event ID as the index for each field. But, there are some problems that have to be resolved.

 

You are hard coding the stress factor for each stress event. This should be stored as part of the data for the events in the database. By, hard coding the values in the code you have created a dependency between the DB records and the code that makes this unmanageable. If you were to add an event in the future you would have to modify the code as well. That's a bad design.

 

A couple other tips. Don't use extract(). There are lots of reasons why that I won't go into. Also, I hope you are not using SELECT * for your query. Only select the fields you need.

 

So, here is what I would do:

 

1. Update the table to include the stress factor for each life event record

2. Create your checkboxes as an array with the value of each option as the life event ID

while($row = mysqli_fetch_assoc($result))
{
    echo "<tr>
            <td style='width:30px'>{$row['life_event_id']}</td>
            <td style='width:300px'>{$row['life_event']}</td>
            <td><input type='checkbox' name='lifeEvent[]' value='{$row['life_event_id']}' /></td>
          </tr>";
}

3. In the processing code, get the selected life event IDs passed in the POST data and get the sum of the stress factor by running a query.

if (isset($_POST['lifeEvents'])) 
{
    $checkedEvents = array_map('intval', $_POST['lifeEvents']);
    $checkedEventsCSV = implode(',', $checkedEvents);
    $query = "SELECT SUM(stress_factor) FROM life_events WHERE life_event_id IN ({$checkedEventsCSV})";
    $result = mysql_query($query);
    $stressScore = mysql_result($result, 0);
 
    echo "Your Stress Score = {$stressScore}\n";
}

You can now add/edit the data in the database without any changes needed to the code.

Edited June 24, 2014 by Psycho

[terungwa]

You are giving all of the checkboxes the same name. You can't do that. Typically in those situations only the last field will be passed. You need to give each field a unique name or make the name an array. In this case, you shoudl make the field an array and use the life event ID as the index for each field. But, there are some problems that have to be resolved.

 

You are hard coding the stress factor for each stress event. This should be stored as part of the data for the events in the database. By, hard coding the values in the code you have created a dependency between the DB records and the code that makes this unmanageable. If you were to add an event in the future you would have to modify the code as well. That's a bad design.

 

A couple other tips. Don't use extract(). There are lots of reasons why that I won't go into. Also, I hope you are not using SELECT * for your query. Only select the fields you need.

 

So, here is what I would do:

 

1. Update the table to include the stress factor for each life event record

2. Create your checkboxes as an array with the value of each option as the life event ID

while($row = mysqli_fetch_assoc($result))
{
    echo "<tr>
            <td style='width:30px'>{$row['life_event_id']}</td>
            <td style='width:300px'>{$row['life_event']}</td>
            <td><input type='checkbox' name='lifeEvent[]' value='{$row['life_event_id']}' /></td>
          </tr>";
}

3. In the processing code, get the selected life event IDs passed in the POST data and get the sum of the stress factor by running a query.

if (isset($_POST['lifeEvents'])) 
{
    $checkedEvents = array_map('intval', $_POST['lifeEvents']);
    $checkedEventsCSV = implode(',', $checkedEvents);
    $query = "SELECT SUM(stress_factor) FROM life_events WHERE life_event_id IN ({$checkedEventsCSV})";
    $result = mysql_query($query);
    $stressScore = mysql_result($result, 0);
 
    echo "Your Stress Score = {$stressScore}\n";
}

You can now add/edit the data in the database without any changes needed to the code.

Thanks Psycho for the response, your approach is brillaint and future proof. I applied the query as below:

if (isset($_POST['lifeEvents'])) 
{
        // build the callback function
	$intval = function ($val) 
	{
	   if(is_numeric($val))
	   {
			$val = "$val,"; // add commas to values to separate them
	   }	 
	   return $val;
	};
    $checkedEvents = array_map($intval, $_POST['lifeEvents']);
    $checkedEventsCSV = implode(',', $checkedEvents);
    $query = "SELECT SUM(stress_factor) as stress FROM life_events WHERE life_event_id IN ({$checkedEventsCSV})"; // initialize statement
    // Create a statement object
    $stmt = $mysqli->stmt_init();
    // Prepare the statement for execution
    $stmt->prepare($query);
    // Execute the statement
    $stmt->execute();
    // Bind the result parameters
    $stmt->bind_result($stress);
    // Cycle through the results and output the data
	while($stmt->fetch())
	printf("%s<br />", $stress);
 
    //echo "Your Stress Score =". $stress;
}

I am getting these errors in the browser:

1. Warning: array_map(): Argument #2 should be an array in ....
2. Warning: implode(): Invalid arguments passed in ....
3. Warning: mysqli_stmt::execute(): invalid object or resource mysqli_stmt in ......

The argument #2 $_POST['lifeEvents'] used in the array_map function is supposed to be an array so I wonder where the error is coming from.

 

Any thoughts?

 

Thanks.

[Psycho]

Where's the code you are using to create the checkbox fields? I'm guessing they aren't created as an array

[terungwa]

This is the code, i am using to generate the checkboxes.

echo "<form action='' method='POST'>
       <table>
	<thead>
	    <th>Serial#</th>
	    <th>Life Event</td>
	    <th>Check if this applies</td>
	</thead>
	<tbody>";
       while($row = mysqli_fetch_assoc($result))
       {
	    echo "<tr>
		   <td style='width:30px'>{$row['life_event_id']}</td>
                   <td style='width:300px'>{$row['life_event']}</td>
                   <td><input type='checkbox' name='lifeEvent[]' value='{$row['life_event_id']}' /></td>
		</tr>";
       }
       echo "</tbody>
    </table>";
   echo "<p><input type='submit' name ='calculate' value='Calculate Total' /></p>
</form>";

And if it helps, this is the Table structure for table `life_events`

CREATE TABLE IF NOT EXISTS `life_events` (
  `life_event_id` int(10) unsigned NOT NULL AUTO_INCREMENT,
  `life_event` varchar(41) DEFAULT NULL,
  `stress_factor` int(11) NOT NULL,
  PRIMARY KEY (`life_event_id`)
) ENGINE=InnoDB  DEFAULT CHARSET=utf8 AUTO_INCREMENT=44 ;

Edited June 25, 2014 by terungwa

[Psycho]

The problem is a typo. The form fields are using "lifeEvent", but the PHP code is using "lifeEvents". Change the name to be the same in both.

[mac_gyver]

also, the $intval = function ($val)...  callback function you have defined in your code has two three problems -

 

1) is_numeric() will allow a lot of values that aren't unsigned positive integers, which is what your query expects. in fact, it will allow a hexadecimal value which mysql happily converts to a string of characters corresponding to the hexadecimal value (depending on context) and may allow sql injection. your callback function should only allow positive integers greater than zero.

 

is_numeric() -

 

Finds whether the given variable is numeric. Numeric strings consist of optional sign, any number of digits, optional decimal part and optional exponential part. Thus +0123.45e6 is a valid numeric value. Hexadecimal (e.g. 0xf4c3b00c), Binary (e.g. 0b10100111001), Octal (e.g. 0777) notation is allowed too but only without sign, decimal and exponential part.

 

 

 

2) you should not be adding the , (comma) after each value as that will leave an extra comma at the end and putting the commas between values is what the implode() statement is for.

 

3) when the value isn't numeric, you are returning the value as is, allowing direct sql injection.

Edited June 25, 2014 by mac_gyver

[Barand]

Make the stress_factor the value of your check box then there is no need to requery on receipt of the POST data, simply array_sum() the data

<td><input type='checkbox' name='lifeEvent[$row['life_event_id']}]' value='{$row['stress_factor']}' /></td>

then

$totalStress = array_sum($_POST['lifeEvent'];

[Psycho]

 

Make the stress_factor the value of your check box then there is no need to requery on receipt of the POST data, simply array_sum() the data

 

I thought about that too. If this is just an learning exercise or for personal use that is fine.

 

But what people ask for is almost never what they really need. Although, the script (as it is now) is just echoing the total to the page I didn't want to assume that it was just throwaway data such as that. For all I know the score would be stored as part of a test/exercise/evaluation. It *may* be something that a person would be interested in manipulating the results through the submission data. So, I (almost) always only pass the selections and do all calculations from the live data on the back end.

[terungwa]

also, the $intval = function ($val)...  callback function you have defined in your code has two three problems -

 

1) is_numeric() will allow a lot of values that aren't unsigned positive integers, which is what your query expects. in fact, it will allow a hexadecimal value which mysql happily converts to a string of characters corresponding to the hexadecimal value (depending on context) and may allow sql injection. your callback function should only allow positive integers greater than zero.

 

is_numeric() -

 

 

2) you should not be adding the , (comma) after each value as that will leave an extra comma at the end and putting the commas between values is what the implode() statement is for.

 

3) when the value isn't numeric, you are returning the value as is, allowing direct sql injection.

@mac_gyver, i  needed to check that the submitted value is a number, hence the use of the is_numeric casting operator, but the issues you highlighted above are valid as the is_numeric() function accepts any type of number, including a hexadecimal one or a string containing a numeric value.

 

Would the is_int() function be a better replacement, or as a precaution i should pass the is_numeric value  to the abs() function, which

converts a number to its absolute value?

 

Thanks

[terungwa]

The problem is a typo. The form fields are using "lifeEvent", but the PHP code is using "lifeEvents". Change the name to be the same in both.

Thank you Psycho, that was the bug.

Problem solved.

[terungwa]

 

Make the stress_factor the value of your check box then there is no need to requery on receipt of the POST data, simply array_sum() the data

<td><input type='checkbox' name='lifeEvent[$row['life_event_id']}]' value='{$row['stress_factor']}' /></td>

then

$totalStress = array_sum($_POST['lifeEvent'];

@Barand, your method also worked, but the solution proposed by Psycho offers me flexibility for a lot of other stuff, I had not initially thought of at the beginning of this little exercise!!

[terungwa]

@mac_gyver, i  needed to check that the submitted value is a number, hence the use of the is_numeric casting operator, but the issues you highlighted above are valid as the is_numeric() function accepts any type of number, including a hexadecimal one or a string containing a numeric value.

 

Would the is_int() function be a better replacement, or as a precaution i should pass the is_numeric value  to the abs() function, which

converts a number to its absolute value?

$intval = function ($val) 
{
   if(is_numeric($val))
   {
     $val = abs($val);
       return $val;
   }
   else
   {
	die('invalid data input');
   }
};

Thanks

[Jacques1]

See this thread for a detailed explanation why it's a bad idea to run user input through intval() or any other string-to-integer converter.

 

The very first reason (value truncation) already applies to you: Your life_event_id is an UNSIGNED INT, which means the IDs can range from 0 to 2³² − 1. However, if you run the application in a 32-bit PHP, then a PHP integer can only cover the first half from 0 to 2³¹ − 1. Any ID above that cannot be represented and is silently reduced to 2³¹ − 1.

 

That's obviously a problem. The solution is to not mangle the user input. Don't run it through intval(), abs() or whatever. Just use a prepared statement to pass the values to the database system in a secure manner.

 

There also seems to be a lot of confusion regarding is_int(), is_numeric() etc. As mac_gyver already explained, you cannot use is_numeric(), because it simply doesn't do what you want. This is for any number (not just integers) in all kinds of “strange” formats. You can't use is_int() either, because this literally checks if the value is a PHP integer. Of course it's not, it's a string.

 

What you want is ctype_digit(). This checks if the string only contains decimal digits, which is exactly how the decimal representation of a non-negative integer looks like.
 

To wrap it up:

• validation with ctype_digit()
• security through a prepared statement