Merkwürdiges am "Admin Register Mod"

[cYbercOsmOnauT]

Schönen guten Tag,

eigentlich bat mich ein Kunde vor einiger Zeit darum, dieses einzubauen. Dabei sind mir einige Punkte aufgefallen im admin_user_register.php, die ich gerne hier auflisten möchte.

1. Unnötige Abfrage der Board Konfiguration

• An zwei Stellen des Scriptes (Zeile 32ff. und 85ff.) werden Konfigurationen des Boards per SQL-Queries abgerufen und die $board_config gesetzt. Dies ist unnötig, da ziemlich am Anfang des Scriptes das pagestart.php included wird, welches die common.php (und somit unter anderem die komplette $board_config - Zeile 210ff. in der common.php) läd. Zumal im Script selber auch Konfigurationseinstellungen benutzt werden, die gar nicht via Query im Script abgefragt wurden. Somit verliert sich der Sinn dieser 2 Queries noch mehr.

2. Unnötig lange If-Bedingungen und Setzen von Variablen, die gar nicht benötigt werden

• An drei Stellen (Zeilen 195, 202 & 211) kommen If-Bedingungen vor, die eine Variable setzen, die später gar nicht verwendet wird. Ein Beispiel: if ( !($result = $db->sql_query($sql, BEGIN_TRANSACTION)) ) wo doch völlig if ( !($db->sql_query($sql, BEGIN_TRANSACTION)) ) genügt.

3. Includen von Scripten, die nicht benötigt werden.

• In Zeile 50 und 51 werden bbcode.php und functions_post.php included. Beim ersten dachte ich, es würde für die Signatur benötigt. Kurz darauf stellte ich fest, dass beim Admin Register Mod gar keine Signatur gesetzt wird. Das Includen des zweiten Scriptes ist mir total schleierhaft. Ich kann mir nicht im Geringsten vorstellen, wozu die Funktionen, die in functions_post.php drin sind, bei einer Registrierung dienen sollen. Fakt ist, beide Dateien werden included ohne das eine Nutzung statt findet.

4. Fehlerhaftes INSERT-Statement

• Im ersten INSERT (beim USERS_TABLE Scriptzeile 193ff.) wird die Zelle user_actkey (Aktivierungsschlüssel bei EMail-Validation) mit dem Wert 'user_actkey' gesetzt. Dies ist in mehrfacher Hinsicht nutzlos. Der User wird sowieso sofort als 'aktiviert' gespeichert und benötigt somit keinen Aktivierungsschlüssel. Zudem besteht dieser Schlüssel normalerweise aus einem MD5 Hash, der ein 32 Byte langer, alphanumerischer String ist. Zu guter Letzt wird diese Zelle überschrieben, sobald der User sich ein neues Passwort schicken lässt, oder seine EMailadresse ändert.

5. Unnötige Überprüfung

• In Zeile 111 bis 119 wird überprüft, ob der User eingeloggt ist. Dies ist eine normale Schutzfunktion im usercp_register.php, die verhindert, dass eingeloggte Benutzer auf "Registrieren" gehen können. In einem Admin-Script hat dieser Part wenig Sinn, der Admin wird kaum sich selber registrieren.

6. Abfrage des Modus, wo nur eines bestehen kann

• An 5 Stellen des Scriptes wird geprüft, ob $mode den Wert 'register' hat. Dieses Script ist ausschliesslich für die Registrierung gedacht, somit kann der andere Modus 'editprofile' gar nicht vorkommen. Diese If-Anweisung könnte entfernt werden, weil sie immer den Wert TRUE ergibt.

7. Umständliche Berechnung der neuen user_id

• Ab Zeile 178 wird erst per MAX(user_id) AS total die höchste user_id ermittelt und dann weiter unten mit 1 addiert, um die user_id zu erhalten, die der neue Benutzer bekommen wird. Dies ist auch innerhalb des Queries möglich, indem man einfach MAX(user_id)+1 AS total schreibt.

Eigentlich wollte ich diese Punkte in Ruhe und privat via ICQ/PN besprechen. Christian gab mir dafür sogar extra die ICQ-Nummer von Karsten. Leider war der Herr Ude scheinbar zu beschäftigt um nett gemeinte Ratschläge anzunehmen. Deswegen musste ich leider diesen "öffentlichen" Weg wählen.

Klar, das Script ist von 2004, funktioniert und lässt Admins neue User registrieren. Jedoch ist es im Punkto Performance und sinnvoller Kürzung noch erheblich verbesserungswürdig. Mein Posting soll auch nicht als "Rumnörgeln" dienen, sondern Defizite aufzeigen und die Ausbesserung anregen.

Viele Grüße,
Tekin

[Christian_N]

