Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to learn PHP in depth by creating this personal CMS. I try to utilize OOP concepts and do best I can...

The way I am trying to design this CMS is to keep including 'modules' within a page to minimize code. I am using extensively POST and GET variables to decide which module will be reached. The Index page will be the main page from which modules will be included and accessible.

My questions:

  • Will this design be handy when code get more and more complicated? More POST requests etc.
  • I could there be any security vulnerabilities?
  • Any performance issues? (None noticed)
  • Do I utilize code properly as this is my real PHP project?
  • I would love some intensive criticism about this code.

Config File:

<?php
    class cms {
        private $dbhost     = "";
        private $dbuser     = "";
        private $dbpass     = "";
        private $dbdb       = "";
        private $dbc;
        public $website;
        public $user;
        public $theme;

        function __construct() {
            $this->session();
            $this->getWebsite();
        }
        function connect() {
            $dbc = mysqli_connect($this->dbhost, $this->dbuser, $this->dbpass, $this->dbdb) or die("Error " . mysqli_error($dbc));
            return $dbc;
        }
        function session() {
            if(!isset($_SESSION)) {
                session_start();
            }
        }
        function getWebsite() {
            $dbc = $this->connect();
            $stmt = $dbc->prepare("SELECT * FROM website");
            $stmt->execute();
            $result = $stmt->get_result();
            $this->website = $result->fetch_assoc();
            $stmt->close();
            $this->theme = $this->website['theme'];
        }
    }
?>

Index.php File:

<?php
    require_once('config/config.inc.php');
    $cms = new cms();
    include("template/" . $cms->theme . "/head.php");
    $moduleArray = array_diff(scandir('module/', 1), array('..', '.', 'admin'));

    if(($_SERVER['REQUEST_METHOD'] === 'GET' || $_SERVER['REQUEST_METHOD'] === 'POST') && !empty($_GET['page'])) {
        $p = $_GET['page'];
        $ext = ".inc.php";

        if (get_magic_quotes_gpc()) {
            $p = stripslashes($p);
        }

        if(in_array($p . $ext, $moduleArray)) { 
            include("module/" . $p . $ext);
        }
        else {
            header('Location: /');
        }
    } // end (GET || POST) && GET
    else {
        foreach($moduleArray as $module) {
            $name = substr($module, 0, -8);
            echo " <a href=\"index.php?page=$name\"> $name </a><br />";
        }
    } // end ELSE

    include("template/" . $cms->theme . "/foot.php");
?>

Demo Module: Page.inc.php

<?php
    $dbc = $cms->connect();

    if(isset($_GET['page'])) {
        if(isset($_GET['title'])) {
            $t = $_GET['title'];

            if (get_magic_quotes_gpc()) {
                $t = stripslashes($username);
            }

            if ($stmt = $dbc->prepare("SELECT title, content_main FROM pages WHERE url=?")) {       
                $stmt->bind_param('s', $t);
                $stmt->execute();
                $stmt->store_result();
                $stmt->bind_result($title, $content_main);

                if($stmt->fetch()) {
                    echo "<h2>$title</h2><p>$content_main</p>";
                }
                else {
                    echo "<h2>404</h2><p>The page doesn't exist!</p>";
                }

                $stmt->free_result();
                $stmt->close();
            }
            else {
                echo "error";
            }
        }
        else {
            if ($stmt = $dbc->prepare("SELECT title, url FROM pages")) {        
                $stmt->execute();
                $stmt->store_result();
                $stmt->bind_result($title, $url);

                while ($stmt->fetch()) {
                    echo "<a href=\"index.php?page=page&title=$url\">$title</a></br />";
                }

                $stmt->free_result();
                $stmt->close();
            }
        }
    }
    else {
        header('Location: ../');
    }
?>
share|improve this question

3 Answers 3

up vote 11 down vote accepted

Developement

& tags you've used are already saying what needs to be done here. At this moment it's a structural code (with some class syntax) and it will be very difficult to expand. Even though it doesn't look like a big project yet and you don't need deep object structures/layers here, I think you should learn some oop anyway. First advantage you'll notice will be less reapeating code. Separation of Request processing, Modeling page content and fetching it further to the "View" code that will handle displaying will make developement much easier. I think that How to write this code in oop manner? should be the question here.

Security

There not much security concenrs in the code you've shown. Using prepared statements will protect you against SQL injection and I don't expect more at the moment assuming that displayed content is supplied by site owner (but owner recognition is important here). Even injection itself wouldn't be a problem if you validate request first (mentioned above). I'm not saying you should drop prepared statements - stick with it even if sometimes it's only for good practise sake.

Performance

No need for file whitelisting using directory scan and checking if request hits one of the files. I'd simply use file_exists() (with some conditions if needed). And again: it could be part of request validation.

