3
\$\begingroup\$

I want to learn OOP and I am creating a little CMS for practice and to use it as a base for other projects in the future.

This is the complete code: GitHub

My major concerns are code structure, security and re-usability, but any feedback is welcome.

About code structure, I am using classes and it all works well but the code seems to me very procedural. Sorry I cannot explain it better, I simply think that I am doing something wrong from the OOP point of view.

For example, I created a router class to display the correct template depending on the URL parameters. But then on some of the templates I need to add some code at the top and it seems to me like I can do it a better way.

router.class.php

<?php

if ( !class_exists( 'router' ) ) {

    class router {

        private $theme_path = null;

        function __construct() {
            global $theme;
            if( $this->theme_path == null ) {
                $this->theme_path = $theme->get_theme_path();
            }
        }

        public function start( $action, $object, $admin_check ) {
            switch ( $action ) {
              case 'list':
                $this->display_list($object);
                break;
              case 'view':
                $this->display($object);
                break;
              case 'edit':
                $this->edit($object);
                break;
              case 'delete':
                $this->delete($object);
                break;
              case 'list':
                $this->display_list($object);
                break;
              case 'options':
                $this->options();
                break;
              case 'register':
                $this->register();
                break;
              case 'login':
                $this->login();
                break;
               case 'logout':
                $this->logout();
                break;
              default:
                if( $admin_check == true ) {
                    $this->dashboard();
                } else {
                    $this->homepage();
                }
            }
        }

        public function dashboard() {
            global $post, $user, $auth, $admin;
            require( ADMIN_PATH . "dashboard.php" );
        }

        public function homepage() {
            global $post, $user, $auth, $frontend, $admin, $theme;
            $posts_array = $post->all_posts();
            require( $this->theme_path . "index.php" );
        }

        public function options() {
            global $post, $user, $auth, $frontend, $admin, $theme, $options;
            $opt_array = $options->all_options();
            require( ADMIN_PATH . "options.php" );
        }

        public function edit($object) {

            global $post, $user, $auth, $admin, $taxonomy, $term, $media;

            if ( !isset($_GET["id"]) || !$_GET["id"] ) {
                $id = '';
            } else {
                $id = $_GET["id"];
            }
            require( ADMIN_PATH . "edit-".$object.".php" );

        }

        public function delete($object) {
            global $post, $user, $auth, $admin, $taxonomy, $term;
            $id = $_GET["id"];
            switch ( $object ) {
                case 'post':
                    $post->delete($id);
                break;
                case 'cat':
                    $taxonomy->delete($id);
                break;
                case 'tag':
                    $taxonomy->delete($id);
                break;
                case 'user':
                    $user->delete($id);
                break;
                case 'media':
                    $media->delete($id);
                break;
            }
            $this->display_list($object);
        }

        public function display($object) {
            if ( !isset($_GET["id"]) || !$_GET["id"] ) {
                $this->homepage();
                return;
            }
            global $post, $user, $auth, $frontend, $term, $theme;
            $id = $_GET["id"];

            switch ( $object ) {
                case 'post':
                    $object_data = $post->select_by_id($id);
                    require( $this->theme_path . "post.php" );
                break;
                case 'cat':
                    $object_data = $term->select_by_id($id);
                    require( $this->theme_path . "cat.php" );
                break;
                case 'tag':
                    $object_data = $term->select_by_id($id);
                    require( $this->theme_path . "tag.php" );
                break;
                case 'user':
                    $object_data = $user->select_by_id($id);
                    require( $this->theme_path . "user.php" );
                break;
            }
        }

        public function display_list( $object ) {
            global $post, $user, $auth, $admin, $frontend, $term, $taxonomy, $media, $theme, $admin_check;

            if( $admin_check == true ) {
                $path = ADMIN_PATH;
            } else {
                $path = $this->theme_path;
            }

            switch ( $object ) {
                case 'post':
                    $objects = $post->all_posts();
                    if( $admin_check == true ) {
                        require( $path . "posts.php" );
                    } else {
                        require( $path . "archive.php" );
                    }
                break;
                case 'cat':
                    $objects = $taxonomy->all_taxonomy('category');
                    require( $path . "cats.php" );
                break;
                case 'tag':
                    $objects = $taxonomy->all_taxonomy('tag');
                    require( $path . "tags.php" );
                break;
                case 'user':
                    $objects = $user->all_users();
                    require( $path . "users.php" );
                break;
                case 'media':
                    $objects =  $media->display_all();
                    require( $path . "media.php" );
                break;
            }
        }

        public function register() {
            global $post, $user, $auth, $admin;
            require( ADMIN_PATH . "register.php" );
        }

        public function login() {
            global $post, $user, $auth, $admin;
            require( ADMIN_PATH . "login.php" );
        }

        public function logout() {
            global $post, $user, $auth, $admin;
            $auth->logout();
            require( ADMIN_PATH . "login.php" );
        }

    }

}

$router = new router;

?>

And this is the edit-user.php template:

<?php $admin->get_header(); ?>
<?php

if ( !empty ( $_POST ) && empty( $id ) ) {

    if ( $id = $auth->register() ) {

        echo '<p>Data inserted successfully!</p>';
        $user_data = $user->select_by_id($id);

    } else {

        echo 'Passwords are not the same';

    }

} elseif ( !empty ( $_POST ) && !empty( $id ) ) {

    if ( $user->update( $id, $_POST ) ) {

        echo '<p>Data updated successfully!</p>';
        $user_data = $user->select_by_id($id);

    }

} elseif ( empty ( $_POST ) && !empty( $id ) ) {

    $user_data = $user->select_by_id($id);

}

