0
\$\begingroup\$

I have an abstract Connection class which looks like this:

abstract class Connection
{

    /**
     * @var mixed $instance Represents the instance of the connection
     */
    protected $instance = null;

    /**
     * @return mixed
     */
    public function getInstance() {
        if (!$this->isInitialized()) {
            echo "Cant get instance yet because the connection isn't open";
        }
        return $this->instance;
    }

    /**
     * @return bool
     */
    public function isInitialized() {
        return $this->instance !== null;
    }

    /**
     * Initializes the connection
     */
    public abstract function initialize();

    /**
     * Closes the connection
     */
    public abstract function terminate();
}

It requires an intialize() and terminate()in its childs or subchilds.

  • The initialize method opens the connection
  • The terminate method closes the connection

The $instance determinates the object which has to be ran to start the Connection. For example, for a MySQL connection, this could be the mysqli object with its parameters.


A child class of the abstract Connection class looks like this (in this case im only showing a Connection to a database).

This class cannot be instantiated because its also abstract.

abstract class DatabaseConnection extends Connection
{

    /**
     * @var string
     */
    protected $host;

    /**
     * @var
     */
    protected $database;

    /**
     * @var
     */
    protected $username;

    /**
     * @var
     */
    protected $password;

    /**
     * @var
     */
    protected $credentials = [];

    /**
     * @param float  $host
     * @param string $database
     * @param string $username
     * @param string $password
     * @param array  $credentials
     */
    public function __construct($host, $database, $username, $password, $credentials = []) {
        $this->host        = $host;
        $this->database    = $database;
        $this->username    = $username;
        $this->password    = $password;
        $this->credentials = $credentials;
    }

    public final function initialize() {
        if ($this->isInitialized()) {
            throw new Exception("Database connection isnt open yet.");
        }
        $this->instance = $this->getConnectionObject();
    }

    public final function terminate() {
        if ($this->isInitialized()) {
            throw new Exception("Database is closed");
        }
        $this->instance = null;
    }

    protected abstract function getConnectionObject();
}

This mostly speaks for itself, you initialize the credentials in the __construct method.

The getConnectionObject() determinates the object wich has to be instantiated to initialize a Database Connection.


A child class of the DatabaseConnection looks like this:

class MySQLConnection extends DatabaseConnection
{

    /**
     * @return mysqli
     */
    public function getConnectionObject() {
        return new mysqli($this->host, $this->username, $this->password, $this->database);
    }
}

This can be instantiated, to use this, i need to use the following code:

$connection = new MySQLConnection('127.0.0.1', 'db', 'username' 'password');

To open the connection you use the following method:

$connection->initialize();

To close the connection you use the following method:

$connection->terminate();

To give this DatabaseConnection functionality I call the getInstance() method on the $connection object i've just made.

$connection->getInstance();

In this case this returns the mysqli object, from here i can call all needed functions.


A few notes:

  • This is for practice, i was trying to create a connection class wich had support for multiple types of connections.
  • What I originally wanted was to seperate each database function in a seperate class, but i did not know how to do this.
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Your code has a major issue. It's breaking a basic rule of inheritance. The "base" class (Connection) does not specify a constructor. That is to say, any constructors that a child class may contain should take either no, or only optional arguments. That's what the Liskov principle states anyway.

Your DatabaseConnection extends Connection (BTW: an abstract extending an abstract... that smells, if you ask me), but it defines a constructor with non-optional arguments. This contradicts, and breaks the inherited contract defined by Connection.
PHP historically, has been very forgiving when it came to conflicting signatures, especially in the constructors, but as the manual says:

When inheriting from an abstract class, all methods marked abstract in the parent's class declaration must be defined by the child; additionally, these methods must be defined with the same (or a less restricted) visibility. For example, if the abstract method is defined as protected, the function implementation must be defined as either protected or public, but not private. Furthermore the signatures of the methods must match, i.e. the type hints and the number of required arguments must be the same. For example, if the child class defines an optional argument, where the abstract method's signature does not, there is no conflict in the signature. This also applies to constructors as of PHP 5.4. Before 5.4 constructor signatures could differ.

That ship has sailed. Now in case you're wondering why this is such a big deal, look at the code below:

function checkConnection(Connection $con)
{//type-hint ensures instance of Connection is passed
    //for some reason, create a copy:
    $class = get_class($con);//get actual class
    $newInstance = new $class();//<-- No arguments!!
}

Now this function has to be allowed to assume that no arguments need to be passed to a constructor, if there even is a constructor. Simply because this function is hinting at Connection instances. Because of the type-hint, the function can only assume the methods/properties that are declared by the Connection class are available, and that the instance that it is being passed is an instance (an extension) of this contract. Connection doesn't declare a constructor, so its children can only create a public, optional constructor.

Fix this issue ASAP.
Apart from that, I'd really like to see where you're going with this, because this abstract class Connection + abstract class DatabaseConnection seems like an awful lot of abstraction for what will, essentially, be a service locator, or DI container.

