MySQL update with PHP; MySQLi prepared statement in loop

Question

I'm going to develop an Android application that performs simple MySQL operations by invoking server-sided PHP scripts. PHP-MySQL communication is done by MySQLi extension. Each operation that I'd like to perform is in a separate PHP file, and in a separate class. There's an example of update.php:

<?php require_once 'functions.php'; if (empty($_POST)) { $_POST['UPDATE_DATA'] = array('numer_vin' => 'SIBISIBI', 'model_pojazdu' => 'Vectra', 'marka' => 'Opel'); $_POST['UPDATE_HEADERS'] = array('table' => 'polisy_oc', 'id' => 'numer_polisy_oc', 'id_value' => '72451'); } class UpdateRecord { protected $db_connection; public function __construct($db_connection) { $this->db_connection = $db_connection->connection; $this->update($_POST['UPDATE_HEADERS'], $_POST['UPDATE_DATA']); } private function update($update_headers, $update_data) { //TODO:single query if (in_array($update_headers['table'], $_SESSION['tables_white_list']) && in_array($update_headers['id'], $_SESSION['columns_white_list'])) { foreach ($update_data as $key => $value) { if (checkColumn($key)) { $update = $this->db_connection->prepare("UPDATE {$update_headers['table']} SET $key = ? WHERE {$update_headers['id']} = ?"); $update->bind_param('ss', $value, $update_headers['id_value']); $update->execute(); $update->close(); } } } } } $db = new Database; $update = new UpdateRecord($db); ?> 

My main concern is whether Depency Injection is properly used regarding database connection (all my other scripts do the same thing). Here's how Database class is built:

class Database { public $connection; function __construct() { if(!isset($_SESSION)){ session_start(); } $this->connection = new mysqli('localhost', $_SESSION['login'], $_SESSION['password'], 'Ubezpieczenia_OC'); if($this->connection->connect_error) { doDatabaseConnectionError(); } else { $this->connection->set_charset("utf8"); if(!isset($_SESSION['tables_white_list']) || !isset($_SESSION['columns_white_list'])){ $_SESSION['tables_white_list'] = array('klienci', 'polisy_oc', 'parametry_oc', 'modele_samochodow', 'marki_samochodow'); $_SESSION['columns_white_list'] = getTablesColumns($this->connection); } } } } 

Secondly, it was extremely hard for me to perform MySQL UPDATE query by mysqli extension's prepared statements for variable number of columns. Is above foreach loop solution a serious performance bottleneck and not elastic solution?

Answer 1

I'll stick to reviewing your PHP code, as it doesn't look like it's recieved the attention it deserves!

I'll do what I can to find faults, but I won't be testing anything. Alright, we ready?

• Nice! First line and we've found something! What kind file is functions.php!? Filenames should be one of the clearest names in your project. If you can't come up with a good filename, then that's a sign you need to break apart the contents within.
• I'm assuming the initial conditional empty() is for testing only? I hope so, because automatically updating data on page load is a disaster waiting to happen. Send a couple hundreds requests to a page that's doing resource heavy database access and you'll happily be sitting in a DoS attack.

• It's a little hard to tell, but it looks like those variable sets in that conditional are longer than any desired line length. It makes it much harder for the reader to effienctly read your code if they have to do a lot of horizontal scrolling.

  You can reduce this a couple ways. In this case, break down your array over multiple lines:

  $_POST['UPDATE_HEADERS'] = array( 'table' => 'polisy_oc', 'id' => 'numer_polisy_oc', 'id_value' => '72451' ); 

  This way is substantially easier to read in my opinion.

  For other really long lines, you may be able to put data in variables, and then reference those variables.

• @200_success had a good point: UpdateRecord isn't a very good name. Although it's hard to rename this class because it really doesn't have a single responsibility. That in itself contradicts the principles of SOLID. You might look towards making a Record class.
• __construct($db_connection) should really be __construct(Database $db_connection) to better implement dependency injection practices.
• I'd avoid automatically calling update() from the constructor. Make update() public and call it where the object is initialized.

• If update() were in a Record class, that'd be OK. But here, I don't think the name is right. For all I know, this could really be updating the record updater!

  If you don't change the class name, I suggest this be a method called updateRecord().

• Where do we get $_SESSION from? The user. And what's our golden rule? Never trust the user. I think it's a bad idea to check the POST data against session data. Make a concrete white-list somewhere. Either that, or eliminate the need to dynamically reference a table. I'm sure with enough whiteboard space you'll be able to figure it out!
• Avoid using the default $key and $value. Give some more descriptive variable names.
• What is checkColumn()?

Alright! One file down, now the next?

• Database is a very vague name. You got the noun part down, but seeing something like this would kill me!
• Something like $connection doesn't need to be public. I'd make that private/protected and encapsulate it.
• Add a visibility to your constructor.
• Your constructor has too much business logic in it! It should be for constructing the object, nothing more. Break the logic into other methods.
• Oh goodness! I do not like how you're setting up the database via the session! That is asking for a lot of trouble! Why can't the credentials be hardcoded? Or maybe put in a config file and access that way? The user has your database credentials!
• Your DBAL shouldn't have any type of business logic in it what-so-ever. It defeats the point of having it in a separate class.

And lastly, you mention your code bottlenecking, but you're not sure why. Profile your code, get some concrete numbers, and then work from there. Is it that foreach that's causing the lag, or it say the query? You'll never know unless you test it!

In the comments a couple questions were asked. I'll do my best to answer them:

functions.php is a file which contains database class and functions that are used by more than one class; shouldn't I use such resolution?

It's vague. What kind of functions are in this file? What kind of functions should go in this file? When is it appropriate to have a new class, versus just add functions into here. It's a problem of extending functional programming along side OO programming.

Making a Record class would mean that it would have methods like updateClient(), insertClient() etc. inside, which I thought would make connecting app to database harder - wouldn't it be simpler for an app just to invoke desired class which is in separate .php file?

Those methods you list might actually be a better fit in a Client class. But having CRUD operations in a single class is not a bad thing. It keeps all "record" things together, and all "client" things together.

Why adding class name in __construct parameters implements better dependency injection?

It ensures that the type hinted class is being introduced into the object. If I say __construct(Database $db), I know that a database will be passed. It also helps inheritance efforts. I could code something like __construct(API $api), and then if I have UserAPI and ServiceAPI classes, both of which extend API, then I can pass either of those, too.

in update class, I check provided table and column with [the $_SESSION global] white-lists, why is it bad?

You don't have control over the session. It's so easy to take advantage of vulnerable sessions, it just takes the right mind. Here's just the a beginner's answer to session hijacking/fixation.

I want an user of app to be the user of the database either (docs.oracle.com/cd/B28359_01/network.111/b28531/…), that's why I've come up with creating connection by $_SESSION.

From the Oracle link you provided, right off the bat I'll give you text from that page:

You can store password credentials for connecting to a database by using a client-side Oracle wallet. An Oracle wallet is a secure software container that stores the authentication and signing credentials needed for a user to log in.

I point this out because of several reason:

1. Oracle isn't advocating PHP sessions. They want you to use their secure software to authenticate. These are two different realms here!
2. Application users should really only need to be database users if you have that much data. I don't see any reason to do this if you are the average web developer. If you are scaling up to this approach, you will want a team. Things will become very messy if each user get's their own set of databases!

How can I connect to database by mysqli using password hashes?

You can't. Not hashes anyways. Know the differences between hashing, encrypting, etc.

You may need to rethink your program architecture before getting into this. I don't have the experience to tell you how it should be done.

Answer 2

UpdateRecord doesn't make much sense as a class. As a rule, classes should be named as nouns. A verb name suggests that it would be more appropriate as a method, either in the Database class or a subclass of Database.

Answer 3

Multiple updates

I think overall your code looks good, though someone who is better with PHP would be better placed to review that aspect of it. I saw part of your question was how to update multiple fields at the same time. You can create a stored PROCEDURE in MySQL and pass parameters for multiple updates at the same time. If you write it correctly you can use IF...THEN statements to tell MySQL to skip NULL input parameters.

I would also suggest to make the updates as part of a TRANSACTION so your database is in a consistent state, in other words all the updates will be committed at the same time while the affected records are locked during the transaction. Here goes:

DELIMITER $$ DROP PROCEDURE IF EXISTS update_data $$ CREATE PROCEDURE update_data ( -- first update params IN column1 TEXT, IN value1 TEXT, IN id_value1 TEXT, -- second update params IN column2 TEXT, IN value2 TEXT, IN id_value2 TEXT -- repeat as needed ) BEGIN START TRANSACTION; -- first update IF table1 IS NOT NULL THEN UPDATE polisy_oc SET column1 = value1 WHERE numer_polisy_oc = id_value1; END IF; -- second update IF table2 IS NOT NULL THEN UPDATE polisy_oc SET column2 = value2 WHERE numer_polisy_oc = id_value2; END IF; -- repeat as needed COMMIT; END $$ DELIMITER ; 

Once the procedure is created, the SQL syntax to call it is:

CALL PROCEDURE(column1, value1, id_value1, column2, value2, id_value2) -- etc. 

So in PHP it would look something like this for one single update. You would need to mofidy your PHP script of course to accomodate more input parameters, but you look like your know your PHP stuff so I'm sure you can figure that part out.

$update = $this->db_connection->prepare("CALL update_data($key, ?, ?");