Variable organization issue

I have a PHP script that has many function declarations in it…

All these functions have in common the fact that they have an error checking mechanism.This uses a mail() function to send an e-mail(that notifies the admin in case of errors).

These functions use as an argument the instantiation of PHPmailer passed in a variable:

$phpmailer = new PHPMailer;

Here is an example of function body:

 function delete_front_app($connection,$apID,$email)
            {
             ...
             ...
             mail_error($email,$phpmailer);return false;     
            . ..                     
            }

And here is my problem:
As you see $phpmailer must be passed to mail_error().
There are two ways this can be done…either declaring $PHP_mailer as global variable within each function either passing it as an argument(in this example in delete_front_app).

The drawbacks of using globals are known…on the other hand if I choose to pass it as an argument this will require me to do it in every function call…there are many functions spread out in many scripts…as such it is a tedious process.

So the question is what is the more convenient way you think to have available $PHPmailer to mail_error?
What do you propose?

What kind of errors are we talking about? Are those severe application errors that cause the script to halt or lighter errors (like when a user did something he was not supposed to) that don’t interrupt the execution flow of your scripts?

If it is the former then exceptions come to mind as a better alternative. Hopefully, you have an entry script to all your code - like a single index.php instead of multiple php files - then in index.php you simply wrap the whole stuff in a try-catch block and you have to instantiate the mailer only in the catch block. Of course, you will have to change mail_error() occurrences to throw occurrences everywhere but I think it’s worth it. You can also use set_error_handler() to catch all normal PHP errors automatically and have them mailed too.

If your application does not go through a central entry script then you can use set_exception_handler() in a file that is included everywhere to catch your exceptions without a try-catch block - otherwise you’d have to use the same try-catch in every php file.

Another idea comes to my mind - for most reliable notifications about errors I would not mail them every time an error occurs because when an error is critical enough then even the mail function may stop working. And if there happens to be a large number of errors in a short period then you may end up with thousands of emails and it can become problematic, too. I would simply log those errors in a file and have a cron script that will read it every x minutes and mail all of them in a single message.

we are talking here about SQL query errors(prepared statements)…no I do not use a single entry point for my app.

So having said the above…can you “narrow” your answer.
everything you suggested was OK…either way.

Why not. Is this something you built custom or that you inherited?

To be completely honest both of those solutions seem messy. With the way the architecture has been built both solutions are equally as crappy. Though gun to my head I would use a function that instantiates, configures, and saves it to a internal static variable. When the mailer exists the function would just return that instance.

function what_a_shittty_thing_i_have_to_do() {
  static $mailer = NULL;

  if($mailer === NULL) {
    new = new PHPMailer();
    // configure mailer and whatnot or delegate to another function handle configuring the mailer.
  }  

  return $mailer;
}

I just don’t understand why anyone wouldn’t build an app using dependency injection and most preferably a well known modern framework which would make this all a breeze… rather than a sloppy procedural mess. Though giving you the benefit of the doubt I’ll assume this is a mess you inherited.

For example, in Laravel you could just register a binding around the mailer and have it automatically injected into any class that requires it as a dependency. All internal to the inner workings of the dependency injection container.

but… everyone wants to keep things “simple”…

First of all, a bit of irrelevant question. Is the site in question going to be public or with a limited access?
I am asking because in the former case you are under the risk of getting your account heavily flooded with error messages in case of some technical failure.

we are talking here about SQL query errors(prepared statements)

Well, I consider myself a guru in error reporting in general, and - especially - in handling database and prepared statements errors in particular. And so I have a solution for you that will make your problem obsolete.

Any prepared statement aware PHP extension is able to throw exceptions in case of database errors. And a very cool thing about exceptions is that you don’t have to handle them right in place! Instead, you can set up a single exception handler, write your handling code once, and it will work for every exception that will be thrown by your prepared statement. Which will make this problem with sending the email handler all around just a non-existent one.

Of course, the same approach could be used for all the kinds of errors. Do you like the idea?

1 Like

Then another question - do you use a single entry point to the database? By that I mean a single class or a collection of your functions? If so, then it’s easy to add error reporting ability to the database class in a single place. If not and you are using native functions directly like mysqli_query() or error() then you can’t benefit from this solution and you have to hack around with a mail_error() function all over the place.

You see, a single entry point is a good and very useful thing :). If you are prepared to change/improve your code in so many places by adjusting your mail_error() function then you may as well switch to a single database class, put all the error handling in there and have an improved architecture even in procedural code. Of course, you’ll then have to pass the database object to your functions.

Or you could use exceptions like I described in my previous post, catch the database exception in a single file included in every script and in your functions you can do this:

mysqli_query($sql) or throw new DatabaseException();

