Helping a user manage a database

Question

The following code will read a db, show the contents to the user and provide instruments to edit them. This has been tested and works well.

model.php

<?php
class Collega {
    private $host="localhost";
    private $user="root";
    private $pwd="";

    private $attiva=false;

    public function connetti () {
        if (!$this->attiva) {
            if (!mysql_connect($this->host, $this->user, $this->pwd)) {
                die (mysql_error());
                exit ();
            } else {
                $this->attiva=true;
            }
        }
    }
    public function selectdatabase ($dbname) {
        if (!$this->attiva) {
            return false;
            exit ();
        } else {
            if (!mysql_select_db($dbname)) {
                echo mysql_error();
                return false;
                exit ();
            } else {
                return true;
            }
        }
    }
    public function interroga($sql) {
        if ($this->attiva) {
            $query = mysql_query($sql) or die (mysql_error());
            return $query;
        }
    }
}
$connessione = new Collega ();
$connessione->connetti();
$connessione->selectdatabase("conoscenze");
$result = $connessione->interroga("SELECT * FROM conoscenti");
?>

view.php

<?php
include ("model.php");
include ("controller.php");


class Visualizza {
    public function tetris ($rex) {
        echo "<table>";
echo "<tr><td>Nome</td><td>Cognome</td><td>Data di nascita</td><td></td><td></td></tr>";
        while ($row=mysql_fetch_array($rex)) {
            $birth=date_parse($row['Nato']);
    echo "<tr><td>".$row['Nome']."</td><td>".$row['Cognome']."</td><td>".$birth['day']."/".$birth['month']."/".$birth['year']."</td><td><form action='".$_SERVER['PHP_SELF']."?action=edit&id=".$row['id']."' method='post'><input type='hidden' name='nome' value='".$row['Nome']."' /><input type='hidden' name='cognome' value='".$row['Cognome']."' /><input type='hidden' name='nato' value='".$row['Nato']."' /><input type='hidden' name='index' value='".$row['id']."' /><button>Modifica</button></form></td><td><form action='".$_SERVER['PHP_SELF']."?action=delete' method='post'><input type='hidden' name='id' value='".$row['id']."' /><button>Elimina</button></form></td></tr>";
        }
        echo "</table>";
        echo "<form action='".$_SERVER['PHP_SELF']."?action=new' method='post'>";
        echo "<input type='hidden' name='nome' value='' />";
        echo "<input type='hidden' name='cognome' value='' />";
        echo "<input type='hidden' name='nato' value='' />";
        echo "<button type='submit'>Crea nuova voce</button>";
        echo "</form>";
    }
    public function editordati () {
        echo "<form action='".$_SERVER['PHP_SELF']."?action=submit' method='post'>";
        echo "<input type='text' value='".$_POST['nome']."' name='nome' />";
        echo "<input type='text' value='".$_POST['cognome']."' name='cognome' />";
        echo "<input type='date' value='".$_POST['nato']."' name='nato' />";
        echo "<input type='hidden' value='".$_GET['id']."' name='id' />";
        echo "<input type='submit' />";
        echo "</form>";
    }
    public function nuovo () {
        echo "<form action='".$_SERVER['PHP_SELF']."?action=submitnew' method='post'>";
        echo "<input type='text' value='' name='nome' />";
        echo "<input type='text' value='' name='cognome' />";
        echo "<input type='date' value='' name='nato' />";
        echo "<input type='submit' />";
        echo "</form>";
    }
}
$show=new Visualizza ();
if (!isset($_GET['action'])) {
    $show->tetris($result);
} elseif ($_GET['action']=="edit") {
    if (!$_GET['id']) {
        exit ();
    } else {
        $show->editordati();
    }
} elseif ($_GET['action']=="new") {
    $show->nuovo();
} elseif ($_GET['action']=="submit") {
    $penna->editexisting("conoscenti", $_POST['id']);
    $show->tetris($connessione->interroga("SELECT * FROM conoscenti"));
} elseif ($_GET['action']=="submitnew") {
    $penna->addnew("conoscenti");
    $show->tetris($connessione->interroga("SELECT * FROM conoscenti"));
} elseif ($_GET['action']=="delete") {
    $penna->destroyer("conoscenti", $_POST['id']);
    $show->tetris($connessione->interroga("SELECT * FROM conoscenti"));
}
?>

controller.php

