Cron PHP script to pull newly added clients from remote db, add their information to local db

Question

I wrote a PHP script and was wondering if it was well written or if it could be improved upon to read or run more efficiently, any help or critique would be greatly appreciated.

The file will be placed in cron jobs and run every 15 minutes. The goal is that it will take new customers from a remote MySQL database and add them to my local database with entries for both customers and users.

These are the general steps:

1. Return a list of new customers from the remote database by selecting all customers who have been added since the last update.

2. For every new customer, create an entry into the users table and import the customer's data from the remote database. Also to note the user_id created (auto incremented) as later it will be used to link the user to their corresponding column in the customers table.

3. Now create an entry into the customers table and import data from the users table. For primary_user_id, reference the id saved above. Note the new incremented customer_id.

4. Update the time last checked table value to reflect the signup timestamp of the most recent customer.

And here is what I have written:

    <?php
defined('DB_SERVER') ? null : define("DB_SERVER", "localhost");
defined('DB_USER')   ? null : define("DB_USER", "username");
defined('DB_PASS')   ? null : define("DB_PASS", "password");
defined('DB_NAME')   ? null : define("DB_NAME", "database");


defined('DB_REMOTESERVER') ? null : define("DB_REMOTESERVER", "remote.server");
defined('DB_REMOTEUSER')   ? null : define("DB_REMOTEUSER", "user");
defined('DB_REMOTEPASS')   ? null : define("REMOTEPASS", "pass");
defined('DB_REMOTENAME')   ? null : define("DB_REMOTENAME", "db");

defined('DS') ? null : define('DS', DIRECTORY_SEPARATOR);
defined('SITE_ROOT') ? null : define('SITE_ROOT', DS.'var'.DS.'www'.DS.'menu');
defined('LIB_PATH') ? null : define('LIB_PATH', SITE_ROOT.DS.'includes');

$db1 = new PDO('mysql:host='.DB_SERVER.';dbname='.DB_NAME.';charset=utf8', DB_USER, DB_PASS);
$db1->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$db2 = new PDO('mysql:host='.DB_REMOTESERVER.';dbname='.DB_REMOTENAME.';charset=utf8', DB_REMOTEUSER, DB_REMOTEPASS);
$db2->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);


//Retrieve timestamp from most recent customer db update.

