5
\$\begingroup\$

I am quite new to the MVC concept. I already know how to program object-orientated from other languages like C++ or Java. I have implemented a little login system for test purpose. I am not sure if my implementation is the best way to follow MVC. Any feedback is welcome.

The class ControllerUser is instantiated in a file called init.php. The $controllerUser variable is therefore accessible on every page. I am also not sure if this is the best way. Can somebody also explain to me when to use the view class and which functions should it have? I know that it should render/output the page and the data... does this mean that it should print the logout/login form?

This is my database class in which the connection is established. All models will inherit from this class.

class.database.php

class Database {
    protected $pdo;

    public function connect() {
        $datahost =  'localhost';
        $datauser =  'root';   
        $datapass =  'PASSWORD'; 
        $database =  'DATABASE';
        try {
            $options = [ PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC ];
            $this->pdo = new PDO('mysql:host='.$datahost.';dbname='.$database.'', $datauser, $datapass, $options);
        } catch (PDOException $e) {
            print "Mysql Connection failed: " . $e->getMessage();
            die();
        }
    }

    public function disconnect() {
        $this->pdo = NULL;
    }
}

Everything will have an own model, controller and view, when necessary. In this case it is the user.

User controller:
controller.user.php

<?php
class ControllerUser {
    private $model;

    function __construct($datatable) {
        $this->model = new ModelUser($datatable);
    }

    public function login($email, $password) {
        $user = $this->model->getByEmail($email);
        if($user && password_verify($password,$user['password'])) {
            if($user['status']==1) {
                if($user['verified']==1) {
                    $_SESSION['id'] = $user['id'];
                    $this->model->updateIP($user['id']);
                    return true;
                }
                else {
                    throw new Exception('Not verified!');
                }
            }
            else {
                throw new Exception('Your account is locked. Contact a staff member.');
            }
        }
        else {
            throw new Exception('Email or password wrong!');
        }
    }

    public function logout() {
        if($this->isLoggedIn()) {
            session_destroy();
            unset($_SESSION['id']);
            return true;
        }
    }

    public function isLoggedIn() {
        if(isset($_SESSION['id'])) {
            $status = $this->model->getById($_SESSION['id'])['status'];
            if($status==1) {
                return true;
            }
            else {
                throw new Exception('Your account is locked. Contact a staff member.');
            }
        }
    }
}
?>

User model:
model.user.php

<?php
class ModelUser extends Database {

    private $datatable;

    function __construct($datatable) {
        $this->connect();
        $this->datatable = $datatable;
    }

    public function getByEmail($email) {
        $stmt = $this->pdo->prepare("SELECT * FROM ". $this->datatable ." WHERE email = :email");
        $result = $stmt->execute(['email' => $email]);
        return $stmt->fetch();
    }

    public function getById($id) {
        $stmt = $this->pdo->prepare("SELECT * FROM ". $this->datatable ." WHERE id = :id");
        $result = $stmt->execute(['id' => $id]);
        return $stmt->fetch();
    }

    public function updateIP($id) {
        $statement = $this->pdo->prepare("UPDATE ". $this->datatable ." SET ip = :ip WHERE id = :id");
        return $statement->execute(['ip' => $_SERVER['REMOTE_ADDR'], 'id' => $id]);
    }

    function __destruct() {
        $this->disconnect();
    }
}
?>

User view... I am not sure what could be inside of view... The function there is only for test purpose:
view.user.php

<?php
class ViewUser {
    public function printUserInformation($data) {
        foreach($data as $key => $value){
            echo $key.': '.$value.'<br>';
        }
    }
}
?>

Everything comes to use on the login page:
login.php

<?php 
if(isset($_GET['login'])) {
    $viewUser = new ViewUser(); //test
    $modelUser = new ModelUser("users"); //test
    try {
        $controllerUser->login($_POST['email'], $_POST['password']);
        $viewUser->printUserInformation($modelUser->getById($_SESSION['id'])); //just a test
    }
    catch(Exception $e) {
        echo $e->getMessage();
    }
}