<?php
class Editor {
    public function editexisting ($table, $id) {
        if (!isset($_POST['nome']) && !isset($_POST['cognome']) && !isset($_POST['nato'])) {
            exit ();
        } else {
            $editq="UPDATE ".$table." SET Nome='".$_POST['nome']."', Cognome='".$_POST['cognome']."', Nato='".$_POST['nato']."' WHERE id='".$id."'";
            $editr=mysql_query($editq) or die (mysql_error());
            return $editr;
        }
    }
    public function destroyer ($table, $id) {
        $delq="DELETE FROM ".$table." WHERE id='".$id."'";
        $delr=mysql_query($delq) or die(mysql_error());
        return $delr;
    }
    public function addnew ($table) {
        $addq="INSERT INTO ".$table."(id, Nome, Cognome, Nato) VALUES (null, '".$_POST['nome']."', '".$_POST['cognome']."', '".$_POST['nato']."')";
        $addr=mysql_query($addq) or die (mysql_error());
        return $addr;
    }
}
$penna= new Editor ();
?>

I named the scripts so it's easy to understand who plays what role in the MVC opera. Did I write a good MVC example or should I move some code somewhere else?

Answer 1

Classes and other code in files

Your classes should only be classes. There should be no code outside the class in the same file as a class, because when a class file is included, it is expected that nothing happens until one of it's methods is called.

You class names also seem to be wrong. Eg Collega isn't really a model of a colleague, but just a wrapper around mysql_.

Structure

• there shouldn't be SQL statements in your view or your controller. SQL statements should only be in one place, and that's either directly in the model, or inside a DAO.
• there shouldn't be any controlling code in your view. Your view only shows stuff, that's all it does. It doesn't deal with user input, that's what the controller does.
• as a result, your current controller doesn't contain any code that should be in a controller. If you fix the two points above, this should be fixed.

Security

Your code is extremely unsecure!

You are completely open to SQL injection as well as XSS. Never ever echo variable data without some kind of sanitation, and never ever put variable data into an SQL query without escaping it or using prepared statements.

Also, mysql_ is deprecated since 2013(!), don't use it.

• XSS: use htmlspecialchars to prevent it when echoing user input/all variable data. Values which have to be encoded: $_SERVER['PHP_SELF'], all $_GET/$_POST, all $row.
• SQLi: use prepared statements with mysqli_ or PDO.

Misc

• avoid duplication. You have your form three times. You can put it in a function which accepts the variable parts as arguments.
• if you can (and you only work with people on your code who also can) use english variable names.
• write function names in camelCase.
• use more spaces, eg around = and ..
• don't die in your model, it will make it hard to handle this in the controller.

Answer 2

@tim I'm stalling. I wrote a new model and a new view, but now i don't know what to put in the controller since it seems that model can do everything. Something's strange still.

model.php

<?php
class Model {
    private $host = "localhost";
    private $user = "root";
    private $pwd = "";

    private $active = false;

    private $mysqli;

