[Slice]

So I'm trying to make a class to handle my database connection, while trying to focus on making it very portable for future projects. At the moment this is giving me an internal server error:

<?php

class connectDB {
	
	private $db_user,
			$db_pass,
			$db,
			$error,
			$dsn;
			
	public $pdo;
			
	public function __construct($db_user, $db_pass, $db)
	{
		$this->db_user = $db_user;
		$this->db_pass = $db_pass;
		$this->db = $db;
		$this->dsn = "mysql:host=localhost;dbname=" . $this->db;
	}
	
	public function connect()
	{	
		$user = $this->db_user; // This seemed to 
		$pass = $this->db_pass; // fix an undefined 
		$dsn = $this->dsn;	// error. Not sure why ..
		
		try
		{
			//$this->pdo = new PDO($this->dsn, $this->user, $this->pass); - undefined
			$this->pdo = new PDO($dsn, $user, $pass); // gives no error
		}
		catch (PDOException $e)
		{
			$this->error = "Connection Failed: " . $e->getMessage();
		}
	
	}
	
	public function getError(){ return $this->error; }
	
	
}


class session {

	private $email,
			$userid,
			$password,
			$firstname,
			$surname,
			$query_login,
			$query_register,
			$reg_result,
			$log_result,
			$db_user,
			$db_pass,
			$db;
			
	public function __construct($db_user, $db_pass, $db)
	{
		$this->db_user = $db_user;
		$this->db_pass = $db_pass;
		$this->db = $db;
	}
									
	public function register($email, $password, $firstname, $surname, $query)
	{
		$this->email = $email;
		$this->password = $password;
		$this->firstname = $firstname;
		$this->surname = $surname;
		$this->query_register = $query;
		
		$array = array(
			'email' => $this->email,
			'password' => hash('sha256', "mysalt" . $this->password),
			'firstname' => $this->firstname,
			'surname' => $this->surname
			);
			
		$this->connection = new connectDB($this->db_user, $this->db_pass, $this->db);
		
		$this->connection->connect();
						
		$ps = $this->connection->pdo->prepare($this->query_register); //not too sure if this is valid
			
		$this->reg_result = $ps->execute($array);
	}
	
	public function getRegResult()
	{
		return $this->reg_result;
	}
	
	
}

?>

and index.php has:

<?php
require_once("require/session.php");

error_reporting(E_ALL);

$session = new session("user","pass","database"); //database credentials

$query = "INSERT INTO users (email, password, firstname, surname) VALUES ( :email, :password, :firstname, :surname )";

$session->register("me@email.com", "password", "John", "Smith", $query);

$result = $session->getRegResult();

if($result)
{
	echo "Registration was succesful!";
}
else
{
	echo $session->getError();
}


?>

I think I've really gone off the rails here somewhere.. I'm not getting any exception errors, or any php errors in my IDE.

Sorry for the amount of code, but I have no idea where the server error is coming from. Any and all help is much appreciated.

[CTphpnwb]

I wouldn't name a class session. That may not be the source of your trouble, but it is confusing, especially if you're using $_SESSION variables.

[Slice]

That's a good point, hadn't thought of it like that. I'll change it to something a little less familiar

EDIT:problem solved.

I've been working on this while I've been out on my laptop (running XAMPP on windows 7 locally with no internet connection). I copied the little I had over to my server when I get home and carried on from there. Files had some weird permissions which wouldn't let me access them. Figured it out from this apache error log:

Quote

[Sun Jul 15 17:47:58 2012] [error] [client 127.0.0.1] PHP Fatal error: Unknown: Failed opening required '/var/www/site/index.php' (include_path='.:/usr/share/php:/usr/share/pear') in Unknown on line 0

Silly XAMPP.

This post has been edited by Slice: 15 July 2012 - 10:09 AM

[Dormilich]

If you’d ask me, drop the connectDB class. just instantiate PDO at the top of the script and pass it to any object that requires the connection. whether you pass the DB credentials to the connectDB object or instantiate PDO makes no difference with your code.

second, Exceptions are not errors. they work differently. you catch Exceptions where you can handle the problem. i.e. if the connection fails and you immediately catch the exception, the script would proceed execution and fail again (because there is no connection). if you enclose all your DB code in a try…catch block, a failed connection will skip all DB code and thus not produce any more errors.

[Atli]

