Bei den Funktionen, die direkt Werte verarbeiten, die von außen kommen, wäre ich restriktiver. Für getContentSite sollte z.B. sichergestellt sein, dass wirklich nur die beabsichtigten PHP-Seiten aufgerufen werden können. Sammel die am besten in einem speziellen Ordner, in dem niemals andere PHP-Dateien abgelegt werden.
Da die Funktion versucht, die Datei direkt einzubinden, würde ich sie etwas anders benennen.
function includeContentSite($defaultSite) {
if (!isset($_GET['site'])) {
require_once($defaultSite . ".php");
return;
}
$site = basename($_GET['site']);
include_once('path/to/content/sites/folder' . $site . '.php');
}
Noch viel besser wäre allerdings ein Routingsystem, welches den Seitenname nicht an eine Datei, sondern eine Funktion knüpft. Zunächst werden alle Anfragen auf einen Dispatcher (index.php o.ä.) umgeleitet. Dort werden alle erwarteten URLs mit ihren zugehörigen Handlern verknüpft und gespeichert. Wenn die Request-URL auf einen bestimmten registrierten Eintrag passt, wird der dazugehörige Handler ausgeführt. Eine einfache Umsetzung wird in diesem Artikel gezeigt, andernfalls gibt es fertige (und ausgereiftere) Implementationen in diversen PHP-Frameworks (Laravel, Symfony, Yii, etc.).
Genauso solltest du restriktiver in getFormAction handeln. Schau erst, ob site einen validen, beabsichtigten Wert beinhaltet, bevor du ihn an eine Adresse hängst. Insofern könnte man sich da auch gleich eine Hilfsfunktion schreiben:
function getContentSiteName($defaultSite) {
if (!isset($_GET['site'])) {
return $defaultSite;
}
$site = basename($_GET['site']);
$sitePath = 'path/to/content/sites/folder' . $site . '.php';
if (file_exists($sitePath)) {
return $site;
}
return $defaultSite;
}
function getFormAction() {
$siteName = getContentSiteName('');
if (!$siteName) {
return $_SERVER['PHP_SELF'];
}
return '?site=' . $siteName;
}
Wenn es einen Querystring gibt, genügt diese relative Adresse. Andernfalls wird der aktuelle Dateiname ausgegeben, damit das action-Attribut, in welches der Wert später hineingeschrieben wird, nicht leer ist.
Hinsichtlich der Funktionen isFormValueChecked und isFormValueSelected kann ich nicht so viel sagen, da sich mir der Kontext nicht erschließt, in dem sie später eingesetzt werden.
Bei der Angabe des Connectionstrings würde sich, aufgrund der vielen Stringkonkatenationen eine Interpolation mal mehr lohnen:
$con = new PDO("mysql:host={$server};dbname={$schema};charset=utf8", $user, $password);
Und insgesamt könnte man alle Datenbankoperationen gut in einer Klasse kapseln:
class DatabaseHandler {
public function __construct() {
/* create PDO connection here ... */
$this->connection = $connection;
}
public function makeStatement($query, $params = null) {
try {
$stmt = $this->connection->prepare($query);
$stmt->execute($params);
return $stmt;
}
catch(Exception $ex) {
$this->printException($ex);
return null;
}
}
/* etc. */
}
// usage example:
$dbHandler = new DatabaseHandler();
$result = $dbHandler->makeStatement('select something from somewhere');
$otherResult = $dbHandler->makeStatement('select somethingElse from somewhereElse');
Auf eine globale Variable $con kann man somit verzichten. Die Rolle übernimmt das connection-Feld, welches nur in dem Kontext bekannt ist, in welchem es auch benötigt wird. Wenn du eine der Funktionen aufrufen möchtest, legst du dir eine Instanz der Klasse an und rufst über diese die jeweilige Funktion auf.
Achte im Übrigen darauf, konsistent bei der Werterückgabe zu sein. Wenn eine Funktion in einem ihrer Programmzweige einen Wert zurückgibt, dann sollte sie das letztendlich auch in allen anderen möglichen Programmzweigen tun.
Im obigen Fall (makeStatement) gibt es dahingehend zwei Möglichkeiten. Entweder du gibst bei Misserfolg null (oder zumindest einen anderen falsy Wert) zurück oder du verzichtest auf das try-catch und lässt eine Exception stattdessen nach oben eskalieren, um sie dann dort entsprechend zu behandeln.
Wenn du die erste Option wählst, könnte ein Aufrufer wie makeTable demzufolge so vorgehen:
public function makeTable($query, $arrV = null) {
$stmt = $this->makeStatement($query, $arrV);
if ($stmt) {
/* print table ... */
}
else {
/* fallback? */
}
Andernfalls bleibt es bei einem try-catch-Konstrukt.
Zu guter Letzt wäre es noch gut, die Namen einiger Bezeichner nochmals zu überdenken. Beispielsweise sollte $r doch eine Spalte repräsentieren und daher besser $column heißen. Bei $arrV oder $array wiederum ist von außen unklar, wozu sie dienen sollen. Ihr Zweck wird erst ersichtlich, wenn man sich den konkreten Code der jeweiligen Funktionen anschaut.