Frage, weshalb tust dann nicht einfach ein neue schreiben für die allgemeinheit mit die punkte die überflüssig sind weglässt usw. damit es Punkto Performance besser ist, so das die allgemeinheit was von hat?

[cYbercOsmOnauT]

Gerne. Hatte ich eh vor, sobald ich Zeit dafür finde. Derzeit bin ich leider voll ausgelastet mit Projekten bei denen ich mitwirke. Aber ich denke, da wird in naher Zukunft ein Mod von mir erscheinen.

Eigentlich fing alles damit an, dass ich beim INSERT diesen unsinnigen Eintrag mit dem Aktivierungsschlüssel sah. Danach hab ich mir das Script ein wenig genauer angesehen und dachte mir, es kann nicht schaden, wenn ich Oxpus meine Entdeckungen mitteile.

[oxpus]

Zu den einzelnen Punkten:
1: Beide Abfragen sind notwendig, da ansonsten die Formate des Users und nicht die des Boards verwendet werden, welche aber hier nötig sind!
Man beachte, daß in der Funktion init_userprefs(); eben genau diese Variablen mit den Userangaben überschrieben werden, was bei der Registrierung wirklich hinderlich sein kann...

2: Nicht ganz korrekt. Dann würde nur bei einer nicht ansprechbaren Datenbank ein Fehler zurückkommen und nicht auch bei einem unmöglichen Ergebnis, weil die Abfragen an sich falsch oder unvollständig sind.

3: Die sind ursprünglich für einen MOD rein gekommen, dieser ist jedoch nicht überall installiert. Die beiden includes habe ich aber dennoch beibehalten, da sie den MOD besser kompatibel zu weiteren Register-MODs macht. Viele vergessen dann nämlich, daß diese beiden Zeilen teilweise zwingend benötigt werden und stören auch nicht wirklich den Ablauf des Scripts (weder zeitlich noch in der Performance). Ja, schon gut, ich werde diese beiden Zeilen löschen...

4: Das Feld muss gesetzt werden, darf also nicht NULL sein. Ein hier "scheinbar" sinnloser Text erfüllt damit seinen Zweck und auch in älteren phpBB-Versionen (ja, die gibts leider immer noch) ist das auch so vorgeschrieben. Damit bleibt der MOD kompatibler. Nun, ich entferne das, damit eben der MOD nur noch ab 2.0.6 läuft...

5: Öhm, mit falscher Anwendung dieses Scripts kann sich jeder neue User zum Admin machen. Daher diese reine Vorsichtsmaßnahme. Werde ich aber aufgrund Punkt 6 etwas entschärfen.

6: Das ist wahrlich falsch, da zudem $mode auch überhaupt nicht sicher übernommen und gebraucht wird. Ich entferne das mal aus dem Script.

7: Funktioniert aber dann nicht mit jeder Datenbank, daher erst + 1 NACH der Abfrage.

Und "Herr Ude" ist nicht zu beschäftigt, um per ICQ zu antworten, aber es sind einfach zu viele Anfragen auf diesem Wege, so daß ich dann wohl eher das Forum ganz abschalten könnte, oder? Daher gibts es keine Ausnahme, wie auch in meiner Signatur beschrieben und nur der öffentliche Weg über das Forum.
Den entsprechenden Unterton in Deinem Post habe ich dann auch mal "überlesen".

Den MOD verfasse ich nun auch so, wie es "Herr Tekin" für richtig hält um dem Script noch weitere "1000stel Millisekunden" an Speed herauszukitzeln.

Danke für das öffentliche Gespräch

[Markus Wandel]

Moin,

ich bekomme folgenden Fehler wenn ich den MOD installiert habe und ins AdminCP gehen will:

Parse error: parse error, unexpected '{' in ***/admin/admin_user_register.php on line 45

Ich habe das File bereits zweimal ins Verzeichnis kopiert.

Gruß,
Markus

[oxpus]

Ich habe es nochmal aktualisiert.
Der Fehler sollte nun weg sein.
Einfach die Datei mit der neuen Version ersetzen...

[Markus Wandel]

Super !

Jetzt funktioniert es.

Danke !

[cYbercOsmOnauT]

Punkt 1: Macht Sinn. Hatte ich übersehen. Jedoch selbst diese zwei SQL-Queries lassen sich zu einem zusammenfügen.

Code: Alles auswählen

$sql = "SELECT config_value FROM " . CONFIG_TABLE . "
           WHERE config_name IN ('board_timezone', 'default_dateformat')";
if ( !($result = $db->sql_query($sql)) )
{
	message_die(GENERAL_ERROR, 'Could not obtain config values', '', __LINE__, __FILE__, $sql);
}
while ( $row = $db->sql_fetchrow($result) )
{
	$board_config[$row[config_name]] = $row[config_value];
}

Das spart einen weiteren Query.

