ZoneCMS: PDO / SECURE / MODERN [PlusEMU - FULL SUPPORT]

[Zaka]

Moved the logo as someone suggested earlier and added the "users online" thingy. I still would like you guys to help me out with the colors of the boxes (marked in red). I want to make this as good as possible.

Spoiler

 

[Hender]

The layout is good, the theme and styling does need work but its getting there.
Some colour changes, font changes and a little CSS should sort it out,
along with useful widgets.

One peice of advice dont go overboard with widgets or it will be BrainCMS all over again (way too much going on).

Other than the theme im glad to see more people coding new systems in modern code.

 

[cain]

I like the housekeeping, but I don't like the simplicity of the CMS, looks flat and a bit tacky..

 

[Zaka]

I like the housekeeping, but I don't like the simplicity of the CMS, looks flat and a bit tacky..

Well then why don't you suggest some improvements? I'm open for all feedback, constructive criticism.

 

[BIOS]

public function __construct(array $mysql) {
       try {
           $this->pdo = new \PDO(
               'mysql:dbname='.$mysql['database'].';host='.$mysql['hostname'].';charset='.$mysql['charset'],
               $mysql['username'],
               $mysql['password']
           );
           $this->pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);
           $this->pdo->exec("SET `time_zone` = '{$this->timeZone()}';");
       } catch(PDOException $e) {
           die($e->getMessage());
       }
   }

   public function __destruct() {
       // Destruct the connection to DB once finished
       try {
           $this->pdo = null;
       } catch(PDOException $e) {
           die($e->getMessage());
       }
   }

This is what I use to construct and destruct the connection.

Same.

Unparameterized queries. You do also realize that it's probably not the best idea to display the getMessage as it can leak information, this is even stated on the PHP manuals.

Well then why don't you suggest some improvements? I'm open for all feedback, constructive criticism.

You're probably best looking up best practices on areas you need to improve rather than listening to a lot of people on here.

Here's an example I did quite a while ago

 

[Zaka]

Same.

Unparameterized queries. You do also realize that it's probably not the best idea to display the getMessage as it can leak information, this is even stated on the PHP manuals.


You're probably best looking up best practices on areas you need to improve rather than listening to a lot of people on here.

Here's an example I did quite a while ago

I know you should not put out messages like that, but I also clearly stated earlier that I wasn't done with the code, I will fix it when I'm "done" with the rest of the cms, for now it's good to get the info.
However thanks for your example!

 

[JMS]

First inspection, it looks great - as the majority have said above, it could do with some sort of customisation, however thats really minor.
I suggest a dynamic theme editor? Allowing users to personalise the colours to their own liking?

 

[Zaka]

First inspection, it looks great - as the majority have said above, it could do with some sort of customisation, however thats really minor.
I suggest a dynamic theme editor? Allowing users to personalise the colours to their own liking?

Yeah, I could let them all like change colors and such on some elements directly on the divs for more personal touch, also they could have the ability to add their own background and so, good idéa actually, might do this!

 

[BIOS]

I know you should not put out messages like that, but I also clearly stated earlier that I wasn't done with the code, I will fix it when I'm "done" with the rest of the cms, for now it's good to get the info.
However thanks for your example!

That wasn't towards you, I was quoting the example that Sentinel gave you.

As you're still learning I just didn't want you to take his advice as the snippet he provided is worse than your current in terms of security, although you should still avoid displaying the error to users in your execute() function etc.

Good luck.

 

[MayoMayn]

That wasn't towards you, I was quoting the example that Sentinel gave you.

As you're still learning I just didn't want you to take his advice as the snippet he provided is worse than your current in terms of security, although you should still avoid displaying the error to users in your execute() function etc.

Good luck.

Actually you're plain wrong. Showing the errors really got nothing to do with security, if you already know at first, you've entered the correct details. It actually DONT try for the connection only, but also for statements etc, which yes, would be better to put into a log or something, but if you have NGINX like I do, an 500 error appears and the error gets logged instead of showing the credentials.

 

[Zaka]

That wasn't towards you, I was quoting the example that Sentinel gave you.

As you're still learning I just didn't want you to take his advice as the snippet he provided is worse than your current in terms of security, although you should still avoid displaying the error to users in your execute() function etc.

Good luck.

I appreciate all input, we are all still learning. We never finish learning and improving.

 

[BIOS]

Actually you're plain wrong. Showing the errors really got nothing to do with security, if you already know at first, you've entered the correct details. It actually DONT try for the connection only, but also for statements etc, which yes, would be better to put into a log or something, but if you have NGINX like I do, an 500 error appears and the error gets logged instead of showing the credentials.

I'm not wrong, it's the developer's role to handle the error correctly by catching it and then usually log it somewhere. Yes it does have something to do with security, it's effectively known as information leakage which was on the top 10 a few years back.

"if you already know at first, you've entered the correct details. It actually DONT try for the connection only, but also for statements etc, which yes, would be better to put into a log or something, but if you have NGINX like I do, an 500 error appears and the error gets logged instead of showing the credentials"

