Was willst du denn kopieren/hinzufügen/bearbeiten?
Die Funktionen in den einzelnen phpBB-Modulen unterscheiden sich hierbei schon sehr stark, dass man nicht wirklich etwas für die gewünschten Funktionen empfehlen kann.
Es kommt dabei ja auch immer darauf an, was man an Daten wie "bewegen" will.
Am besten geht das immer noch mit neu geschriebenen Script-Abläufen.
Und das sollte man sich besser auch immer erst mal auf ein Stück Papier schreiben, damit man nicht die Übersicht verliert, wenn man in einer groß gewordenen Datei noch etwas finden möchte...
Zur Datei selber (umfangreicher, als ich es eigentlich wollte):
if (!$auth->acl_get('a_commands_view'))
{
trigger_error('NOT_AUTHORISED');
}
$user->add_lang('mods/info_acp_commands');
Dieser Block ist eigentlich überflüssig.
Wenn die Berechtigungen eingestellt werden können, verwaltet das Berechtigungssystem des phpBB 3 bereits selber, ob ein Modul angezeigt wird oder nicht. Also je nach User, bzw. Usergruppe.
Auch muss eine info_acp_...php-Sprachdatei nicht noch einmal manuell eingebunden werden, da dieses das Forum selber schon macht. Hierzu wird der Dateiname der Moduldatei durch ein vorangestelltes "info_" ergänzt und gesucht, bzw. dann auch eingebunden, wenn eine entsprechende Datei vorhanden ist.
Die Aktionen "add" und "edit" würde ich in einem Block zusammenführen.
Also
Denn so kann man mit weniger Programmcode allgemeine Variablen und Werte setzen, die für beide Aktionen gelten und dann einfach mit einer Bedingung, wie z. B. das "Vorhandensein" (z. B. aus einer Übergabe aus einem Formular heraus) einer ID-Nummer für den gerade zu bearbeitenden Datensatz, die Aktionen trennen. Du hast dafür die Variable $command_id verwendet.
Hiermit lässt sich gut zwischen 'add' und 'edit' unterscheiden, indem du folgende Bedingung verwendest:
if ($command_id)
{
// Bereich für 'edit', um z. B. die Daten für diese ID aus der Datenbank zu lesen
}
else
{
// Aktionen, um die später zu verwendenden / darzustellenden Variablen zu initieren/vorzubereiten.
}[/code]
Damit wird der switch-Block um ein Stück kleiner und du musst nicht zweimal einen Code-Block schreiben, um z. B. die Inhalte an das Template zu übergeben.
Auch kann man in diesen "gemeinsamen" Block auch viele weitere gemeinsame Punkte auf einmal abarbeiten, was sonst immer verdoppelt werden müsste.
Merke:
Ein fehlendes break; in einem switch-Block aktiviert die Aktionen für alle mit case angegebenen Möglichkeiten.
Also wenn $action eben 'add' oder 'edit' wäre, würde man so immer den gleichen Script-Part abarbeiten lassen.
Und zuletzt wird die Datei damit auch kleiner und belegt weniger PHP-Speicher, was die Performance des Servers auch wieder ein Stück verbessert.
Das gleiche Prinzip gilt dann für deine Idee, zwei switch-Blöcke zu verwenden (Vorbereitung und Durchführung).
Auch dieses Prinzip dient nur dem vielleicht besserem Verständnis, was im Script passiert (auch wenn man selber das Script nach längerer Zeit mal wieder anfasst und zunächst auch erst wieder nachsehen muss, was wo gemacht wird).
Aber: Wenn du im ersten switch-Block 'add' und 'edit' zusammenlegst, kannst du dir den zweiten Schritt über den zweiten switch-Block sparen.
Das verringert dann auch gleich wieder die Dateigrösse und verbessert hierbei aber auch die "Lesbarkeit" des Scripts: Man muss sich nicht erst "oben" merken, was "unten" wie verwendet wird.
Dann hast du im 2. switch-Block eingetragen:
Das ist logisch doppelt gemoppelt.
Ich denke, anstelle
solltest du hier lieber
abfragen, da sich ja entscheidet, ob $command_id einen Wert hat, also ein bestehender Datensatz aktualisiert (SQL-UPDATE) oder neu hinzuzufügen (SQL-INSERT) ist.
Bei der von dir vorgegebenen Reihenfolge wäre allerdings die Bedingung so einzutragen:
Also mit einem vorangestellten Ausrufezeichen, was die Bedingung negiert (= "wenn $command_id nicht vorhanden ist").
Oder du würdest das INSERT und UPDATE in der Ablaufreihenfolge umdrehen, dann gilt eben die Bedingung ohne Ausrufezeichen.
Zuletzt noch ein Hinweis zu Abfragen, wenn du nur
einen bestimmten Wert aus einer Tabelle, bzw. einem Datensatz holen möchtest:
Code: Alles auswählen
$sql = 'SELECT MAX(command_id) as max_command_id
FROM ' . COMMANDS_TABLE;
$result = $db->sql_query($sql);
$row = $db->sql_fetchrow($result);
$db->sql_freeresult($result);
Hier fragst du die höchste command_id ab, die gespeichert wurde, um diese (um 1 erhöht) für den nächsten einzufügenden Datensatz verwenden kannst.
Das geht in der Abfrage so einfacher und performanter:
Code: Alles auswählen
$sql = 'SELECT MAX(command_id) as max_command_id
FROM ' . COMMANDS_TABLE;
$result = $db->sql_query($sql);
$command_id = $db->sql_fetchfield('max_command_id');
$db->sql_freeresult($result);
Das spart dann auch die Verwendung von $row, wie du es im weiteren Verlauf des Scripts getan hast.
Zu diesem Punkt wäre aber grundsätzlich zu überlegen, ob man diese eher unnötigen Abfragen nicht einfach mit einem Auto-Wert für das Feld command_id direkt in der Datenbank erledigt.
Denn damit müsste man für einen neuen Datensatz nicht zwangsweise die letzte command_id auslesen (gerade bei grösseren Tabellen kann dieses dazu auch richtig viel Zeit kosten!) und dann um 1 erhöhen, sondern fügt einfach den Datensatz ohne eine vorgegebene command_id ein.
In phpBB kann man dannach, bei Bedarf, die ID für den gerade neu gespeicherten Datensatz auch mittels
abfragen, muss ihn dann also auch nicht vorher erst "berechnen".
Dann zu deiner Löschroutine:
Die ist nicht falsch, aber unnötig langsam.
Erst fragst du ab, ob der zu löschende Datensatz vorhanden ist, um ihn dann zum löschen anzubieten, bzw. auch tatsächlich zu löschen.
Das ist unnötig, sofern du den Inhalt aus der Prüfung nicht auch gleich in die Bestätigungsabfrage zum löschen einbindest (der Benutzer wüsste dann auch gleich, was er gerade löscht).
Wenn du auf diese Zusatzinformation für den Benutzer verzichten willst, was du aktuell ja tust, dann würde es absolut ausreichen, ohne Prüfung, ob hinter der $command_id auch ein Datensatz in der Datenbank steht, die Bestätigung anzuzeigen, bzw. das Löschen durchzuführen.
Denn: Auch wenn ein Datensatz nicht vorhanden ist, gibt die Datenbank ein OK nach dem Löschen zurück, da ja kein Fehler vorliegt ("nicht löschen" bedeutet ja schliesslich auch, dass der Löschvorgang erfolgreich war, auch wenn "0 Datensätze" gelöscht wurden!!).
Und so solltest du die Löschung nicht durchführen:
Code: Alles auswählen
$db->sql_query('DELETE FROM ' . COMMANDS_TABLE . " WHERE command_id = $command_id");
Das ist zwar nicht falsch, aber wenn du dich schon an die Vorgaben aus dem Wiki auf phpbb.com hälst, solltest du auch die Coding Guidelines beachten und die erlauben solche Konstrukte eher nicht.
Auch lässt sich so (in deinem Fall weniger, aber bei anderen eher komplexeren SQL-Anweisungen) nicht so schnell etwas einfügen, ohne Gefahr zu laufen, den gesamten Befehl und damit auch das Script in der Syntax zu zerstören und zunächst mit einem Fehler anzuhalten. Was so eben schneller passieren kann.
Also merke: Immer $sql = ''; definieren und dieses dann mit $db->sql_query($sql); ausführen.
Ist sauberer programmiert, lesbarer und besser in der späteren Pflege des Codes zu handhaben.
Ansonsten könnte ich jetzt nicht behaupten, dass dein Script auch ohne meine Anmerkungen Probleme hätte.
Ich würde es einfach starten und ausprobieren.
Da du ja keine phpBB-Daten und Tabellen anfasst, wäre ein Fehler auch schnell verzeihbar und "deine" Tabelle kann ja auch während der Tests schnell mal wieder geleert werden...