    public function connectDatabase ($dbname) {
            if (!$this->active) {
                $this->mysqli = new mysqli($this->host, $this->user, $this->pwd, $dbname);
                if ($this->mysqli->connect_errno) {
                echo $this->mysqli->connect_errno.": ".$$this->mysqli->connect_error;
            } else {
                echo "Connected to database ".$dbname."<br />";
            }
        $this->active=true;
    }
}

private function stringifyList (array $list) {
    $n = count($list);
    $liststring = "";
    for ($i=0; $i<$n; $i++) {
        if (!($i==$n-1)) {
            $liststring = $liststring.$list[$i].", ";
        } else {
            $liststring = $liststring.$list[$i];
        }
    }
    return $liststring;
}
private function pointsOfQuestion (array $list) {
    $n = count($list);
    $what = "";
    for ($i=0; $i<$n; $i++) {
        if (!($i==$n-1)) {
            $what = $what."?, ";
        } else {
            $what = $what."?";
        }
    }
    return $what;
}
public function dataTypes (array $list) {
    $n = count($list);
    $dt = "";
    for ($i=0; $i<$n; $i++) {
        switch (gettype($list[$i])) {
            case ("string") :
            $dt = $dt."s";
            break;
            case ("double") :
            $dt = $dt."d";
            break;
            case ("integer") :
            $dt = $dt."i";
            break;
            case ("object") :
            $dt = $dt."b";
            break;
            case ("NULL") :
            $dt = $dt."i";
        }
    }
    return $dt;
}

public function selectq ($table, array $voices) {
    $mysqli=$this->mysqli;
    if (!$stmt = $mysqli->prepare("SELECT ".$this->stringifyList($voices)." FROM ".$table)) {
        echo "Errore ".$mysqli->errno.": ".$mysqli->error;
    } else {
        echo "Query eseguita correttamente<br />";
    }
    $stmt->execute ();
    $result = $stmt->get_result();
    $stmt->close();
    return $result;
}

public function insertq ($table, array $voices, array $params) {
    $mysqli=$this->mysqli;
    $voicestring = "";
    $paramsuse = array ();
    $args= array ();
    $what = "";
    $dt = "";
    if (!(count($voices) == count($params))) {
        echo "Errore: numero parametri e voci diversi. Controlla.";
    } else {
        $voicestring = $this->stringifyList($voices);
        //$paramstring = $this->stringifyList($params);
        $what = $this->pointsOfQuestion($voices);
        $dt = $this->dataTypes($params);
        $paramsuse[0] = $dt;
        $args = array_merge($paramsuse, $params);
        $refs = array ();

        foreach ($args as $k=>$arg) {
            $refs[$k]=&$args[$k];
        }
    }

    if (!$stmt = $mysqli->prepare("INSERT INTO ".$table." (".$voicestring.") VALUES (".$what.")")) {
        echo "Errore ".$mysqli->errno.": ".$mysqli->error;
    } else {
        echo "Preparazione eseguita <br />";
    }
    $bindparams = call_user_func_array(array($stmt, 'bind_param'), $refs);

    if (!$bindparams) {
        echo "Non ho bindato un cazzo. Errore ".$stmt->errno;
    } else {
        echo "Binding eseguito correttamente";
    }
    if (!$stmt->execute()) {
        echo "Non sono riuscito a eseguire :(";
    } else {
        echo "Database scritto con successo";
    }
    $stmt->close();
}
public function deleteq ($table, $voice, $param) {
    $mysqli=$this->mysqli;

    if (!$stmt = $mysqli->prepare ("DELETE from ".$table." WHERE ".$voice." = ".$param)) {
        echo "Qualcosa non va(".$stmt->errno.": ".$stmt->error;
    }
    $stmt->execute();

    if (!$mysqli->affected_rows>0) {
        echo "Non esiste!";
    } else {
        echo "Cancellato";
    }
    $stmt->close();
}

public function editq ($table, array $voices, array $params, $voice, $param) {
    $mysqli = $this->mysqli;
    $querystr = "";

    if (!(count($voices) == count($params))) {
        echo "Errore: numero parametri e voci diversi. Controlla.";
    } else {
        for ($i=0; $i<count($voices); $i++) {
            if (!($i == count($voices)-1)) {
                $querystr = $querystr.$voices[$i]." = "."'".$params[$i]."'".", ";
            } else {
                $querystr = $querystr.$voices[$i]." = "."'".$params[$i]."'";
            }
        }
    }

    if (!$stmt = $mysqli->prepare("UPDATE ".$table." SET ".$querystr." WHERE ".$voice." = ".$param)) {
        echo "Problemi: ".$mysqli->errno.": ".$mysqli->error;
    } else {
        echo "Modifica preparata <br />";
    }

    $stmt->execute ();
    $stmt->close ();
}
}
?>

view.php

<?php
class View {
    public function showTableFirstRow ($row) {
        $html = "<tr>";
        foreach ($row as $key => $value) {
            $html = $html."<td>".htmlentities($key)."</td>";
        }
        $html = $html."</tr>";
        return $html;
    }
    public function showResultTable ($row) {
        $html = "<tr>";
        foreach ($row as $key => $value) {
            $html = $html."<td>".htmlentities($value)."</td>";
        }
        $html = $html."</tr>";
        return $html;
    }
    public function tableWrap ($html) {
        echo "<table>";
        echo $html;
        echo "</table>";
    }
    public function showEditForm ($action, array $fields) {
        echo "<form action='".htmlentities($_SERVER['PHP_SELF'])."?action=".$action."' method='post'>";
        foreach ($fields as $key => $value) {
            echo "<input type='text' name='".$key."' value='".$value."' />";
        }
        echo "<button type='submit'>Invia</button>";
        echo "</form>";
    }
}
?>