Coding-Standards und Sicherheit sowie Fehlervermeidung

[noRiddle (revilonetz)]

Hallo Community.
Wie Andere auch schon angemahnt haben (z.B. modulfux) finde ich, daß es dringend Vorgaben für Coding-Standards bedarf was Erweiterungen und überhaupt Code für modified eCommerce betrifft.
Können wir das nicht mal umsetzen ?

Ich sehe so viel, sorry für die deutlichen Worte, so viel Schrott, daß es weh tut.
Es scheint vor allem auch die Faulheit Code lesbar zu formatieren um sich zu greifen.

Aktuell, allerdings, fiel mir wieder einmal auf, daß in DB-Queries ständig Single Quotes benutzt werden wo mit einem INT-Feld in der DB verglichen wird (siehe dazu auch dieses Ticket #1417).
Das ist nicht nur eine Unart sondern kann auch zu Fehlern (siehe zitiertes Ticket), evtl. zu Sicherheitslücken und zukünftig sogar zu Funktionseinbußen kommen.

Simples Beispiel:
Verkehrt

Code: SQL  [Auswählen]

SELECT *
FROM customers_status
WHERE customers_status_id = '';

Richtig

Code: SQL  [Auswählen]

SELECT *
FROM customers_status
WHERE customers_status_id IS NULL;

bzw. in Code mit PHP-Variablen immer gecastet

Code: PHP  [Auswählen]

xtc_db_query("SELECT *
                FROM customers_status
               WHERE customers_status_id = ".(int)$id);

und eben nicht so (ob gecastet oder nicht)

Code: PHP  [Auswählen]

xtc_db_query("SELECT *
                FROM customers_status
               WHERE customers_status_id = '".$id."'");

Wer die erstgenannte Query ausführt wird (wenn unwissend evtl. erstaunt) feststellen, daß es ein Ergebnis gibt, nämlich das wo customers_status_id == 0 ist, obwohl wir doch nach WHERE customers_status_id = '' gefragt haben.

Man muß immer die Types der Felder kennen und wissen ob das Feld NULL sein kann wenn man Queries baut.
D.h. auch, daß, ändert man die Eigenschaften eines DB-Feldes, Queries auf das Feld im Code überprüft werden müssen.

Bitte, bitte, hört auf damit die Parameter in WHERE-Clauses zu quoten wenn es sich um INT-Felder in der DB handelt.
(Ich habe das bestimmt auch schon gemacht  )

Ein weiterer, eher kosmetischer Grund, der zur Lesbarkeit und Verständlichkeit beiträgt, wäre das Einschieben von Zeilen.
Früher hat man eigtl. immer einen Tab weit eingerückt und ein Tab sollte im Editor der Wahl so eingestellt sein, daß er 4 Leerzeichen setzt.
Viele, und vor allem auch in modified oft so zu sehen, setzen einen Tab jedoch auf 2 Leerzeichen.
Es wäre schön wenn wir da mal einen Standard für hätten.

Ich wäre sogar dafür, daß hier eingestellte Erweiterungen und Module ein Rating bekommen sollten.
Wenn modified-Team-Mitglieder oder Experten eine Erweiterung bewertet haben bekommt sie Sternchen.

So. Nachdem meine Wenigkeit in dieser Community nicht wenig beiträgt, vor allem auch mit Code, Erweiterungen und Modulen, ich jedoch wenn ich mal etwas benötige regelmäßig Schrott vorfinde
- nicht immer natürlich, da gibt es schon die Guten -
geht mir das langsam auf den Zeiger.
Deshalb nochmals:
Lasst uns eine FAQ mit Coding-Standards machen, bitte.

Gruß,
noRiddle

Linkback: https://www.modified-shop.org/forum/index.php?topic=40017.0

[noRiddle (revilonetz)]

Das Interesse erschlägt einen ja fast...

Gruß,
noRiddle

[noRiddle (revilonetz)]

Ich geb' nicht auf...

[GTB]

Ich höre dich. Ich bin mir fast sicher, dass wir auch noch Stellen im Core finden, wo es falsch gemacht wird. Allerdings werden diese korrigiert sowie sie entdeckt werden.

Wir verwenden schon seit Jahren 2 Leerzeichen und keine Tabs. Das zu ändern hat grosse Auswirkungen auf das SVN. Wenn wir eine Datei ändern und dort umstellen auf 4 Leerzeichen, hat das zur Folge dass man im Trac die Codeänderungen nicht mehr sieht, da die gesamte Datei geändert wurde.

Gruss Gerhard

[noRiddle (revilonetz)]

Ja klar, TAB
- damit meine ich ja im Endeffekt lediglich die Keyboard-Taste -
ist auf 4 Leerzeichen eingestellt bei mir. Leerzeichen für TAB war ja schon immer empfehlenswert.
Wenn Ihr 2 Leerzeichen und nicht 4 nehmt und als Standard haben möchtet, dann mache ich das ab jetzt auch so.
Wäre schön eine Art FAQ für Entwickler und Modul-Hersteller gäbe was Coding-Vorgaben betrifft...

Gruß,
noRiddle

[GTB]

Für individuelle Module kann gerne auf 4 Leerzeichen gestellt werden.

Gruss Gerhard

[vr]

Hallo noRiddle,

Aktuell, allerdings, fiel mir wieder einmal auf, daß in DB-Queries ständig Single Quotes benutzt werden wo mit einem INT-Feld in der DB verglichen wird (siehe dazu auch dieses Ticket #1417).

Das Parametrisieren eines sql-Statements ist im Code noch an einigen Stellen schräg, hat wohl historische Gründe und evtl halbes Verständnis von Abfragen und php-Typumwandlung. Zb:

Code: PHP  [Auswählen]

echo (int)$id; // 0, plus notice

$id = '';
echo (int)$id; // 0
 
$id = 'huhu';
echo (int)$id; // 0
 
$id = null;
echo (int)$id; // 0

liefern alle 0. In einer Abfrage bekommt man damit Treffer, wenn das Feld 0 enthält, in Deinem Beispiel also immer den Admin

is null triffts aber nur dann, wenn in den Daten dieser Status zur Steuerung verwendet wird. Das muss nicht der Fall sein und wird speziell in der MySQL-Szene immer noch eher wenig genutzt. Und Abfragen können dann nicht einfach Parameter reingesteckt bekommen, sondern müssen auch den Vergleichsoperator ändern.

Bzgl coding standard:

Ein weites Feld. Es gibt mehrere offizielle (Zend, PEAR, ...). Du weißt selber, wie viele hunderte Details man da regeln kann. 2 Leerzeichen weicht bspw von Zend ab, aber auch ich finde es lesbarer so. Tabs führen zuverlässig zu Chaos, wenn mal wieder ein übereifriger Editor eine Quelldatei umformatiert. Weitere Kleinigkeit: Leerzeichen links und rechts vom Gleichheitszeichen.

Bei Formatierung von SQL-Statements ist es gruselig. Ich lese SQL wie Fließtext und mag keine Großschreibung der SQL-Keywords, aber das ist der historische Standard. Stell Dir vor, Du müsstest in php jeden Funktionsnamen großschreiben. Auch die Konvention, jedes Feld einer Selektion auf einer separaten Zeile zu notieren, versperrt die Sicht auf das sql-Statement, bei komplexeren Abfragen wird es unleserlich. In anderen Fällen ist es hilfreich. Das wurde im Projekt mal als Konvention eingeführt und seitdem ist es so.

Zb:

Code: PHP  [Auswählen]

"SELECT customers_status_show_price_tax,
        customers_status_add_tax_ot,
        customers_status_discount,
        customers_status_discount_attributes
FROM ".TABLE_CUSTOMERS_STATUS."
WHERE customers_status_id = '".$order->info['status']."'
AND language_id ='".(int)$lang['languages_id']."'");

oder so

Code: PHP  [Auswählen]

$lang_id = (int) $lang['languages_id'];
...

Code: PHP  [Auswählen]

"select show_price_tax, add_tax_ot, discount, discount_attributes
from customers_status
where id = {$order->info['status']} and language_id = $lang_id";

Das zweite bedeutet weitreichende Änderungen:
- Rauschen in den Spaltennamen entfernt, der Tabellenname muss nicht nochmal enthalten sein
- Tabellendefines: haben wir das je gebraucht?
- eingebettete Variablen: komplexe brauchen {} drumrum, Funktionsaufrufe lassen sich nicht einbetten
- im Skript häufig genutzte Variablen (languages_id) rausziehen

printf oder prepared statements wären Alternativen, auch die haben Nachteile.

Kriterium ist immer, ob man auf ein Stück Code schaut und schnell erkennen kann, was Sache ist. Das ist nicht durch eine festgelegte Konvention erreichbar, weil es viele unterschiedliche Szenarien gibt. Ein Minimalkonsens bzgl Konvention sollte allerdings schon da sein.

Grüße, Volker

[noRiddle (revilonetz)]

Hallo vr.
Danke für deinen Beitrag.
Ja, bzgl. deines letzten Absatzes stimme ich dir zu.
Allerdings sehe ich ständig völlig unformatierten oder schwachsinnig formatierten Code bei Erweiterungen und Modulen. Es liegt jeweils auf der Hand, daß es sich entweder um Copy & Paste von Teilen von Code aus alten Dateien handelt oder schlicht um das was auch auf das Erstgenannte zutrifft, Faulheit.
Wenn es sich um frewillig und kostenlos hier geposteten Code handelt, was soll man sagen. Wenn es sich jedoch auch bei gekauften Erweiterungen so verhällt bekomme ich Würgegefühle.

...
is null triffts aber nur dann, wenn in den Daten dieser Status zur Steuerung verwendet wird. Das muss nicht der Fall sein und wird speziell in der MySQL-Szene immer noch eher wenig genutzt. Und Abfragen können dann nicht einfach Parameter reingesteckt bekommen, sondern müssen auch den Vergleichsoperator ändern.
...

Auch klar.
Wenn man allerdings mySQL-Abfragen baut sollte man die Struktur der Tabelle und deren Felder kennen, das heißt auch die Feld-Typen und ob NULL oder NOT NULL, und nicht "blind" Queries formulieren.
Das

Code: PHP  [Auswählen]

WHERE customers_status_id = ".(int)$id

ist natürlich auch nur dann korrekt wenn bei der Definition von $id bereits ein leerer oder nicht numerischer String ausgeschlossen wurde. Meist wird der int-Cast ja, was die Intention betrifft, lediglich als Schutz vor SQL-Injection verwendet wenn es sich um benutzer-beinflusste Parameter handelt.
Das reicht aber eben nicht, man muß die DB-Tabelle genau kennen.

(Jetzt hoffe ich nur noch, daß niemand älteren von mir hier geposteten Code genauer anschaut... :schwitz:)

Gruß,
noRiddle

[Modulfux]

Ich bin ja auch ein absoluter Befürworter von lesbarem und "sauberen" Code. Insbesondere was die SQL-Queries angeht, füge ich bei meinen Modulen immer noch FILE und LINE ein, damit ich sofort sehe, wo es irgendwo hakt. Genauso frage ich beim Hinzufügen von Spalten vorher ab, ob diese Spalte nicht schon existiert, einfach um die error_logs klein zu halten.

Hier ein Beispiel:

Code: PHP  [Auswählen]

public function check() {
  if (!isset($this->_check)) {
    $query = xtc_db_query("-- " . __LINE__ . __FILE__ . "
      SELECT 1
      FROM   " . TABLE_CONFIGURATION . "
      WHERE  configuration_key = '" . $this->getDefine('STATUS') . "'
      LIMIT  1
    ");
    $this->_check = xtc_db_num_rows($query);
  }
  return $this->_check;
}

oder

Code: PHP  [Auswählen]

if (!$this->getColumn(TABLE_PRODUCTS, 'products_no_discount')) {
  xtc_db_query("ALTER TABLE " . TABLE_PRODUCTS . " ADD products_no_discount TINYINT(1) NOT NULL DEFAULT '0' AFTER products_status");
}

[Timm]

@noRiddle
Ich kann nicht viel zum Thema beitragen.

Aber: Was nützt ein sauberer Code, wenn er vom Nutzer selbst integriert wird? Viele Module haben ja eine Anleitung, wo man selbst noch was in seinen Dateien einfügen muss. Und wenn ein Tab bei dem einen 2 Leerzeichen ist und bei anderen 4 Leerzeichen, dann ist es schon wieder unterschiedlich. Und der Normalnutzer weiß das nichtmal. Für den ist Tab gleich Tab.

Wenn die Selbsteinbauer wenigstens eine BOF und EOF Zeile einfügen würden bei jeder geänderten Codestelle (bzw jeder Modulentwickler das schon vorgibt in seiner Anleitung und es dem Nutzer so einfacher macht für copy und paste), dann hättet ihr Entwickler auch schneller einen Überblick, wenn der Nutzer es nicht mehr hat, weil er selbst nicht mehr weiß wo er was eingebaut hat und sich dann an euch wendet.

Gruß Timm

[noRiddle (revilonetz)]

@Modulfux
Ich wußte doch, daß du für Coding-Standards bist, hatte deinen Kommentar dazu schon vermisst.

@FräuleinGarn
Tja, das Kommentieren
- übrigens mit BOC (= Beginning Of Code) und EOC (= End of Code) und nicht ~OF (= ~ Of File) -
sollte Standard sein bei Code-Änderungen, einschl. eines Kürzels dessen der es einbaut.
Aber das bleibt wohl ein Traum.

Das mit TAB und Leerzeichen ist auf Coder und Entwickler beschränkt. Klar, daß da nicht jeder Unbedarfte drüber Bescheid weiß.

Was mir aber auf den Zeiger geht ist, daß auch Leute die hier Code einstellen Code-Müll produzieren was Formatierung betrifft, offensichtlich eben aus Faulheit.
Von unsinnigen Algorithmen und Fehlern im Code reden wir ja nicht, wobei sich Einige die hier schon Module und Erweiterungen angeblich auf modified 2.0.X angepasst haben wollen das lieber sein lassen sollten.

Gruß,
noRiddle

[Timm]

Ich bin ja auch so ein Symmetrie-Fetischist, insofern bin ich da ganz bei dir.

Ich hab mir das damals, warum auch immer, als BOF (begin of) - Einbauer - was wird eingebaut und EOF (end of) - Einbauer - was wurde eingebaut gemerkt und nun krieg ich das nicht mehr raus aus dem denken.

Gruß Timm

[Q]

Wie Andere auch schon angemahnt haben (z.B. modulfux) finde ich, daß es dringend Vorgaben für Coding-Standards bedarf was Erweiterungen und überhaupt Code für modified eCommerce betrifft.
Können wir das nicht mal umsetzen ?

Ich sehe so viel, sorry für die deutlichen Worte, so viel Schrott, daß es weh tut.
Es scheint vor allem auch die Faulheit Code lesbar zu formatieren um sich zu greifen.

An sich finde ich die Idee schon gut. Aber was machen dann so Laien wie ich, die gelegentlich versuchen ein Problem zu lösen, evtl. ein altes oder uralt OSC Modul auf die aktuelle Version umzubauen? Soll das dann hier im Forum gelöscht werden, wenn es nicht den Vorgaben entspricht?

Die Einrückungen versuche ich möglichst schon selbst zwecks der Übersicht einzuhalten, aber achte da wenig drauf, ob das jetzt 2, 4, 5 oder 10 Leerzeichen sind. Das kann ich versuchen zu ändern. Im Prinzip würfel ich meist irgendwelche Codeschnippsel zusammen und so sieht das dann auch manchmal aus  EOC und BOC vergesse ich auch hin und wieder, aber meist füge ich - wie hier auch schon erwähnt - einen kleinen Kommentar mit meinem Namen ein. Alleine schon um die Änderungen später schneller wieder zu finden.

Ich bin immer wieder schwer beeindruckt welches Level hier einige von Euch haben!
 

[Q]

Früher hat man eigtl. immer einen Tab weit eingerückt und ein Tab sollte im Editor der Wahl so eingestellt sein, daß er 4 Leerzeichen setzt.
Viele, und vor allem auch in modified oft so zu sehen, setzen einen Tab jedoch auf 2 Leerzeichen.

Meine Fresse ist die Option bei Notepad++ versteckt  

Bei Geany innerhalb einer Minute gefunden und angepasst, aber bei Notepad++ trotz Google noch suchen müssen.
Optionen --> Sprache --> rechter Rahmen "Tabulatoren"

Aber was nimmt man jetzt? 2 oder 4? Oder egal, Hauptsache Leerzeichen?

[noRiddle (revilonetz)]

Jau, hab' ich in notepad++ vor Aeonen (was so alt bin ich schon  ) eingestellt auf 4 Leerzeichen.
Wenn ich's neu einstellen müsste würde ich auch Tante G. benötigen oder zig Tage des Suchens.

Dein letzter Satz: Korrekt.

Ich würde das ohnehin machen wie ich es für sinnvoll und am Besten erachte, bei Code für modified wollte ich mich jedoch an die Wünsche des Teams anpassen.
Scheint denen aber nicht so wichtig zu sein.

Für individuelle Module kann gerne auf 4 Leerzeichen gestellt werden.

Gruss Gerhard

Gruß,
noRiddle