Storing page content in database is an overkill imo since you display it directly to the screen. Why not writing it as files? You don't and can't really do anything data-processing-wise with it (sorting, grouping...) except for FULLTEXT search maybe (that could be resolved another way). Simple include would replace db SELECT. Same goes for website table - it looks like a (semi-)constant config that could be stored in file (as a db cache at least).

share|improve this answer
    
I got a question, you mentioned that reading the database each time the page was accessed is a bad idea. how could this be solved? I could use sessions, but then there's the need of cookies, how could I store data without cookie? file on the server per user ? –  HelpNeeder Jul 25 at 5:35
1  
Implementing user accounts and not only CMS content admin changes a lot. It's hard to avoid using sessions/cookies in this case and using db along with it is justified. The topic is too wide to get into details here, but there's plenty of tutorials and scripts covering this. I advice you not going after "smarter" solutions on this matter and accept popular ones. Also note that higher level of security is required, as your user supplied content might try some nasty things. –  shudder Jul 25 at 16:38

Alright, I'll start off by answering the questions you've asked, and depending if it all get's covered, I might append more at the end. We'll see, I have yet to actually read your questions!

So, let's grab the first one...

Will this design be handy when code get more and more complicated? More POST requests etc.

No. Right now, I see no way of expanding this. As soon as it does "get more and more complicated", you will run into lots of problems, especially with the website architecture.

Part of the Object Oriented paradigm is a principle called SOLID. If we start off with the S, which stands for Single Responsibility, we immediately notice multiple flaws in your code.

For instance, your cms class is handling database connections, database querying, and session handling. So really, that tells us right off the bat that cms isn't even necessary! It tells you that you really need a database connection handler, a querying utility, and a separate session handling class. But even those could be broken down further to prevent having all-powerful "Manager" classes.

The second principle in SOLID is code be open and closed. That means open for extension and closed to modification. I see neither in your code right now. We cannot build onto cms or the index file or the demo page. And it's certainly going to be a pain to alter these classes, and since we even need to alter them, your code is not closed.

One possible solution would be follow KISS. Because your classes are responsible for so much, it's hard to keep things separated. This makes it difficult to change page-unique information. Give the functionality room to breathe, don't expect things to always fit in the same class, because that's not OOP.

Study the rest of SOLID, it's a large topic and I would be able to point out other mistakes, but it's best we move on!

could there be any security vulnerabilities?

Well, what are you afraid of? And under what circumstances are the code and the developer being placed in?

I say this because the threats will be different if you're the only one seeing this site, as opposed to the threats where the attacker knows your exact database scheme.

I'll point out some issues I see, but I cannot give a full security audit as I'm not a professional.

  • Using .inc.php is a convention, but can be considered a bad one. There are threats associated to using it if you leave off the appended .php. Luckily, you look good! Be careful though.
  • As of the PHP docs, using magic quote functions is deprecated. Avoid using any of these as support may fault and open you up to different types of attacks.
  • Following that, you strip slash the page. However, that's only if the magic quotes are working. If they're not, the attacker can include any page he wants (given the right permissions). I'd avoid giving the user so much power.
  • stripslashes($username) doesn't make sense. I'm not sure why you'd do this if you're using prepared statements. On top of this, I don't even know where you're getting $username from. It's dead code, which is also a security risk.
  • All of the code you're injecting into your page via the database better be the the most clean code ever. If an attacker is able to insert page data into your database, they could potentially insert malicious HTML/JavaScript into the pages that every site user will see. Very bad.

This is what I could spot immediately, I'm sure I missed something though.

Any performance issues?

That's hard to say. Why don't you profile your code and see if it bottlenecks at any points? Then examine those parts and see about refactoring them.

From what I can tell, the database looks inefficient and you make a lot of unnecessary calls. Improper connection storage and caching also looks prevalent here. Every time you load a page, you start a new database connection, and then perform a whole new set of queries. Will the theme be changing on every single page load, or is that something you can store?

Do I utilize code properly as this is my real PHP project?

What does "properly" mean? I don't even know how to answer this! Are you asking if this is production worthy? If so, I'd automatically say no for all of the points I have and will point out.

I would love some intensive criticism about this code.