You're forgetting this is in the development section therefore at some point it may be released and used by other people, they're not all going to have the same details; therefore there's an high chance that the connection will fail and expose the user/host and so on. Not all hotel users will run NGINX nor will they have the same configuration as you, most use IIS if I'm correct in saying so, therefore it will still be exposed.

 

[Zaka]

I'm not wrong, it's the developer's role to handle the error correctly by catching it and then usually log it somewhere. Yes it does have something to do with security, it's effectively known as information leakage which was on the top 10 a few years back.

"if you already know at first, you've entered the correct details. It actually DONT try for the connection only, but also for statements etc, which yes, would be better to put into a log or something, but if you have NGINX like I do, an 500 error appears and the error gets logged instead of showing the credentials"

You're forgetting this is in the development section therefore at some point it may be released and used by other people, they're not all going to have the same details; therefore there's an high chance that the connection will fail and expose the user/host and so on. Not all hotel users will run NGINX nor will they have the same configuration as you, most use IIS if I'm correct in saying so, therefore it will still be exposed.

Guys please stay on topic, this is a development thread for ZoneCMS. And yes you are right others will have other information, and I don't leave anything for chance. I will re-code the db class a bit and implement try and catch with the error messages not showing up in public, but logged instead.
 
I have made some progress, and updated some of the code. Thanks to @Sentinel for examples of improvements regarding my database class. The credentials are now read from a config.ini file which allows me to, later, create a install file for the cms.

Spoiler: Images

Database class

config.ini

I know that I have not fixed the "security issue" with messages going straight out, but I will fix that when time comes. Also worth to notice was that when I tried to run __destruct() inside my database class I would just get a 500 Internal Server Error code since it seems that when database class is destroyed I can't use it in my subclasses which extends the database class. So I ended up creating a destruct() function in the database class which I then run in the other classes like this:

public function __destruct() {
    parent::destruct();
}

I appreciate all the feedback I've been given, all the tips and tricks to improve my code! Thanks all of you, and hopefully I will soon be able to release this to everyone.

 

[MayoMayn]

Guys please stay on topic, this is a development thread for ZoneCMS. And yes you are right others will have other information, and I don't leave anything for chance. I will re-code the db class a bit and implement try and catch with the error messages not showing up in public, but logged instead.
 
I have made some progress, and updated some of the code. Thanks to @Sentinel for examples of improvements regarding my database class. The credentials are now read from a config.ini file which allows me to, later, create a install file for the cms.

Spoiler: Images

Database class

config.ini

I know that I have not fixed the "security issue" with messages going straight out, but I will fix that when time comes. Also worth to notice was that when I tried to run __destruct() inside my database class I would just get a 500 Internal Server Error code since it seems that when database class is destroyed I can't use it in my subclasses which extends the database class. So I ended up creating a destruct() function in the database class which I then run in the other classes like this:

public function __destruct() {
    parent::destruct();
}

I appreciate all the feedback I've been given, all the tips and tricks to improve my code! Thanks all of you, and hopefully I will soon be able to release this to everyone.

You gotta use __destruct() and not just destruct() in your database class, otherwise it sees is as a class function . It seems to be working fine for me. Did you try to destruct the self::$pdoStmt then thats why, because it can only be capable of destroying self::$pdo since the self::$pdoStmt is still used later on.

 

[Zaka]

You gotta use __destruct() and not just destruct() in your database class, otherwise it sees is as a class function . It seems to be working fine for me.

I'm aware of that, I don't have a valid destruct function in database class, but I do have a valid one in the rest of the classes. I don't know really why it's messing around, but my guess so far is that it's giving me 500 internal server error because of me using the class in subclasses. Which makes no sense because in subclasses you call the constructor of the parent again. It got me irritated so I did it like this and left the database class without a __destruct() function.

 

[MayoMayn]

I'm aware of that, I don't have a valid destruct function in database class, but I do have a valid one in the rest of the classes. I don't know really why it's messing around, but my guess so far is that it's giving me 500 internal server error because of me using the class in subclasses. Which makes no sense because in subclasses you call the constructor of the parent again. It got me irritated so I did it like this and left the database class without a __destruct() function.

Oh yeah okay, I only extend my autoload class, so it works fine for me

 

[Zaka]

Oh yeah okay, I only extend my autoload class, so it works fine for me

Yeah, so there is really no query in there that needs to be executed after database class is run. Also I don't use an autoload class since there isn't really loads of classes to load. Seems like overkill, but might do it just because it looks prettier.

 

[MayoMayn]

Yeah, so there is really no query in there that needs to be executed after database class is run. Also I don't use an autoload class since there isn't really loads of classes to load. Seems like overkill, but might do it just because it looks prettier.

Yeah, I got around 20 classes, so its just way easier that way for me

 

[Zaka]

Yeah, I got around 20 classes, so its just way easier that way for me

I guess it's just good practice to add a autoload class so that in the future, if new classes were to be added, they would basically just have to put the file in the correct folder.

 

[Zaka]

Dunno if this can be of any use to your CMS, but I've just begun coding a container for the sake of it:
index.php

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