A DI Container, or SL shouldn't even care what kind of dependencies/services/objects it is injecting and instantiating. So there's even less need of these abstractions you've written from that perspective.

I'm also having doubts concerning the terminate method. If a piece of code uses a mysqli connection, and needs the connection to be open for some reason (ie: a prepared statement that is pending, a huge result set is being fetched in chunks,...), having the connections managed at a central level might cause conflicts. I'd say that the only place where the container can close connections is when it is being destroyed: if the container is going out of scope, all that it has spawned is probably gone, too.


Update - A crude example of a container:

Say you have config files, that define DB connections, which could look like this;

connections:
    db:
        readConnection:
            class: mysqli
            host: 127.0.0.1
            database: db
            username: user
            password: pass
        writeConnection:
            class: PDO
            host: 8.8.8.8
            database: db
            username: user
            password: pass

Now, a container would parse this config, and allow you to write something like this:

$mysqli = $container->get('connections.db.readConnection');

This get method would then parse the slug, and interpret the configuration it finds, to end up returning:

$path = explode('.', $slug);
$section = $this->getFullConfig();
foreach ($path as $key)
    $section = $section->get($key);
$class = $section->get('class');
return new $class(
    $section->get('host'),
    $section->get('database'),
    $section->get('username'),
    $section->get('password')
);

Note that this is a grossly oversimplified, but you get the idea.

\$\endgroup\$
11
  • \$\begingroup\$ I'm sorry for my late reply, but thanks for your time and answer. What do you mean by an Dependency Injection Container? I've heard of it but dont know how to use it. Can you please show some code examples? And would it be better to use an interface instead of the abstract Connection class? \$\endgroup\$
    – Bas
    Nov 21, 2014 at 10:47
  • \$\begingroup\$ @Bas: It would be better not to use a Connection abstract class, or interface. Connections in PHP are more like resources, than they are objects (By that I mean: you use a connection as-is, it doesn't have an API as such). Look at most frameworks being used today, the vast majority have containers, or injection components \$\endgroup\$ Nov 21, 2014 at 10:51
  • \$\begingroup\$ so it would be better to leave the databaseconnection abstract without extending or implementing anything? \$\endgroup\$
    – Bas
    Nov 22, 2014 at 12:22
  • \$\begingroup\$ @bas: imho, creating an abstract db connection wrapper is really rather pointless, without also writing an entire DBAL, like doctrine does. Just check my reviews of PDO abstraction/wrapper classes as to why I say this \$\endgroup\$ Nov 23, 2014 at 18:22
  • \$\begingroup\$ this is really getting to comiclicated for me at the moment.. This was my whole idea to have support for multiple connections types, in different files? \$\endgroup\$
    – Bas
    Nov 24, 2014 at 7:08
1
\$\begingroup\$

It looks reasonably well thought out to me, although I am not sure how much it would be used beyond practice.

There are a few small things that can be improved. Comments are inline

public final function terminate() {
    // ### Is this test correct, if I terminate after initialization I always get an exception?
    if ($this->isInitialized()) {
        throw new Exception("Database is closed");
    }
    $this->instance = null;
}


// ### You can typehint array here to throw a warning if an array is not supplied
public function __construct($host, $database, $username, $password, array $credentials = array()) {
    $this->host        = $host;
    $this->database    = $database;
    $this->username    = $username;
    $this->password    = $password;
    $this->credentials = $credentials;
}


// ### It would be simple to automatically initialize a connection
public function getInstance() {
    if (!$this->isInitialized()) {
        $this->initialize();
    }
    return $this->instance;
}


// ### Is this going to be overridden in practice, if not make it private
public function isInitialized() {
    return $this->instance !== null;
}


// ### If you are going to throw exceptions it might be a good idea to subclass them
// ### So you could throw a ConnectionException for example, this would make it   
// ### easier to catch Connection specific errors
public final function initialize() {
    if ($this->isInitialized()) {
        throw new Exception("Database connection isnt open yet.");
    }
    $this->instance = $this->getConnectionObject();
}
\$\endgroup\$
3
  • \$\begingroup\$ I'm sorry I took so long to answer. But thanks for your answer and time. I see that i've made a small mistake there indeed at the terminate() method, thanks for pointing that out. What do you mean by this line in at the getInstance function though? // ### It would be simple to automatically initialize a connection \$\endgroup\$
    – Bas
    Nov 19, 2014 at 7:59
  • \$\begingroup\$ The way it was coded you call getInstance, then have to call initialize(), then use the object. I was just suggesting you could call getInstance and if it wasn't initialize then initialize a connection then return it \$\endgroup\$
    – bumperbox
    Nov 19, 2014 at 8:51
  • \$\begingroup\$ Could you edit your post how you would use it then? \$\endgroup\$
    – Bas
    Nov 19, 2014 at 9:03

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.