0

I am trying to make an inventory / invoice web application. The user enters information such as order ID, date, order total, and then each of the products bought along with their respective quantity. I'm using PDO for the sql queries.

I do not know in advance how many unique products are going to be in an invoice so I have an associative array that stores the products and their quantities (product name is used as the key) when the form is submitted.

On submit a prepared statement is built/executed.
Right now I have the order_id, date, and order_total query done. 

$stmt = $connection->prepare("INSERT INTO table_1 (order_id, order_date, order_total) VALUES ('$orderid', '$date', '$total_cost')");
$stmt->execute();

That part is simple enough. The aim of the other query is the following.

$testStmt = $connection->prepare("INSERT INTO table_2 (keys from the assoc array are listed here) VALUES (values from the assoc arrays are listed here)");
$testStm->execute();

My array would end up looking like this once the user inputs some products: 

$array
    (
        "product1" => quantity1
        "product2" => quantity2

    )

The idea I have had so far is to make a string for columns that need to be included in the sql query and then a string for the values for the sql query. Then iterate through the array and append the keys and values to the respective strings in such a way that I could use them in the sql query. I haven't gotten it to work and am worried that it could open myself up to sql injection (I am still quite unfamiliar with sql injection so I have been trying to read up on it).

$columns;
$values_input;

foreach($assoc_array as $product => $quant)
{
     $columns .= "' " . $product . "', ";
     $values_input .= "' " . $quant . "', ";
}

The idea being that $columns and $values_input string would end up containing all the appropriate column names and the quantities to be entered into those columns. Then I figured I could be able to use those strings as part of the SQL query. Something like this.

INSERT INTO $columns VALUES $values_input

I'd appreciate any help or insight. If I'm way off here or doing something in a retarded way feel free to shout about it, I'd rather fix a screw up than continue on with it if that's the case.

Csw
  • 105
  • 1
  • 8
  • You are really avoiding much of the benefit of using PDO and prepared statements by concatenating your values into the SQL string rather than preparing a statement with placeholders and binding the values. – Don't Panic Feb 29 '16 at 19:46

2 Answers2

1

Since you are trying to make an inventory/invoice application, do you happen to have a product database? If you do, you may want to use the product id instead of the product names as key. As product names sound like there could be duplicates or can change. If product name changes, you will have problems querying.

Do you accept products not in db to be entered into the invoice? If so, it adds some complications.

On SQL injections, you should sanitize input before using it for queries. Read: What's the best method for sanitizing user input with PHP?

Most modern frameworks have many built in protections against SQL injections if you do not query manually. So consider using them.

Many of them use active record pattern see: http://www.phpactiverecord.org/projects/main/wiki/Basic_CRUD (So you don't have to deal with writing queries manually like you do.)

An example of active record in a framework: https://www.codeigniter.com/user_guide/database/query_builder.html

Community
  • 1
  • 1
JC Lee
  • 2,337
  • 2
  • 18
  • 25
  • I have a table that contains the the UPCs in 1 column and then the product names in another. The way the user enters products in the form is handled with a dropdown menu which is populated by the products found in the product table. So the user can't enter a product not in the database currently since if it's not in the database it won't be in the dropdown menu. – Csw Feb 29 '16 at 19:33
  • This is way bad design my friend. You need to normalize your database before you work on this. – VikingBlooded Feb 29 '16 at 19:37
  • Your $assoc_array used for inserting should be using the UPCs. You may decide to insert the product names as well for record keeping (As an invoice should be a static document. Users don't expect product names to change on their invoice although the actual product names have changed.) But queries to the invoices should be done using the UPCs. – JC Lee Feb 29 '16 at 19:38
  • Oh, adding on. Queries depends on the use case. If the user is searching for the invoices, he may not know the product name changes. So you may want to query using the product names as well. – JC Lee Feb 29 '16 at 19:46
  • @vikingblooded can you go into detail a bit more by what is particularly bad about it? I have gone through one or two iterations so far, nothing is permanent with how the database / tables are setup for now. – Csw Feb 29 '16 at 19:57
  • @komirad using the UPCs for the queries was something I was considering so your suggestion makes that decision easier. Thanks again. – Csw Feb 29 '16 at 19:59
  • You should be creating auto-increment id's for the records in your tables and using those in a primary key -> foreign key relationship. You should be querying based on the key values. The reason for this is, UPCs can and have changed, and now you've got a problem. The keys should be independent of external data changes. – VikingBlooded Feb 29 '16 at 20:03
1

You are already using PDO, which is a good thing if you want to protect yourself from SQL injection. You are even trying to prepare your statement, but since you are not binding any parameters, one could argue if that is really what you are doing. Example 5 on the PHP docs page is in fact pretty close to what you want to do. Allow me to adapt it to your use case:

// create a placeholders string looking like "?, ?, ..., ?"
$placeholders = implode(',', array_fill(0, count($params), '?'));

// prepare the statement
$qry = $connection->prepare("INSERT INTO table_2 ($params) VALUES ($params)");

// bind the parameters to the statement. (We first need all columns, then all values)
$qry->execute(array_merge(array_keys($params), array_values($params)));

This should result in a query that looks exactly like your first example, but with a dynamic number of columns, or parameters. And since you are preparing your statement and binding the parameters on execution, PDO should handle all quoting and escaping to prevent SQL injection.

As a side note, your table structure seems a bit of to me. I don't think you normalized your data correctly, though it is a bit hard to tell with the table names you are using. I believe your structure should look something like this, and I fear it doesn't:

TABLE orders (id, date, total, client_id)
TABLE products (id, name, price, ...)
TABLE order_lines (id, order_id, product_id, quantity)
TABLE clients (...)

The exact structure obviously depends on your use case, but I believe this is about the simplest structure you can get away with if you want to build an order system that you can easily query and that can serve as a base for possible expansion in the future.

Pevara
  • 14,242
  • 1
  • 34
  • 47
  • The way I plan to have the tables setup in the end is the following: ` TABLE order (id, date, store_name, total) TABLE products (upc, name, price) TABLE order_items (order_id, product_id, quantity). ` That is far as I have gotten to for now with the database. Thank you very much for the example, this seems to be exactly what I have been trying to figure out. – Csw Feb 29 '16 at 20:09
  • That seems about right. You know, database normalization is in fact an exact science, there are "algorithms" you can follow (you are probably looking for the fourth normal form). It might be worth looking that up if you are interested. And you can always post a new question if you get stuck... – Pevara Feb 29 '16 at 20:16
  • I'll need to read up more on database normalization then. One final question, in the example you wrote up should the $placeholders be being used in place of $params when the statement is being prepared? Then the $params keys/values replace the "?" marks upon execute? I have been reviewing the example you provided and the one on php manual, am I misunderstanding it? – Csw Feb 29 '16 at 20:40
  • Indeed, you are understanding correct I think. On prepare you use a question mark for each 'dynamic' value, and definitely it that value comes from direct user input. On execution you 'replace' those question marks by providing an array of valus that will replace each question mark in order. (named parameters are also an option, but in this case that would just make thing harder) – Pevara Feb 29 '16 at 20:51