if(isset($_GET['logout'])) {
    $controllerUser->logout();
}

if(!$controllerUser->isLoggedIn()) { 
?>
<form class="login-form" action="?login=1" method="post">
<input type="email" size="40" maxlength="250" name="email" placeholder="Email address" required>
<input type="password" size="40"  maxlength="250" name="password" placeholder="Password" required>
<button type="submit" value="Login" class="button style1">login</button>
<input type="checkbox" id="stayloggedin" name="stay_loggedin"><label for="stay_loggedin" >stay logged in</label>
</form>
<?php }
else {
?>
<form class="logout" action="?logout=1" method="POST" style="display: inline-block;">
    <button type="submit" name="logout">logout</button>
</form>
<?php }
?>

\$\endgroup\$
0
6
+50
\$\begingroup\$

You have a lot of problems in your code, so let's break them down.

First, you have a database class that is useless. Add all of your database access config (user, password, host, etc) in configuration files. Preferably in environment variables. Then, use PDO directly when you need a connection.

Second, you have a ModelUser class. Imagine your manager comes and requests you a feature to notify the user when he doesn't log in for a month. Is he gonna say "I want to notify the model user" or "I wan to notify the user"? So, rename your classe to User.

Third, your domain class (User) knows about database details. What if you want to save it in a file? Or send it throught a message queue? An User must have email, password, etc. It doesn't have a $database. That doesn't make sense. Search for Entity and Repository patterns.

Fourth, you said: "The class ControllerUser is instantiated in a file called init.php". That's not cool. You should use the front controller pattern. You have a single entry point in your application that configures routes and tells to which controller a request should go.

Fifth, a Controller must only:

  1. Receive a request
  2. Return a response

Sixth, the view layer can't have any logic. It should be just html.

Seventh, you should follow the PSR-12 in you code style: https://www.php-fig.org/psr/psr-12/

I strongly sugest you study more about OOP, Domain-Driven Design, design patterns, etc.

I have a simple MVC example but the code is in Portuguese: https://github.com/CViniciusSDias/php-mvc

\$\endgroup\$
5
  • 1
    \$\begingroup\$ I am a little bit confused. You said that the controller must only get a request and return a response. I read on many pages that the controller has all the logic and the model all the database stuff. Therefore, when a user should be notified there would be a function in the controller called notifyInactive() or so which then calls the function of the model to get data of inactive users. Then the controller would use that and send an email to those. That is how I understand it... \$\endgroup\$ – Chryfi Apr 14 '20 at 14:48
  • 3
    \$\begingroup\$ On many pages they were wrong. You can read the books related to MVC from the 80's that used smaltalk to understand how MVC really works. Imagine this: You have all your business logic in your controller, and now you need to run the same logic from the CLI. How would you do it? You can't run the Controller from the CLI, right? That's why the controller just receives a request and returns a response. The logic HAS to be elsewhere. \$\endgroup\$ – Vinicius Dias Apr 14 '20 at 17:53
  • 2
    \$\begingroup\$ Well... that's unfortunate that many pages are wrong on that topic. I'll then have to rethink everything. But the concept of OOP is not stuck together with MVC, meaning that you don't need to know MVC to know OOP. The only thing I need to investigate further are those books you named (and maybe the standard coding styles). Thanks for the feedback. \$\endgroup\$ – Chryfi Apr 14 '20 at 21:13
  • \$\begingroup\$ You are right. OOP is not stuck with MVC. You can study more robust architecture patterns other than MVC, like: Hexagonal Architecture, Clean Architecture, SOA, etc. \$\endgroup\$ – Vinicius Dias Apr 15 '20 at 20:36
  • 1
    \$\begingroup\$ Looking at the tag page I thought at first: gosh, a spammer! But now I see you know your stuff. Welcome to codereview! \$\endgroup\$ – Your Common Sense Apr 16 '20 at 13:02

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.