Well perfect, because I still have lots! :)

  • First off, follow a PHP coding style standard. To start you off, I'll point you in the almost official PHP-FIG standard. It's important that every bit of code is consistent and readable. Follow the styles for naming things especially.
  • Avoid shorthanding names such as "database" in names like $dbhost. It doesn't provide any benefit to shorten. Same with "password" in $dbpass.
  • $dbdb is ridiculous. This obviously needs a new name.
  • $dbc has no meaning or clarity what-so-ever. Give your variable context!
  • Things like public $website should not be public. Encapsulate and give them getters and setters.
  • Your constructor should not be creating those types of variables. The whole class needs to be rethought, and then apply dependency injection principles, learn more from the D in SOLID.
  • Give your methods a visibility. Leaving the default is considered bad practice.
  • connect() is bad name. Connect to what? What is cms connecting to? Oh! Maybe it needs to connect to the Google Patents API!
  • session() really doesn't belong here, we talked about this.
  • getWebsite() is extremely vague. This needs a lot of work.
  • $moduleArray has Hungarian notation and it seems confused about it's responsibility.
  • You really need to lay off the single character variables ($p, $t)
  • If your code needs comments such as // end (GET || POST) && GET, then your conditional is too long. I shouldn't have to need a comment to know the different stages of the code.
  • Overall, Index.php has a lot of responsibility which could be out sourced to different files/classes.
  • $dbc = $cms->connect(); where is $cms defined; this is dead code.
  • I see you have 4 levels of nesting, you need to work on breaking this down. Either by simple refactoring, or if you can, guard clauses.
  • echo "error"; looks terrifying. Add in user-friendly and proper error handling. The Stack Exchange network is perfect for finding questions about how to do that.
  • Having literals such as <a href=\"index.php?page=page&title=$url\">$title</a></br /> are very hard to read. Either eleminate the HTML from the PHP (and do it dome where else), or use a function like sprintf.

Well, that's what I could squeeze out! I hope you know what needs to be done :) I'd be happy to answer any questions.

share|improve this answer
    
Awesome! It will be a bit before I digest that but I really appreciate the response. Thank you. –  HelpNeeder Jul 24 at 4:12
    
I wasn't the one who asked for "intensive criticism" ;) –  Alex L Jul 24 at 4:13
3  
It's super good. This was my point. :P –  HelpNeeder Jul 24 at 4:14
    
So it's said, the security issues with .inc are not present when using .inc.php. (The problem with .inc is that the server is not usually configured to treat them as PHP. That's not an issue when the filename has .php at the end. –  cHao Jul 24 at 8:51
    
@chao I knew something was strange about the extension, unfortunately I misread that page. Thanks! –  Alex L Jul 24 at 9:01

This is my first review. Let's take a look at your code.

One class, one task.

Firstly, I would suggest you to use namespaces.

Namespaces are very handy when it comes to large projects, which will be further expanded. Click here for the Documentation. The reason I am suggesting you to use a Namespace is because you have to simplify your code, and not use just one class to handle everything. Perhaps, you can have one class to store the logic/variables of the program, and another class to store all functions which handle user interaction and call the Mapper or such to do the job for it. Let's take a look at a sample code we can use.

namespace CMS;
class UserHandler {

}
class Mapper {

}
class CMSData {

}
/* In this below section, we instantiate each class according to the way we will pass their dependencies to the class. */

$CMSData     = new CMSData();
$Mapper      = new Mapper($CMSData);
$UserHandler = new UserHandler($CMSData, $Mapper);

If you do not know about Dependency Injection, click here.

In this above code, we have created a class called CMSData, to carry all data, and that class is accessible by every other class. So, when you want to do something with Data Storage or whatever, your class would be CMSData, where you change the stuff. The UserHandler class is to Handle user interactions. Let's take an example, suppose your $_GET is tell you that the URL has changed, and a new page has to load, so there would be a function called, lets say handlePageLoad in the index.php and an argument which contains what page is requested is also passed along, to the UserHandler class. The UserHandler class calls functions in the Mapper class which does validity checks, etc and if necessary, stores some information in the CMSData, where there are bunch of variables available.

Will this design be handy when code get more and more complicated? More POST requests etc.

Your current design will be too messy, unless you organize it. If you code it in a Generalized way like I suggested you, then it will be fine with more complicated code.

I could there be any security vulnerabilities?

No, as long as you take in user input in a secure way, there will not. And it seems that you are doing it right.

Any performance issues? (None noticed)

I don't see anything wrong. Just make sure you try simplifying code in a better logic.

Do I utilize code properly as this is my real PHP project?

I would like to suggest you, that only allow your Mapper class access to Database, and the other pages, have to call out functions to the Mapper class directly for Database related work, and you create different functions which will be available there, this will allow you to make your program easy to manage. For example, you have a new theme, and you want to display things a bit more different, all you have to do is, go to your class, and edit everything accordingly, instead of you having to find code all over.

I would love some intensive criticism about this code.

Why not use keywords like public, private and protected in functions? You are doing it on variables only.

share|improve this answer
    
Those last two points remind me of another answer here ;) good to see the tables turn and watch you answer instead of ask! :) –  Alex L Jul 24 at 7:35
    
Awesome points! I will try to do more reading about all this! Thank you! –  HelpNeeder Jul 24 at 7:36
    
Oh @AlexL I'll remove them since you have stated. Didn't know. –  Hassan Althaf Jul 24 at 7:39

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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