$name = isset( $user_data ) ? $user_data['user_name'] : "";
$username = isset( $user_data ) ? $user_data['user_login'] : "";
$email = isset( $user_data ) ? $user_data['user_email'] : "";

?>

<main>

    <h1 class="page-title">New User</h1>

    <div class="wrap edit-user">

        <form method="post" enctype="multipart/form-data">

            <?php if ( empty( $id ) ) { ?>
                <input type="file" name="image" />
            <?php } elseif( !empty( $id ) ) {
                $user_data = $user->select_by_id($id);
                ?>
                <div class="uploaded-images">
                    <img src="<?php echo $media->get_url( $user_data['user_avatar'] ) ?>" width="60px" />
                </div>
                <input type="file" name="image" />
            <?php } ?>

            <label>Name:</label>
            <input type="text" name="user_name" value="<?php echo $name ?>" />

            <label>Username:</label>
            <input type="text" name="user_login" value="<?php echo $username ?>" />

            <label>Email:</label>
            <input type="text" name="user_email" value="<?php echo $email ?>" />

            <label>Password:</label>
            <input type="password" name="user_pass" />

            <label>Repeat Password:</label>
            <input type="password" name="user_pass_rpt" />

            <?php if ( empty($user_data) ) { ?>
                <input type="hidden" name="user_registered" value="<?php echo time(); ?>" />
            <?php } ?>

            <input type="submit" value="Submit" />

        </form>

    </div>

</div>

<?php $admin->get_sidebar(); ?>

<?php $admin->get_footer(); ?>

The CMS have this database tables: posts, terms, term_relationships, term_taxonomy, users, options.

PS: In order to not make you wasting time, I know that:

  • in a production site a would have a to turn off errors, but now I am in development ;)
  • that I have to validate all the data inserted in the database. I have not done it yet in order to keep it more simple... I will do it in the future.
\$\endgroup\$
1
  • \$\begingroup\$ There are a lot of different ways to learn how to code OOP, frankly I think starting with a custom CMS is not the way. There's a lot of things that you can unintentionally get wrong and this will lead to you practicing imperfectness. A lot of work goes into a complete CMS, and I suggest you start with a game or something else. :) \$\endgroup\$
    – Alex L
    Dec 22, 2015 at 3:52

1 Answer 1

3
\$\begingroup\$

Security

Validating data when inserting it in the database is always a good idea, but should not be the main defense against anything (SQL injection should be handled with prepared statements, as you are doing; XSS should be handled via HTML encoding, etc).

You are probably open to persistent XSS via name, username and email. When echoing variable data, always HTML encode it. This is a self-XSS, but it still matters, especially if your login is not CSRF protected, in which case an attacker can perform CSRF to force-login a victim and then execute the injected JavaScript code.

It also seems that you are open to CSRF attacks. Your forms should have an anti-CSRF token, which is at least not present in the code posted here, and which I could also not find in the rest of the code on github.

When including things like this: require( ADMIN_PATH . "edit-".$object.".php" );, it would be ideal to check that no directory traversal takes place. Zero byte poisoning and path truncation are not possible in newer PHP versions, but it's still a good practice (an attacker could eg still include PHP files).

Structure

View

Your view template does a lot more than present data. It also handles data.

Ideally, a template only contains HTML and echoes variables it was given. It should not read out user input, and it should not perform changes.

Router

Your router also does more than simple routing. What I would expect from a router is that it reads out the given URL, and then calls the appropriate methods in the right classes.

Ideally, you create a config file which contains allowed classes/methods and which maps those to a given URL structure (this is important as a whitelist for security reasons, and also gives you an easy way to reconfigure your URL structure later on).

A router should not care about what those methods are, or in what classes they are. Right now, if you want to add new actions, you have to not only update the class containing those actions, but also the router class. After a while, your router will grow quite large and will be hard to handle.

To give you an idea, here is some dummy code how it might look:

// config:
    POST /user/edit/[id] -> UserController.edit
    GET /user/show/[id] -> UserController.show
    POST /user/create -> UserController.create

//router:
/*
read URL, map it to config, call method (eg via call_user_func). 
Eg if the URL is /user/edit/5, you would call the edit method 
of the UserController class with the argument 5. 
If it matches none of the config patterns, show an error.
POST data can be extracted here as well, or be extracted in the controller.
Note that the router does not care what exactly is in the config, 
or what controllers exist.
*/

// controller
class UserController {

    public function edit($id, $username, ...) {
        $userDAO->editUser($id, $username, ...)
        $userTemplate->show($username, $email, ...);
    }

    [...]
}

Passing Data

Try to pass as much data as possible via methods, instead of declaring them in one script and then including a different script. In the first script, it's hard to see why those values need to be set and what happens to them, and in the second script it's hard to see where the data comes from.

Misc

  • You have quite a lot of global variables, which is not ideal. It's hard to handle, and hard to test.
  • You are inconsistent with your spacing.
  • labels should have a for attribute.
  • try to be consistent with your names. Eg the input user_name uses the variable name, while the input user_login uses the variable username, which is confusing.
  • files with classes in them should only contain the class, nothing else, to avoid side effects and increase reusability. Same goes for files with functions.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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