4

I have the following global statement calls sprinkled throughout my PHP legacy codebase:

$jobnumber = db_quick($sql);
db_query($sql);

That is, those function calls have been initialized in the global scope and they themselves call upon a previously initialized MySQL global object, i.e:

//initializes MySQL connection object in the global scope
$dblink = MySqlConnector::getMySql();

function db_query($sql)
{
    //previously initialized MySQL connection object
    global $dblink;  

    //native MySQL API call:
    return mysqli_query($dblink, $sql);
}

Question

Are there any benefits to refactoring these global statements, other than just to say "I have now removed them from the global scope"?

The spirit of my question goes ... those are already in the codebase, and they work. Must I mess with them? So far [3 years working with the code] there has been no apparent harm. Even with transition from deprecated mysql to mysqli, it was a painless transition. Will keeping these as global cause any measurable harm later? Are these good enough to be left as is?

What if I refactor?

By the way if I refactor I am thinking of using a Singleton pattern for database link instantiation and then calling it via an instantiated class, that calls the Singleton. Like so:

//in a calling class:
$repository = new GenericRepository();
$repository->getMySql()->persist($sql);
$value = $repository->getMySql()->paramQuery($sql, $param);

//GenericRepository makes a static call to the actual connector
class GenericRepository 
{
    final public function getMySql()
    {
        if ($this->link === null)
            $this->link = MySqlConnector::getMySql();

            return $this->link;
    }
}

//Connector calls MySQL API
class MySqlConnector
{

    public static function getMySql()
    {
        if (null === static::$instance)
        {
            include 'include/config.php';

            //class MySql extends mysqli library
            static::$instance = new MySql(DBHOST, DBUSER, DBPASS);
        }

        return static::$instance;
    }
}

So in essence, by refactoring, I will be removing the globals database methods and replacing them with instantiated calls to the Singleton pattern. The code change will be fairly extensive, as there are a couple of thousand of global database calls.

Effects of the above Refactoring

Currently I can call the methods "from anywhere" - they are global.

db_query($sql);    

After refactoring I will still be able to call them "from anywhere", but using a longer connection string.

(new \Namespace\GenericRepository())->getMySql()->persist($sql);

That sounds a bit like a detriment - longer string, more to type, more to copy & paste. Are there really some benefits to this?

2

There's a reason why db_query is in the global scope, and that is because you need it to be visible everywhere so that you can call it from anywhere. When you use jQuery,$ is in the global scope, and that's why.

Before you go through a complex refactoring just to remove a function from the global scope, consider the reasons why global scope is problematic. I think you'll find that it's not global scope that is bad per se, but rather global variables.

Also, note that global variables aren't always bad. If you're using one so that you don't have to repeat a connection string endlessly throughout your program, that's a good thing.

  • 1
    The OP actually uses a global variable, $dblink. – Doc Brown Dec 28 '16 at 8:46
2

Refactoring is not an end in itself, it is a means to an end. And the goal is to make code more and testable, maintainable, and evolvable.

In your case, using the same global function $db_querywhich accesses the same global $dblink object during the whole codebase prevents you from easily using this function with two different database connections during one execution run. The restriction "one and only one db connection per application instance" is manifested into the architecture. Any code which is build upon your $dbquery will have to live with this assumption, and it cannot be easily reused in a different context where the db environment will look differently, or put into a testing context with no database at all.

This can be perfectly ok. For many applications, the "one db connection at a time" architecture is all they need during their whole life span. If your application is of this type, and you do not notice any real drawbacks from this restriction, not even for testing, then leave it as it is.

However, if this architecture starts getting in your way for implementing new features or tests, then a kind of refactoring you need is a refactoring where the $dblink will be provided in a context-dependend manner, or the database behind $dbquery can be easily mocked out. That shows that why your Singleton refactoring is probably the wrong approach, because it does not solve the core restrictions of your architecture.

  • well what I have now is MySQL (internal) and MsSQL databases (external). MySQL one is called $dblink/db_query(), the other is currently $pdo/get_pdo(). So at times there are two global database objects that are not interchangeable with one another both in the global scope. Singleton approach does remove the method and the global variable from global scope and moves it into a namespace. – Dennis Dec 28 '16 at 16:22
  • The connection is still instantiated and kept for the lifetime of the HTTP request, but it can no longer be called from the global scope. So Singleton pattern does do something. Behind the scenes the Singleton is alive for the duration of the application, but code-wise it looks like it can be called via standard OO routines and initialized and look like a local variable (while the singleton keeps global behind the scenes). I am not sure what other ways are available in HTTP architecture aside from Singleton to solve this issue. – Dennis Dec 28 '16 at 16:22
  • That said ... it almost looks to me like it is a matter of preference. Do I want to see a global function call in my code, or a namespaced class that is instantiated, with various methods to use for DB access. I don't know ... both do the job, both are the same in essence (keep global state), it is the semantics that is different (I'm talking about my global state vs using Singleton). I am not aware of other ways... Database engine runs 24/7 so it's all about how I connect to it and how I use it. I can do an init with every request (probably not good) or init once and keep it in global state – Dennis Dec 28 '16 at 16:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.