Neuigkeiten
  • Die modified eCommerce Shopsoftware ist kostenlos, aber nicht umsonst.
  • Damit wir die modified eCommerce Shopsoftware auch zukünftig kostenlos anbieten können:

Umfrage

Wer ist für Coding-Standards in modified

Unbedingt und schnell
2 (40%)
Unbedingt so bald es geht
1 (20%)
Nicht nötig
2 (40%)
Auf keinen Fall
0 (0%)

Stimmen insgesamt: 5

Umfrage schließt: 26. Februar 2019, 12:59:50

Autor Thema: Coding-Standards und Sicherheit sowie Fehlervermeidung  (Gelesen 380 mal)

Offline noRiddle

  • Experte
  • *****
  • Beiträge: 9.654
  • Geschlecht: Männlich
    • Teile Beitrag
    • Webdesign Bonn - Köln
Coding-Standards und Sicherheit sowie Fehlervermeidung
« am: 12. Februar 2019, 12:48:07 »
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  :beef:)

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

Offline noRiddle

  • Experte
  • *****
  • Beiträge: 9.654
  • Geschlecht: Männlich
    • Teile Beitrag
    • Webdesign Bonn - Köln
Re: Coding-Standards und Sicherheit sowie Fehlervermeidung
« Antwort #1 am: 17. Februar 2019, 15:28:08 »
Das Interesse erschlägt einen ja fast...

Gruß,
noRiddle

Offline noRiddle

  • Experte
  • *****
  • Beiträge: 9.654
  • Geschlecht: Männlich
    • Teile Beitrag
    • Webdesign Bonn - Köln
Re: Coding-Standards und Sicherheit sowie Fehlervermeidung
« Antwort #2 am: Gestern um 17:03:40 »
Ich geb' nicht auf...

Offline GTB

  • modified Team
  • *****
  • Gravatar
  • Beiträge: 5.234
  • Geschlecht: Männlich
    • Teile Beitrag
Re: Coding-Standards und Sicherheit sowie Fehlervermeidung
« Antwort #3 am: Gestern um 17:23:26 »
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

Offline noRiddle

  • Experte
  • *****
  • Beiträge: 9.654
  • Geschlecht: Männlich
    • Teile Beitrag
    • Webdesign Bonn - Köln
Re: Coding-Standards und Sicherheit sowie Fehlervermeidung
« Antwort #4 am: Gestern um 17:28:53 »
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

Offline GTB

  • modified Team
  • *****
  • Gravatar
  • Beiträge: 5.234
  • Geschlecht: Männlich
    • Teile Beitrag
Re: Coding-Standards und Sicherheit sowie Fehlervermeidung
« Antwort #5 am: Gestern um 17:36:48 »
Für individuelle Module kann gerne auf 4 Leerzeichen gestellt werden.

Gruss Gerhard

Offline vr

  • modified Team
  • *****
  • Beiträge: 2.630
    • Teile Beitrag
Re: Coding-Standards und Sicherheit sowie Fehlervermeidung
« Antwort #6 am: Gestern um 22:10:03 »
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

Offline noRiddle

  • Experte
  • *****
  • Beiträge: 9.654
  • Geschlecht: Männlich
    • Teile Beitrag
    • Webdesign Bonn - Köln
Re: Coding-Standards und Sicherheit sowie Fehlervermeidung
« Antwort #7 am: Heute um 10:35:19 »
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

Offline Modulfux

  • Experte
  • *****
  • Beiträge: 3.575
  • Geschlecht: Männlich
    • Teile Beitrag
    • Modulfux - die ultimative SEO-Url für modified
Re: Coding-Standards und Sicherheit sowie Fehlervermeidung
« Antwort #8 am: Heute um 10:41:11 »
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");
}

Offline FräuleinGarn

  • Viel Schreiber
  • *****
  • Beiträge: 2.546
    • Teile Beitrag
    • Fräulein Garn's Stoffbiotop
Re: Coding-Standards und Sicherheit sowie Fehlervermeidung
« Antwort #9 am: Heute um 10:53:36 »
@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

Offline noRiddle

  • Experte
  • *****
  • Beiträge: 9.654
  • Geschlecht: Männlich
    • Teile Beitrag
    • Webdesign Bonn - Köln
Re: Coding-Standards und Sicherheit sowie Fehlervermeidung
« Antwort #10 am: Heute um 12:56:44 »
@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

Offline FräuleinGarn

  • Viel Schreiber
  • *****
  • Beiträge: 2.546
    • Teile Beitrag
    • Fräulein Garn's Stoffbiotop
Re: Coding-Standards und Sicherheit sowie Fehlervermeidung
« Antwort #11 am: Heute um 13:48:30 »
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


Teile per facebook Teile per linkedin Teile per twitter