• Fortress ??

Ein Beispiel für "schlechten" phpNuke code

Die Datei block-modules.php aus phpNuke 6.5

Nachfolgend eine kleine Demonstration bzw. Analyse einer typischen phpNuke Datei.
Es ist der gesamte Quellcode gelistet, mit Beschreibung, was in den wichtigsten Zeilen passiert.
Ausserdem, dahinter die jeweiligen Fehler und Unzulänglichkeiten.

Zeile	Code	was passiert	Fehler / Bemerkungen
15	if (eregi("block-Modules.php", $_SERVER['PHP_SELF'])) {	Prüfen ob der Name des Blocks innerhalb der server-Variablen PHP_SELF vorkommt.	eregi() ist langsamer als z.B. substr() und Gross/Kleinschreibung kann ja ruhig beachtet werden unflexibel, beim Umbenennen des Blocks
16	Header("Location: index.php");	Falls ja, wurde die Datei direkt gestartet und es soll auf die Startseite umgeleitet werden.	Fehler: die Blockdatei liegt im Ordner blocks, dort gibt es keine index.php, also kommt 404er Fehler
17	die();
18	}
19
20	global $prefix, $db, $admin;	Versch. Globale Variablen verfügbar machen	- Anstatt $admin sollte $_COOKIE["admin"] verwendet werden. Das ist eindeutiger und die Variable darf nur ein Cookie sein!
21
22	$ThemeSel = get_theme();	Das aktuell eingestellte Theme abfragen	In der Funktion get_theme() wird mit is_user() geprüft ob der Besucher eingeloggt ist. Die erste Datenbankabfrage. Komplett Unnötig! Die Blöcke werden in der header.php oder footer.php includet. In der header.php wird bereits das Theme abgefragt und die Userauthentifizierung durchgeführt. Die jeweiligen Werte können in einer globalen Variablen oder in den jeweiligen Funktionen als statische Variable zwischengespeichert werden.
23	if (file_exists("themes/$ThemeSel/module.php")) {	Prüfen ob in dem ermittelten theme-Ordner eine Datei module.php vorhanden ist...	In den ganzen bei phpNuke 6.5 mitgelieferten themes ist diese Datei nur 1x vorhanden. Darin wird nur eine Variable $default_module deklariert, sonst nichts.
24	include("themes/$ThemeSel/module.php");	... und einbinden
25	if (is_active("$default_module") AND file_exists("modules/$default_module/index.php")) {	Prüfen, ob dieses Defaultmodul aktiv ist und dort eine index.php existiert	Die 2te Datenbankabfrage in der Funktion is_active(). Wenn vorher getestet wird, ob die Datei vorhanden ist, kann man sich die Datenbankabfrage sparen.
26	$def_module = $default_module;	Variable $def_module mit der Variablen $default_module initialisieren
27	} else {	ansonsten
28	$def_module = "";	Variable $def_module auf Leerstring setzen
29	}
30	}
31
32	$sql = "SELECT main_module FROM ".$prefix."_main";	Das Startseitenmodul aus der Datenbank auslesen	Die 3te Datenbankabfrage Es wird nicht geprüft, ob die Abfrage erfolgreich war. Ist kein Datensatz in der Tabelle vorhanden, dann ist $row[main_module] ein Leerstring.
33	$result = $db->sql_query($sql);
34	$row = $db->sql_fetchrow($result);
35	$main_module = $row[main_module];	Variable $main_module mit dem Datenbankwert belegen.	- SyntaxFehler! Bei $row[main_module] muss main_module in Anführungsstrichen stehen, ansonsten wird main_module als fehlende Konstante interpretiert und verursacht eine Warning-Meldung - Warum das Ganze überhaupt? Mit der Array-Variablen $row["main_module"] kann genauso weitergearbeitet werden....
36
37	/* If the module doesn't exist, it will be removed from the database automaticaly /*
38
39	$sql = "SELECT title FROM ".$prefix."_modules";	Alle Module aus der Datenbank abfragen.	Die 4te Datenbankabfrage
40	$result = $db->sql_query($sql);	Alle Module aus der Datenbank abfragen.	Die 4te Datenbankabfrage
41	while ($row = $db->sql_fetchrow($result)) {	In Schleife alle Modulnamen auslesen
42	$title = $row[title];	Siehe Zeile 35	Der gleiche Fehler wie in Zeile 35, in der Schleife also gleich vielfach...
43	$a = 0;
44	$handle=opendir('modules');	Den Modul-Ordner zum auslesen öffnen	Warum in der Schleife? Siehe Zeile 50
45	while ($file = readdir($handle)) {	In Schleife alle Unterordner einlesen	Warum die Schleife überhaupt? Ein einfaches file_exists("modules/".$row[title]); würde genügen
46	if ($file == $title) {	Und den Ordnernamen mit dem Modulnamen aus der Datenbank vergleichen
47	$a = 1;	Wenn übereinstimmt....	In diesem Moment kann die Schleife mit break beendet werden und braucht nicht weiter durchlaufen werden.
48	}
49	}
50	closedir($handle);	Den Modulordner wieder schliessen	Warum? Beim nächsten Schleifendurchlauf wird er wieder geöffnet! Das kann nach der Datenbankschleife geschehen. Dazwischen genügt rewinddir() um das handle zurückzusetzen.
51	if ($a == 0) {	Ist das Modul nicht mehr vorhanden, wird es aus der Tabelle gelöscht.	Das ist der eigentliche Sinn dieses Codeabschnittes. Er wird bei jedem Seitenaufruf von jedem Besucher ausgeführt. Wie oft werden Module vom Admin gelöscht? Also komplett überflüssig, ein Aufruf des Modul-Adminmenüs genügt, weil das dort alles ebenfalls passiert!
52	$db->sql_query("DELETE FROM ".$prefix."_modules WHERE title='$title'");
53	}
54	}
55
56	/* Now we make the Modules block with the correspondent links /*
57
58	$content .= "<strong><big>·</big></strong> <a href=\"index.php\">"._HOME."</a><br>\n";		Fehler! Die Variable $content ist noch nicht initialisiert, also ist der Punkt vor dem = nicht zulässig. Wieder ein Warning.
59	$sql = "SELECT title, custom_title, view FROM ".$prefix."_modules WHERE active='1' AND title!='$def_module' AND inmenu='1' ORDER BY custom_title ASC";	Abfrage die alle aktiven Module aus der Tabelle zurückgibt die im Menü angezeigt werden sollen. Ausser dem Modul, welches evtl. in dem Theme (Zeile 24-28) als $def_module deklariert ist.	Fehler! Der Vergleichsoperator != ist kein zulässiger SQL-Operator. Wird zwar in MySql akzeptiert, in anderen Datenbanksystemen aber nicht! Fehler! Das Feld custom_title kann leer sein. Eine Sortierung nach diesem Feld ist also unsinnig. Wenn weiter oben, sowieso geprüft wird ob der User eingeloggt ist, warum schliesst man (wenn es kein Admin ist) nicht auch die Module von der Abfrage aus, welche für Anonyme nicht zu sehen sind?
60	$result = $db->sql_query($sql);
61	while ($row = $db->sql_fetchrow($result)) {	Schleife, die die Modulnamen aus der Tabelle ausliest
62	$m_title = $row[title];		Gleiche Fehler wie in Zeile 35.
63	$custom_title = $row[custom_title];
64	$view = $row[view];
65	$m_title2 = ereg_replace("_", " ", $m_title);		Die Verwendung von str_replace ist schneller und soll lt. Php-Manual bevorzugt werden.
66	if ($custom_title != "") {	Wenn im Feld custom_title was drin steht..
67	$m_title2 = $custom_title;	Diesen Wert anstatt des Modulnamens verwenden
68	}
69	if ($m_title != $main_module) {	Nur wenn der Modulname ungleich dem im Adminmenü eingestellten Startseitenmodul ist.	Überflüssig, das kann direkt in die SQL-Abfrage mit rein, spart einen Schleifendurchlauf. Hier fällt noch die doppelte Abfrage nach einem Standardmodul oder Startseitenmodul auf. Ist in dem Theme die Variable $def_module gesetzt, wird dieses Modul nicht angezeigt und ausserdem das Modul welches Startseitenmodul eingestellt ist. Unterscheiden sich diese beiden Werte, fehlen im Block gleich 2 Links? Was macht diese theme-Variable für einen Sinn?
70	if ((is_admin($admin) AND $view == 2) OR $view != 2) {	Wenn Benutzer als Admin eingeloggt, oder Modul für "nicht-Admins" zu sehen..	Der Performance-Hammer! In der Funktion wird jedesmal der Admincookie decodiert und eine Datenbankabfrage ausgeführt. Diese Funktion wird bei jedem Schleifendurchlauf ausgeführt. Macht bei 15 aktivierten Modulen, 15 unnötige Datenbankabfragen
71	$content .= "<strong><big>·</big></strong> <a href=\"modules.php?name=$m_title\">$m_title2</a><br>\n";	Die Ausgabe des Menüs für ALLE User ist hiermit abgeschlossen
72	}
73	}
74	}
75
76	/* If you're Admin you and only you can see Inactive modules and test it /*
77	/* If you copied a new module is the /modules/ directory, it will be added to the database /*
78
79	if (is_admin($admin)) {	Erneute Prüfung ob als Admin eingeloggt...	Wieder unnötige Datenbankabfrage! Das ist bereits x mal oben geschehen, das Ergebnis der Funktion is_admin() kann man in einer Variablen zwischenspeichern.
80	$handle=opendir('modules');	Erneutes Öffnen des Modulordners	Unnötig! Das Ganze ist bereits x Mal in den Zeilen 44-50 geschehen.
81	while ($file = readdir($handle)) {	Wieder die Schleife...
82	if ( (!ereg("[.]",$file)) ) {
83	$modlist .= "$file ";	Einen String mit allen Modulnamen getrennt durch Komma erstellen
84	}
85	}
86	closedir($handle);
87	$modlist = explode(" ", $modlist);	Den String in Array umwandeln	Unnötig! Man könnte direkt in der Schleife dieses Array mit Werten füllen.
88	sort($modlist);		Unnötig! Warum sortieren, wenn keine HTML-Ausgabe erfolgt?
89	for ($i=0; $i < sizeof($modlist); $i++) {	Schleife über das gesamte Array	Foreach ist einfacher!
90	if($modlist[$i] != "") {
91	$sql = "SELECT mid FROM ".$prefix."_modules WHERE title='$modlist[$i]'";
92	$result = $db->sql_query($sql);	Je eine Datenbankabfrage, ob der Modulname in der Tabelle vorhanden ist	Bei z.B. 15 installierten Modulen, wieder 15 Datenbankabfragen!
93	$row = $db->sql_fetchrow($result);
94	$mid = $row[mid];		Der gleiche Fehler wie in Zeile 35, in der Schleife also gleich vielfach...
95	if ($mid == "") {	Wenn Modul nicht in der Tabelle eingetragen...
96	$db->sql_query("INSERT INTO ".$prefix."_modules VALUES (NULL, '$modlist[$i]', '$modlist[$i]', '0', '0', '1')");	Das Modul in der Tabelle eintragen	Die insert Anweisung ist unvollständig und Fehlerbehaftet! Die Feldnamen sollten mit angegeben werden, dies vereinfacht eine spätere Änderung der Tabellenstruktur. Auch die Feldtypen werden nicht beachtet. MySql toleriert dies, andere Datenbanksysteme nicht. 0 ist nicht gleich '0' Die autoincrement Eigenschaft des Feldes mid wird nicht beachtet, die 0 also überflüssig.
97	}
98	}
99	}
100	$content .= "<br><center><b>"._INVISIBLEMODULES."</b><br>";
101	$content .= "<font class=\"tiny\">"._ACTIVEBUTNOTSEE."</font></center><br>";
102	$sql = "SELECT title, custom_title FROM ".$prefix."_modules WHERE active='1' AND inmenu='0' ORDER BY title ASC";	Abfrage die alle aktiven Module aus der Tabelle zurückgibt die nicht im Menü angezeigt werden sollen.
103	$result = $db->sql_query($sql);
104	while ($row = $db->sql_fetchrow($result)) {
105	$mn_title = $row[title];		Der gleiche Fehler wie in Zeile 35, in der Schleife also gleich vielfach...
106	$custom_title = $row[custom_title];
107	$mn_title2 = ereg_replace("_", " ", $mn_title);	Unterstrich in custom_title in Leerzeichen konvertieren	Wieder ereg_replace() statt str_replace()
108	if ($custom_title != "") {
109	$mn_title2 = $custom_title;
110	}
111	if ($mn_title2 != "") {
112	$content .= "<strong><big>·</big></strong> <a href=\"modules.php?name=$mn_title\">$mn_title2</a><br>\n";
113	$dummy = 1;
114	} else {
115	$a = 1;
116	}
117	}
118	if ($a == 1 AND $dummy != 1) {	Keine Ahnung was der $dummy soll ??	Die Variable $dummy kann evtl. nicht vorhanden sein, ergibt Warning. Richtige Überprüfung wäre: empty($dummy). Ausserdem kann die Variable $a bereits in Zeile 47 auf 1 gesetzt sein. Sie wird nirgends auf 0 zurückgesetzt. Die Beedingung ($a==1) also immer wahr.
119	$content .= "<strong><big>·</big></strong> <i>"._NONE."</i><br>\n";
120	}
121	$content .= "<br><center><b>"._NOACTIVEMODULES."</b><br>";
122	$content .= "<font class=\"tiny\">"._FORADMINTESTS."</font></center><br>";
123	$sql = "SELECT title, custom_title FROM ".$prefix."_modules WHERE active='0' ORDER BY title ASC";	Ab hier nochmal das Gleiche wie in den Zeilen 102 - 120, mit dem Unterschied, dass die inaktiven Module aufgelistet werden.	.... Mit allen dort eingebauten Fehlern....
124	$result = $db->sql_query($sql);
125	while ($row = $db->sql_fetchrow($result)) {
126	$mn_title = $row[title];
127	$custom_title = $row[custom_title];
128	$mn_title2 = ereg_replace("_", " ", $mn_title);
129	if ($custom_title != "") {
130	$mn_title2 = $custom_title;
131	}
132	if ($mn_title2 != "") {
133	$content .= "<strong><big>·</big></strong> <a href=\"modules.php?name=$mn_title\">$mn_title2</a><br>\n";
134	$dummy = 1;
135	} else {
136	$a = 1;
137	}
138	}
139	if ($a == 1 AND $dummy != 1) {
140	$content .= "<strong><big>·</big></strong> <i>"._NONE."</i><br>\n";
141	}
142	}

Zusammenfassung

Blockstruktur:

Der Block gliedert sich in 6 Funktionsteile.

- Defaultmodul und Theme-Standardmodul ermitteln
- nichtexistierende Module aus der Tabelle löschen
- Anzeigen der Modul-Links für alle aktiven Module
- Neu installierte Module in die Tabelle aufnehmen
- Anzeigen der Modul-Links, die nur für Admins im Menü stehen
- Anzeigen der Modul-Links, der deaktivierten Module, nur für Admins

Die Ausgangsbasis:

- Bei phpNuke 6.5 werden ~25 Module mitgeliefert, bzw installiert.
- Standardmässig sind 17 Module aktiviert und für alle Besucher zu sehen.
- Die restlichen 8 Module sind nicht aktiv und nur für Admins zu sehen.
- Als Admin eingeloggt

Anzahl der Datenbankabfragen:

1.	Ermitteln des Themes, in Funktion get_theme() bzw. is_user()	1
	Ermitteln des Startseitenmoduls, in Funktion is_active()	1
2.	Ermitteln aller (in DB) installierten Module	1
	Zuzüglich der evtuellen Löschabfragen	?
3.	Haupt-Abfrage zur Anzeige der aktiven Module	1
	In Schleife, Prüfung ob Admin eingeloggt, in Funktion is_admin() je 1x = 17 evtl. abzüglich dem defaultModul aus dem Theme	16 (17)
4.	weitere Prüfung, ob Admin eingeloggt	1
	Schleife durch den Modulordner, mit je 1x Abfrage ob Modul in Tabelle vorhanden ist	25
	Zuzüglich der evtuellen Anfügeabfragen	?
5.	Haupt-Abfrage zur Anzeige der "versteckten" Module	1
6.	Haupt-Abfrage zur Anzeige der inaktiven Module	1
	Summe:	48

Ist der Besucher nicht als Admin eingeloggt fallen 2 Abfragen weg (5 u. 6)

Fehlermeldungen:

Der Block verursacht rund 100 PHP-Meldungen. Hier die Liste

Ziel:

Es muss möglich sein, den Block, mit nur einer Datenbankabfrage darzustellen.

Lösungen:

- Vor Aufruf aller Blöcke wird in der header.php bereits geprüft ob ein User eingeloggt wird ist, und das entsprechende Theme ermittelt. Das Ergebnis kann man dort in einer Variablen ablegen und im Block abfragen.
- In der Funktion blocks() (mainfile.php), also vor Aufruf aller Blöcke, wird bereits geprüft ob ein Admin eingeloggt ist. . Das Ergebnis kann man dort in einer Variablen ablegen und im Block abfragen.
- Die 4 verschiedenen Abfragen auf die _modules Tabelle kann man in eine Einzige zusammenfassen. Mit einem join auf die _main Tabelle kann man gleichzeitig das Startseitenmodul ausklammern. In der darauffolgenden Schleife werden alle benötigten Felder, aller Datensätze, je nach Bedingung, in 3 verschiedene Arrays gelesen. Diese Arrays können dann zur Anzeige der Daten verwendet werden.
- In der Schleife wird gleich (file_exists) geprüft, ob das Modul noch im System vorhanden ist. Wenn nein, wird es hier gleich aus der Tabelle gelöscht.

Ergebnis:

Eine Datenbankabfrage anstatt 48 !!