Yeah, it occurred to me a while later that in this kind of architecture this might not be worth the trouble implementing best dependency injection patterns. DI can be used in procedural programming but it’s not as flexible and doesn’t give as much benefits as in OOP. With this in mind I would go with the simplest solution and do something like this (if I really didn’t want to do it with exceptions for some reasons):

function mail_error($errorMsg) {
  $mailer = new PHPMailer();
  // send stuff
  // ...
}

I would advice against storing the mailer instance in a static variable for later use. I have done it in the past and sometimes have run into hard to debug problems where the messages were not sent properly as the mailer instance remembered some configuration from a previously sent mail. It’s much safer to make a new instance for each mail - and doing it is not so resource intensive, anyway.

The question to the OP is this: what are you trying to solve? If you want to implement full dependency injection then passing the mailer is not the only issue. One of the benefits of DI is to make your dependencies obvious when you look at the function or method definition. Even if you pass the mailer to your functions you still have a hidden dependency, which is the mail_error() function - it is like a global variable used in your functions. So if you want to do it ‘properly’ then you’d also have to pass a MailError instance to your functions. Now, this MailError class could take care of instantiating PHPMailer and sending the message.

But in your case I would choose these solutions:

  1. Switch to a central database class and use it everywhere. An alternative can be pure PDO, because PDO can be set up to throw exceptions.
  2. Use exceptions to catch the database errors and send emails.

In fact you cannot write it like this because an object cannot be cast to bool.

The good news, you don’t have to write this code at all, as mysqli is able to throw exceptions on its own, just like PDO. All you need is just set the right error reporting mode before connect:

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);

and you will need not a single line in the application code to report mysqli errors. Just write

mysqli_query($conn, $sql);
$mysqli->prepare($sql);
$mysqli->whatever();

and errors will be reported automatically

Correct, I got mislead by the popular shorthand usage mysqli_query() or die(); but that works only because die() can be cast to bool in the or comparison. A regular if would have to be used then.

Indeed, I didn’t know mysqli can throw exceptions! In this case the problem is easily solved - unless the OP uses the old mysql extension :scream:

first of all…I am considering abandoning the idea of sending e-mails cause there is the danger of account flooding as stated above.
So the only thing left do is this(example function follows):

function delete_front_app($connection,$apID,$email)
                {
                $connection->set_charset("utf8");
                if($stmt = $connection->prepare('UPDATE appointments
                                               SET delete_front=true 
                                               WHERE appointments.apID=?'))
                {
                 $stmt->bind_param('s',$apID);    
                 $stmt->execute(); 
                   $stmt->store_result();
                if($stmt->errno!==0)
                {
                error_log("error message for delete_front_app()-execution failed:".$stmt->error.",time of error:".date('d F Y h i')." \n", 3,"error_log.log");
          
                } 
                }
                else
                {
                 error_log("error message for delete_front_app()-prepare failed:".$connection->error.",time of error:".date('d F Y h i')." \n", 3,"error_log.log");
            
                }
    
          return true;      
                
          }

As you see I just write the error details to the log.What do you think?
Do you think I must add something else to the above?

Well, to my taste you should not add but rather take out most of the code:

  • charset definition should be moved to the connection
  • error logging I would move into error handler.
  • store_result doesn’t belong here at all

So I would write this code like this:

function delete_front_app($connection,$apID)
{
    $sql = 'UPDATE appointments SET delete_front=true WHERE apID=?';
    $stmt = $connection->prepare($sql);
    $stmt->bind_param('s',$apID);
    $stmt->execute();
}

I totally agree about store_result…I missed it.
This is the function I use for connection…where exactly the charset definition should be placed:
function db_connect()
{
$dbconnection = new mysqli(‘localhost’, ‘webapp’, 'passwordhidden, ‘appointments’);
if (!$dbconnection) {
throw new Exception(‘Cannot connect with the Dbase.’);
return false;
} else {
return $dbconnection;
}
}

Here is your function with mysqli configured to throw exceptions

function db_connect()
{
    mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
    $dbconnection = new mysqli('localhost', 'webapp', 'passwordhidden', 'appointments');
    $dbconnection->set_charset("utf8");
    return $dbconnection;
}

To have your error logged you have to add the following lines into your site boodstrap/config file:

error_reporting(E_ALL);
ini_set('display_errors', 0);
ini_set('log_errors', 1); 

and you will find all errors in the web server’s error_log

regarding db_connect() I think you made a mistake.
it is

$dbconnection->set_charset("utf8");

no

$connection->set_charset("utf8");

I GOT THE REST

Yes, of course. Thanks for the correction.

You know, SitePoint forums is such a relief after Stack Overflow. There, users usually are only able to report helplessly that the code provided doesn’t work. They don’t even try to understand what does it do.

By the way, one more cool thing about exceptions. Just try a deliberate syntax error in your function and check the error message. You will find that you don’t even have to add a stuff like “error message for delete_front_app()” - as it will be already there!

1 Like

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.