I would also suggest changing the name of the connectDB, if you intend on keeping it, to something else. That name sounds like a function name, in that it specifies an action, like: doSometing(), whereas class names should be more like actual names, like: SomeObject. In your case, I would consider something more like: DatabaseConnection, or perhaps DBConnect. (I prefer the longer version myself, just for clarity.) - It's also a good tradition to always use Pascal case (CamelCase starting with an upper-case letter) when naming classes. That sets it a part from the functions.


And, like Dormilich says, that class isn't altogether that useful in it's current state. It would actually be simpler to just use PDO in your session class. - I'd recommend using a Singleton class instead to handle the database connection. It's one step up from Dormilich's idea of creating the connection in the global scope and passing it into all functions/objects that need it, in that the functions/objects can simply fetch the connection themselves.

[AyaneFukazawa]

I'm sure you are aware of it, but as you mentioned you wanted portability, you should not hardcode the host to localhost. Often enough you may need to connect to external databases.

Also, like Atli said, you should reconsider your naming convention for clarity.

[Slice]

Dormilich, on 15 July 2012 - 11:55 AM, said:

If you’d ask me, drop the connectDB class. just instantiate PDO at the top of the script and pass it to any object that requires the connection. whether you pass the DB credentials to the connectDB object or instantiate PDO makes no difference with your code.

I did this last time but only got it to work with a global inside my class. How do I use it without globals?

[Dormilich]

well, you pass it as parameter:

class Test
{
    private $pdo;

    public function __construct(PDO $pdo)
    {
        $this->pdo = $pdo;
    }

    // ...
}

try
{
    $pdo  = new PDO($dsn, $login, $pass, $options);
    $test = new Test($pdo);
    $test->doSomethingWithPdo();
}
catch (PDOException $e)
{
    echo "DB Error";
    send_error_mail($e->getMessage());
}

[Slice]

Would PDO not then need to be initialized every time we start a new object?

[Dormilich]

depends on where (in what scope) you start a new object. if it’s always in the same (e.g. the global scope), it doesn’t matter, if it’s different scopes, use a service provider (e.g. the Singleton Atli mentioned).

[Slice]

I've made some changes to what I first had.

So in my class SessionControl (changed name for clarity), I have a constructor that makes a new connection.

	public function __construct($db_user, $db_pass, $db)
	{
		$this->db_user = $db_user;
		$this->db_pass = $db_pass;
		$this->db = $db;
		$this->connection = new DatabaseConnection($this->db_user, $this->db_pass, $this->db);
		$this->connection->connect();		
	}

and my DatabaseConnection class looks like:

class DatabaseConnection {
	
	private $db_user,
			$db_pass,
			$db,
			$dsn;
			
	public $pdo;
			
	public function __construct($db_user, $db_pass, $db)
	{
		$this->db_user = $db_user;
		$this->db_pass = $db_pass;
		$this->db = $db;
		$this->dsn = "mysql:host=localhost;dbname=" . $this->db;
	}
	
	public function connect()
	{	
		$user = $this->db_user; // This seemed to 
		$pass = $this->db_pass; // fix an undefined 
		$dsn = $this->dsn;		// error. Not sure why ..
		
		try
		{
			//$this->pdo = new PDO($this->dsn, $this->user, $this->pass); - undefined
			$this->pdo = new PDO($dsn, $user, $pass); // no undefined error
		}
		catch (PDOException $e)
		{
			echo "Connection Failed: " . $e->getMessage();
		}
	
	}
		
}	

This way the class SessionControl has database access via $this->connection , and in any page I need to use my SessionContol class, all I have to do is:

<?php
$SesControl = new SessionControl($user, $pass, $db); 
$SesControl->checkLogin(); //or register/login
?>

Is this method not easier? I know it's a lot of effort to have the class for connecting but it means only one line of code on any page wanting to use the ControlSession. Are there any security disadvantages here or is it mainly preference / bad practice?

[Dormilich]

Slice, on 16 July 2012 - 02:53 PM, said:

Is this method not easier?

not at all. your SessionControl class would have to know the DB credentials, which is not its responsibility. the database credentials are the sole responsibility of the DatabaseConnection class. and in the end it’s even more complicated than instantiating PDO by hand (which would need only the login, password and database name as variables as well).

it were sensible, if the DatabaseConnection class would get the DB credentials from a configuration file.

[calebjonasson]

Along with the previous recommendation of keeping connection information in a config file. You should:

• Keep the config file outside of the hosting directory
  
• Make your connection class extend PDO or Mysqli
  
• I like to use a singleton wrapper to recycle connections and information. It's not a bad idea and will help you log and monitor errors internally to the application.