Punkt 2: Jeder wie er/sie es mag. Ich würde trotzdem die kürzere Version wählen und hatte damit bis dato auch nie Probleme.

Punkt 4: Stimmt, bei extrem alten Versionen von phpBB fehlt im Db-Schema der Defaultwert NULL. Doch dann reicht es, wenn man user_actkey einfach mit '' setzt. So wird es selbst im usercp_activate.php von phpbb2.0.1 gemacht.

Punkt 5: Deine Aussage ist nicht nachvollziehbar. Die einzigen User, die an dieses Script heran kommen, sind Administratoren. pagestart.php wird included und dieser prüft sofort, ob der Nutzer des Scriptes überhaupt eingeloggt und Administrator ist.

Punkt 7: Ist ein Argument.

Eines möchte ich noch sagen: Du bist beileibe nicht der einzige Mensch auf Erden, der mit "Supportanfragen" zugemüllt wird. Ich habe (und wirke) in all meinen Jahren schon in vielen großen Communities Als TechOP, Supporter, Admin und dergleichen gewirkt. Manch einer lernt zu differenzieren und weiß worauf er reagieren kann und was ungelesen in der Ablage A landet. Bestes Beispiel hier wäre der Christian. Auf meinen ersten Kontakt damals hat er sehr freundlich reagiert und niemals Arroganz durchscheinen lassen. Niemand ist so wichtig, wie er vielleicht zu sein glaubt. Wenn Deine Altersangabe stimmt, sind wir beide so ziemlich im gleichen Altersbereich. Also wozu soll ich die Samthandschuhe auspacken? Ich denke, ein reifer Mann ist alt genug um konstruktive Kritik anzunehmen.

Thema Performance: Klar sind es hier nur ein paar Millisekunden und da nur ein paar Millisekunden. Jedoch darf man nie vergessen, dass auf dem Server nicht nur ein Script gleichzeitig läuft. Alle Prozesse zusammen ergeben, wenn sie nicht oder wenig optimiert sind, sehr viel unnötige Last. Welcher Benutzer kann schon behaupten einen dedizierten Rootserver sein eigen zu nennen? Die meisten haben ihre Domains auf Servern, die sie sich mit hunderten oder gar tausend anderen teilen. Auch Besitzer von dedizierten Servern bekommen Probleme mit der Performance, wenn ihre Community "richtig groß" (einige 10.000 Beiträge und mehr) wird. Bestes Negativbeispiel hierbei das Intro&Portal Mod von AWSW, welches z.B. bei Markus zu Queries im Größenbereich von mehreren hundert Kilobyte pro Anweisung geführt hat und seinen Server in die Knie zwang.

Gruß,
Tekin

P.S.: Du sprachst Deine Signatur an. Diese habe ich erst hier nach meiner Anmeldung zu sehen bekommen.

[oxpus]

Eines möchte ich noch sagen: Du bist beileibe nicht der einzige Mensch auf Erden, der mit "Supportanfragen" zugemüllt wird. Ich habe (und wirke) in all meinen Jahren schon in vielen großen Communities Als TechOP, Supporter, Admin und dergleichen gewirkt. Manch einer lernt zu differenzieren und weiß worauf er reagieren kann und was ungelesen in der Ablage A landet. Bestes Beispiel hier wäre der Christian. Auf meinen ersten Kontakt damals hat er sehr freundlich reagiert und niemals Arroganz durchscheinen lassen. Niemand ist so wichtig, wie er vielleicht zu sein glaubt. Wenn Deine Altersangabe stimmt, sind wir beide so ziemlich im gleichen Altersbereich. Also wozu soll ich die Samthandschuhe auspacken? Ich denke, ein reifer Mann ist alt genug um konstruktive Kritik anzunehmen.

Oha, hier übertreibt aber jemand massiv, denn

A. Ich habe nie behauptet, daß andere nicht ebenfalls zugemüllt werden
B. und ich habe Nachrichten nicht eingetragener Absender abgeschaltet, um nicht alle 2 Sekunden angepingt zu werden (Nimmt leider warum auch immer ständig zu!). Dazu:
C: Warum gibt es denn überhaupt Foren?
D. Ich handhabe die Kontakte im Messanger eben anders, als Christian und
E. bin dennoch nicht unfreundlich,
F. lasse mich aber nicht selbiges vorwerfen, nur weil ich auf Support/MOD-Fragen im Forum und nicht per Messenger behandeln will.
G. Ich halte mich nicht für wichtig
H. Konstruktive Kritik ist auch kein Thema, aber vorher nachschauen, warum etwas gemacht wird und nicht "lospoltern" (bezieht sich dann auch mehr auf Deine Ausführungen nach den einzelnen Punkten) wäre auch nicht verkehrt, oder?

Noch ergänzend zu Deinen Antworten:

1: Okay, wäre ein Argument. Will ich gerne noch im MOD ändern.
2: Klar, ist jedem selber überlassen, aber die usercp_register.php ist exakt genau so aufgebaut.
5: Ich wäre wohl leicht verrückt, wenn ich hier Ansätze aufzeigen würde, oder? Und ich bezweifel auch immer noch, daß die Absicherung des ACP im phpBB 100 ausreichend ist.
Performance: Wie oft würde ein Admin dieses Script nun aufrufen? 1000 mal die Sekunde? Aber Spaß beiseite: Was man sparen kann ist nie verkehrt, da bin ich klar bei Dir.
Und zum Hintergrund meines "Mini-Mods": Die usercp_register.php war hier, wie man eigentlich unschwer erkennen kann, oft Pate der einzelnen Code-Blöcke, daher auch kein Unterschied zu dieser Daten an der ein und anderen Stelle.
Klar, keine Glanzleistung, aber das sollte es auch nicht sein, sondern nur funktionieren.
Dazu halte ich es auch für wichtig, gerade bei der Registrierung von Usern, bei der Entscheidendes schief gehen kann (!), besonders viele Sicherheiten einzubauen, was in der usercp_register.php ja auch getan wurde.

Aber jetzt genug der Diskussion.
Ich danke Dir für die Tipps, dieses winzige Script mal zu "renovieren" und baue auch noch die Abfrage der Config um.
Alles andere hake ich jetzt mal ab und möchte nur zuletzt auf das Thema Messenger noch anmerken, daß ich diesen persönlich nicht gerade mag und daher meist nur auf "Freunde" reagiere.
Jeder handhabt das eben anders.
Und wenn Du mich deswegen als unfreundlich, arrogant oder gar intolerant abstempelst... Bitte schön...

[cback]

Zu 1 muss ich auch noch Senf dazugeben:

man kann $board_config trotzdem nutzen! Die Konfiguration des Users selbst steckt doch in $userdata drin. $board_config ergibt den standardwert der beim Board definiert ist, denn die Allgemeine Forenkonfiguration greift auch von der $board_config ab ohne die config Table neu einzulesen. Sollte also möglich sein, einfach mal ausprobieren.

[oxpus]

Nein, Cback, eben genau das nicht.
Die Werte in $board_config werden IMMER mit den Userdaten überklatscht, sofern die Userinitialisierung aufgerufen wurde (was ja bei den phpBB-Scripten die Regel ist).
Daher würden hier Zeitformat und Zeitzone des gerade aktiven Admins angezeigt und nicht die Formate des Boards.
Schau Dir die Funktion init_userprefs(); in der functions.php mal an, dann siehst Du es.
Dort werden z. B. auch alle Sprachdateien diverser MODs eingebunden.

Und genau daher braucht man auch keine Sprach-Dateien für Gäste explizit auswählen, denn diese werden mit der Funktion immer sauber eingebunden. War auch mein Fehler bei meinen ersten MODs, bis ich hierzu von der phpBB.com-Group eindeutig mit dem Pfahl vom Winkzaun bearbeitet wurde...
Ebenso auch der Style wodurch dann eben jeder Gast auch den im ACP eingestellten Standard-Style zu Gesicht bekommt und nicht "irgendeinen" oder den "ersten" in der Datenbank.

$userdata als Gegenstück wäre dann wirklich "fehlerhafter" Gerade beim Gast-Account!

[AmigaLink]

Die Werte in $board_config werden IMMER mit den Userdaten überklatscht, sofern die Userinitialisierung aufgerufen wurde (was ja bei den phpBB-Scripten die Regel ist).
Daher würden hier Zeitformat und Zeitzone des gerade aktiven Admins angezeigt und nicht die Formate des Boards.

Dem kann ich zustimmen. Da bin ich nämlich auch bereits drauf reingefallen.

[cback]

Ah ja, das is ja blöd, hätten sie besser schön aufgetrennt. Naja... is nun mal nich anders.

[oxpus]

Jupp.

[cYbercOsmOnauT]

Jetzt hat sich bei mir doch ein Minifehler eingeschlichen.

Natürlich muss der Query

Code: Alles auswählen

SELECT config_name, config_value ...

lauten. Aber das Euch sowieso klar.

[oxpus]

<!-- SARKASMUSS AN -->

Code: Alles auswählen

$board_config[$row[config_name]] = $row[config_value];

müsste dann auch besser

Code: Alles auswählen

$board_config[$row['config_name']] = $row['config_value'];

heissen
<!-- SARKASMUSS AUS -->
Und so hatte ich es auch bereits "von Dir" übernommen

<-- *kommt sich vor, wie im Kindergarten*

[AmigaLink]

*kommt sich vor, wie im Kindergarten*

Muss auch mal sein.