$smt = $db1->prepare("
    SELECT time_last_checked FROM clients_update;
 ");

$smt->execute();
$data = $smt->fetchAll(); 


//Set time_last_checked to variable last_customer_entry_time, this will be used in the next query which selects all new customers added since this timestamp.

foreach ($data as $row):
    $last_customer_entry_time = $row["time_last_checked"];
endforeach 


//Select values from customers added since last_customer_entry_time

$smt1 = $db2->prepare("
    SELECT first_name, last_name, phone, email, signup_date FROM customers where signup_date >= '".$last_customer_entry_time."';
 ");
$smt1->execute();
$data = $smt1->fetchAll(); 


foreach ($data as $row):

    //Take new customer values from remote server db (db2) and insert into the users table in db1

    $smt2 = $db1->prepare("
        INSERT INTO `database`.`users` 
            (`email`, `name`, `last_name`, `phone`, `date_created`, `date_updated`) 
        VALUES (
            '".$row["email"]."', 
            '".$row["first_name"]."', 
            '".$row["last_name"]."', 
            '".$row["phone"]."', 
            CURDATE(), 
            CURDATE()
        );
     ");
    $smt2->execute();

    //Set signup date of customer to variable newtimestamp. When the last customer is added, this value will update time_last_checked
    $newtimestamp = $row["signup_date"];

    //Set the last auto incremented id value to a variable in order to insert the new customer's user_id in the customers table
    $id = $db->lastInsertId();


    //Take values from users table and insert them into customers table

    $smt3 = $db1->prepare("
        INSERT INTO `database`.`customers` 
            (`primary_user_id`, `customer_name`, `date_created`, `date_updated`) 
        VALUES (
            '".$id."', 
            '".$row["first_name"]." ".$row["last_name"]."', 
            CURDATE(), 
            CURDATE()
        );
     ");
    $smt3->execute();


    //Update users table with new customer ID from customers table

    $customer_id = $db1->lastInsertId();
    $smt4 = $db1->prepare("
        UPDATE users SET customer_id = '".$customer_id."' ORDER BY user_id DESC LIMIT 1;
     ");
    $smt4->execute();


    //Update time last checked table value to reflect the signup timestamp of the most recent customer

    $smt5 = $db->prepare("
        UPDATE clients_update SET time_last_checked = '".$newtimestamp."';
     ");
    $smt5->execute();

endforeach

?>

Answer 1

I don't have a lot of time, So I'm just going to list a couple of things that come to mind:

• lastIndertId does not work on all database, and i could return a wrong result
• $stmt1 doesn't tell me much. Use variable names that are self-explaining. It will help you in the long run.
• If you use prepared statements, atleast use them correctly. You are using prepare, but still injecting the variables into the query directly. Instead of using addParam, ...
• On the prepared statements, only create the prepared statement once, and then execute it multiple times inside the foreach. This is where prepared statements are made for.
• don't use endofreach but use curly brackets. Always use curly brackets. It just adds for better readability.
• Don't define your configurations. Have them in an array. Much cleaner. Maybe even wrap the PDO creation in a function.
• When creating a PDO object, you can pass in attributes as 4th parameter. Do that instead of calling the setAttribute.
• use less line-endings. You go a little crazy on the line-endings. Maybe read up on the php-fig coding standards. This kind of tends to produce easy to read code.

Answer 2

Security: SQL Injection

You are using PDO:prepare, which is good. But you are still just inserting the variable data in the query instead of binding it to a variable. So you are not actually using prepared statements correctly.

All your queries should actually look like this:

$smt3 = $db1->prepare("
    INSERT INTO `database`.`customers` 
        (`primary_user_id`, `customer_name`, `date_created`, `date_updated`) 
    VALUES (
        ?, 
        ?, 
        CURDATE(), 
        CURDATE()
    );
 ");

$smt3->execute(array($id, $row["first_name"] . " " . $row["last_name"]));

It doesn't matter if first_name was checked on the other server, I would not trust it. Maybe the code there changed, and users can change their name, and the check there is forgotten. Who knows?

Using prepared statements for the id is just for uniformity. It doesn't hurt, and when there are no variables in the query at all, it is really easy to find forgotten once that really don't belong there (even automatically).

Security: Opening Database for remote access

If this script is the only reason that the remove server opens its entire database for remote access, I would just write an API that exposes this data on the remote server (with an authentication mechanism of course).

That way, you minimize damage done to the remote server in case the database credentials are retrieved either from it, or from your other server.

Or you could only open the remote server to this server (come to think of it, that's probably what you are doing). This still leaves the remote server vulnerable in case this server is compromised, but that might be acceptable for you.

Performance

As @Pinoniq said, using prepared statements correctly will mean that they are only prepared once, which will increase performance.

Additionally, you are running a query on the remote host on each loop iteration, setting the time last checked, which is probably slowing down your script considerably.

This does not seem necessary. Just insert the last data after the loop is done.

Although this seems a little off. Why is the registration date of the last user relevant? You don't sort users by registration date. So either do sort the users, just insert the current data (minus some amount as buffer), or keep track of the latest date in a variable, and then perform one update after the loop is done.

Misc

• use more spaces, eg around .
• $smt1, $smt2, ... are not very good names. I would use $stmSelectNewCustomersRemote, $stmInsertNewCustomerLocal, etc. That way, it's easier to read, and you will not accidentally mess up the numbers. Also, you can get rid of some comments.
• db and db1 aren't the worst names, but localDatabase and remoteDatabase are a lot clearer.
• your customers table seems odd. Instead of keeping duplicate data, I would just store an id that links it to the users table. But then again, it doesn't seem to hold any data that is not already in the users table.
• your first foreach loop doesn't make sense to me. It seems that it only assigns the time_last_checked from the very last entry to $last_customer_entry_time. Why then even have a loop?
• your code is pretty nicely formatted, which makes it generally very readable :)

Answer 3

A cron job? Really?

There must be some really weird reasons why you would want to use a cron job to do a task like this. It cannot be for backup reasons, since you're always many minutes behind the other database, and I cannot think of any other reasons. Perhaps you don't have a webserver running on your local machine?

I would prefer a push system. As soon as a new customer is created it is stored in the local database and pushed to the remote database. As long as there's a working connection between the two sytems both databases will be up to date.

So the only thing left to do is handle situations where there's no working connection.

1: The push system should work asynchronous, so it doesn't block the script that's running. There are quite a few multithreaded CURL libraries out there that can do this. (backgrounder: https://segment.com/blog/how-to-make-async-requests-in-php)

2: The push system should check whether the customer was indeed stored in the other database. If it wasn't the same customer should be pushed again as soon as the connection is working again. Clearly you need to buffer this information during a disconnect. There are many ways to do this.

The way forward should be a more general system. Now you're pushing customers, but in half a years time you would want to push orders, and so on. Why not do it right from the start, since all pushes are basically the same?

But wait, they've thought of that! There are ways to synchronize two databases. See, for instance:

https://www.howtoforge.com/mysql_database_replication

So, whether or not the script you wrote is efficient, entirely depends on what your real goals are. Since you've not told us, your question is very difficult to answer. All we can do is tweak your code a bit, but we cannot question your method. Well, I just did.