$mysql = parse_ini_file('mysql.ini');

if(!defined('DIR')) {
    define('DIR', dirname(__FILE__) . DIRECTORY_SEPARATOR);
    foreach($mysql as $key => $value) {
        define($key, $value);
    }
}

require_once DIR . 'app/class.container.php';

$container = new Container();

// Set all the required classes
$container->setClasses([
    'Database'    => 'app/class.db.php',
    'Core'         => 'app/class.core.php'
]);

// Use the class outside a function
$core = $container->Core;

echo $core->test();

class.container.php

class Container
{

    /**
     * @var array;
     */
    public static $library, $class, $store;

    /**
    * How to access the Container file.
    * There's two ways:
    * 1) __construct function
    * 2) extend Container class
    */

    public function __construct() {
        if(isset(self::$class)) {
            foreach(self::$class as $key => $value) {
                $this->{$key} = $value;
            }
        }
    }

    public function setClasses($class = []) {
        if(!isset(self::$class)) {
            //$dir = DIR . "app/*/";
            foreach($class as $name => $file) {
                $file = DIR . $file;
                //$name = substr($file, 0, strrpos($file, '.'));
                //$name = ucfirst(substr($name, ($pos = strpos($name, '.')) !== false ? $pos + 1 : 0));
                //$name = ucfirst(substr($file, strlen($dir) + 6, -4));

                if(!isset(self::$library[$name])) {
                    require_once $file;
                    if(class_exists($name)) {
                        self::$library[$name] = $file;
               
                        self::$class[$name] = new $name(new self);
                        $this->{$name} = self::$class[$name];
                    } else {
                        die("Unable to load class: {$name}");
                        unset(self::$library[$name]);
                    }
                }
            }
        }
    }

    public function __destruct() {
        self::$store = [];
    }

    public static function setLocal(array $data) {
        if(!isset(self::$store[md5($data)])) {
            foreach($data as $key => $value) {
                self::$store[$key] = $value;
            }
            self::$store[md5($data)] = true;
        }
    }

    public static function getLocal() {
        return (isset(self::$store) ? self::$store : null);
    }

  public static function getClasses() {
        return self::$class;
    }

    public static function getLibraries() {
        return self::$library;
    }

    public static function getLibrary($name) {
        return self::$library[$name];
    }

    public static function getClass($name) {
        return $this->{$name};
    }

}

class.core.php - with __construct()

class Core
{

    protected $db;

    public function __construct(Container $container) {
        $this->db = $container->Database;
    }

    public function test() {
        $result = $this->db->query("SELECT `username` FROM `users` WHERE `rank`>= 1")->run()->fetch();
        return $result;
    }

    public function test2() {
        return $this->core->getIP();
    }

}

class.core.php - with extended class

class Core extends Container
{

    protected $db;

    public function test() {
        $result = Database::query("SELECT `username` FROM `users` WHERE `rank`>= 1")->run()->fetch();
        return $result;
    }

    public function test2() {
        return $this->core->getIP();
    }

}

class.db.php

class Database {

    protected $HOST = hostname;
    protected $DB   = database;
    protected $USER = username;
    protected $PASS = password;
    protected $CHAR = charset;

    private static $pdo, $stmt;

    public function __construct() {
        if(!isset(self::$pdo)) {
            try {
                self::$pdo = new \PDO(sprintf("mysql:host=%s;dbname=%s;",
                  $this->HOST,
                  $this->DB),
                    $this->USER,
                  $this->PASS,
                    [
                        \PDO::ATTR_EMULATE_PREPARES => false,
                        \PDO::ATTR_ERRMODE => \PDO::ERRMODE_EXCEPTION
                    ]
                );
            } catch(PDOException $e) {
                die('An error occurred when trying to communicate with the database.');
            }
        } else {
            return self::$pdo;
        }
    }

    public function __destruct() {
        // Destruct the connection to DB once finished
        try {
            self::$pdo  = null;
        } catch(PDOException $e) {
            die('An error occurred when trying to close the connection with the database.');
            //die($e->getMessage());
        }
    }

    public static function query($sql) {
        self::$stmt = self::$pdo->prepare($sql);
        return new self;
    }

    public static function run($data = null) {
        try {
            self::$stmt->execute($data);
            return new self;
        } catch(PDOException $e) {
            die("Execution error: " . $e->getMessage());
        }
    }

    public static function fetchAll($type = \PDO::FETCH_ASSOC) {
        return self::$stmt->fetch($type);
    }

    public static function fetch() {
        return self::$stmt->fetchColumn();
    }

    public static function fetchEach($data = []) {
        foreach(self::$stmt as $value) {
        }
    }

}

I did make it automatic checking for all files inside the app folder, but there were some issues, so I just decided that you'd have to manually choose the url for the class, which is fine by my opinion.
I'm definitely gonna use this for my own.

Some fine code right there. So what is a container class really good for? Now this is some new territory, and it seems very interesting, so I would like to understand this fully. When I do understand it, then I can think about implementing the code into the CMS.