phpBB Ext Check - Diskussion bezüglich Prozedur und Reports

[LukeWCS]

Für den Bestanden-Demo-Bericht wird nicht mehr Version 2.0.0 von LFWWH verwendet, sondern die kommende Version 2.1.0. Dieser Bericht ist jetzt auch ein besseres Beispiel für das, was unter Punkt 5.1.b im Startbeitrag geschrieben wurde.

[LukeWCS]

Seit kurzem hat auch Dr.Death Vollzugriff auf den EC Server. Somit bin ich nicht mehr die einzige Person, die bei technischen Problemen danach schauen kann.

Startbeitrag um den Punkt "1.1 Ansprechpartner bezüglich Logins und technischen Dingen" ergänzt.

[oxpus]

So, ich habe mich jetzt auch entschlossen, das Tool zu nutzen und muss sagen:
Hut ab, ist wirklich gut geworden.
Es arbeitet zudem auch sehr fix, was ich so nicht erwartet hätte.

Aber ich habe auch 2 Anmerkungen zu den Prüfungen:

1. In einer Extensions sollte über die composer.json u. a. angegeben werden, für oder ab welcher PHP-Version die Extension eingesetzt werden soll(te).
Somit sollten meiner Meinung nach auch alle Prüfungen der PHP-Versionen nicht ausführt werden, welche von der Extension "offiziell" nicht unterstützt werden.
Gerade auch PHP 5.3 bis 5.5 sind mittlerweile so alt, dass diese noch nicht einmal ansatzweise in einer Live-Umgebung eingesetzt werden sollten.
Alternativ dazu sollten die Ergebnisse aus den Prüfungen zumindest nicht als Fehler sondern eher als Hinweis oder notfalls als Warnung angezeigt werden.
Hintergrund:
Ich verwende in der Download Extension anstatt array() nur noch [], was erst seit PHP 5.4 unterstützt wird.
Da in der Extension aber nun mal sehr viele Arrays verwendet und angesprochen werden, erhalte ich dadurch unnötigerweise über 4500 Fehlermeldungen, welche so keine Fehler sind, da die Extension ab PHP 7.2 und neuer eingesetzt werden sollte; zumindest in den Fassung ab 8.0.0, 8.2.5 ist dazu die aktuell letzte Version.
Ähnlich verhält es sich auch mit der Verwendung von Konstanten, die nicht über define() sondern über const in den Klassen definiert werden.
Auch das ist in php 5.x nicht möglich, führt also auch in diesen Tools zu "false positive".

2. Für die Download Extension werden auch über 120 Fehler zu fehlender Initialisierung von Array Variablen aufgelistet.
Genau diese hatte ich überall gesetzt und das wurde mir auf phpbb.com bei der Validierung der Extension auch prompt um die Ohren gehauen.
Begründung:
Man braucht kein Array initialisieren, wenn dieses vorher noch nicht verwendet / gesetzt ist, da die erste Zuweisung ein Array automatisch initiiert.
Die konkrete vorherige Initialisierung als "leeres" Array ist daher überflüssig und sollte somit unterbleiben.

Diese beiden Punkte stelle ich mal hier zur Diskussion.
Für meine Download Extension werden diese Punkte eben nun zahlreich als Fehler ermittelt, welche aber defacto keine Fehler sind, bzw. nicht sein sollten.

Für mich waren es jetzt letztlich nur "Kleinigkeiten", die ich an der Extension noch korrigieren musste und das neueste Ergebnis ist für mich somit absolut zufriedenstellen.
Somit sollte ich der erfolgreichen Validierung meiner Download Extension positiv entgegensehen können.

[LukeWCS]

So, ich habe mich jetzt auch entschlossen, das Tool zu nutzen und muss sagen:
Hut ab, ist wirklich gut geworden.

Merci!

Hier im Thema sind wir jetzt im richtigen Rahmen, danke das du es hier nochmal ausführlich geschrieben hast.

Okay, zu Punkt 1: Dein Vorschlag ist bei mir auch schon länger als Idee im Hinterkopf und zwar im Prinzip schon seit phpBB 3.3 veröffentlicht wurde. Bei dieser phpBB Version erübrigen sich eine ganze Menge alter PHP Versionen. Aber grundsätzlich sind auch die alten PHP Versionen fallweise durchaus noch interessant, zum Beispiel um sich schnell einen Überblick über eine alte Ext verschaffen zu können, da sind inbesondere auch die PHP 5 Versionen interessant.

Wenn es aber darum geht die eigene, aktuelle Ext prüfen zu lassen, sind natürlich nur die PHP Versionen interessant, die man auch selbst per composer.json per Mindestversion vorgibt. Da hat du völlig Recht. Ich habe mir vor ein paar Monaten auch schon überlegt, wie man das a) komfortabel und b) praxisorientiert lösen kann. Damals bin ich zu diesem Konzept gekommen:

• Einen PHPC-Auto-Modus einbauen, der aus der composer.json die Mindestversion bei den PHP Anforderungen ermittelt. Dieser Auto-Modus führt dann nur die PHPC Module aus, die auch wirklich benötigt werden.
• Auf der Upload-Seite eine Checkbox einfügen mit der der Auto-Modus fallweise deaktiviert werden kann. Standardmässig ist die Checkbox schon gesetzt, also Auto-Modus aktiviert. Deaktiviert man den Auto-Modus, wird klassisch wieder das komplette PHP Spektrum geprüft, also 5.3 bis 8.0.

Auf diese Weise ist alles abgedeckt und die Prio liegt bei eigenen Exts. Trotzdem kann man fallweise dann eine alte oder fremde Ext prüfen lassen. Ich denke so ist das praktikabel.

Zu Punkt 2: Lies dir bitte einmal komplett den Startbeitrag durch, da stehen wichtige Informationen zu EC. Gleich vorab: Der EC Bericht muss nicht rundum "grün" sein, das dachten schon mehrere EC Benutzer. Wenn da also in deinem Fall über 4000 Fehler bei PHP 5.3 aufgeführt werden, ist das irrelevant, weil PHP 5.3 bei DL Ext überhaupt nicht (mehr) interessiert. Und die Meldungen von VA sind immer als "Hinweise" zu verstehen. Alle Details im Startbeitrag, inklusive der Info welche Prüfmodule wirklich wichtig sind und welche zweitrangig sind.

[oxpus]

Zu 1:
Mit der Auswertung der composer.json und einer möglichen manuellen Übersteuerung sollten die Prüfungen der PHP Kompatibilitäten genüge getan sein.
Hört sich sehr gut an.

Zu 2:
Nun ja, um aus über 160 angezeigten Hinweisen die wesentlichen Fehler herauszusuchen und zu korrigieren, kostet schon eine Menge Zeit.
Ja, ich hatte schon verstanden, dass das keine echten Fehler sind. Dennoch werden die Stellen aufgeführt, die zwar nicht alle relevant sind, aber dennoch erst einmal gesichtet werden müssen.
Daher sollte dieser Punkt vielleicht hier mal in der größeren Runde diskutiert werden, wie das andere sehen.
Gerade auch in Bezug zu der auf phpbb.com aktuell vertretende Meinung, dass Array-Initiierungen in dem Umfang, wie dieses durch die Validierung aufgezeigt wird, nicht umzusetzen sind.
Hier prallen eben unterschiedliche Auffassungen aufeinander.
Ich wäre auch für die durchgängige Initialisierung von Arrays, auch wenn diese das erste Mal in einem Script verwendet werden. Das ist immer noch besser und kann eine höhere Sicherheit bringen, als sich auf den Automatismus in PHP zu verlassen, welcher unerwartete Fehler enthalten könnte.

[LukeWCS]

Wegen VA: Das in dem Fall natürlich bei dir eine ganze Menge Zeug gemeldet wird, dass du gar nicht ändern darfst, ist natürlich unschön. Daran ändern kann ich jedoch nichts. Sämtliche Analysetools und Regelwerke stammen nicht von mir, mit Ausnahme von phpBB33YAMLcheck, das habe ich selbst geschrieben und in EC integriert. Das heisst ich habe hier keinen Einfluss auf die fremden Tools. Notfalls VA einfach ignorieren. EC bietet ja einen grösseren Prüfumfang als GA und in dem Fall ist VA als Bonus zu sehen. Ich schau bei Gelegenheit ob es irgendeine Möglichkeit per CodeSniffer Variable gibt, um gezielt fehlende Array Deklarationen zu ignorieren. Versprechen kann ich nichts.

Was den PHPC Auto-Modus angeht, gibt es bereits einen funktionierenden Prototyp. Es gab schon einen Mechanismus um eine ganze Modul-Kategorie deaktivieren zu können, hilfreich beim Entwickeln. Allerdings kann man damit z.B. PHPC nur ganz aus- oder einschalten, nicht individuelle Untermodule. Da muss ich erst noch Funktionen nachrüsten, damit aus dem Prototyp wieder eine ordentliche Lösung wird.

[oxpus]

Nur keinen Stress.
Ich validiere meine Extensions ja nicht permanent, daher geht man für ein neues Release einmal durch die auffälligen Stellen durch und gut ist es.
Dauert dann eben einfach länger.

Dass die einzelnen Tools nicht so einstellbar sind, wie dieses in den Coding Guidelines für phpBB vorgeben wird, habe ich auch schon an ganz anderen Stellen erlebt.
Beispiel Tabs anstatt 4 Leerzeichen zum Einrücken und öffnende Klammern immer in der nächsten Zeile anstatt mit Leerzeichen getrennt hinter dem Befehl / Funktionsaufruf.
Diese und weitere Vorgaben bekomme ich auch nur über ein eigenes Ruleset in meine IDE rein, so dass hierbei allein im Listener der Download Extension über 1000 Fehler abweichend zu aktuellen Standards angezeigt werden, wenn ich alle Prüfungen nach Standard durchführen würde.

Da kann man sich jetzt darüber aufregen oder nicht, ist aber halt so.
Und das wohl auch nur, weil der Code für das phpBB - vorsichtig ausgedrückt - "schick" aussehen soll...

Wie gesagt:
Wenn es eine Möglichkeit geben sollte, bzw. das auch anderweitig Sinn macht, die VA noch mal anzupassen, würde ich das begrüßen.
Ansonsten muss ich dann mit den zahlreichen "false positive" leben.

[LukeWCS]

Moin

Nur keinen Stress.

Der Prototyp war auf die Schnelle. Einfach mal schauen, wie ich die Automatik umsetzen kann. Da fehlt noch die Checkbox, neue Sprachvariablen und noch so ein paar Dinge.

Und das wohl auch nur, weil der Code für das phpBB - vorsichtig ausgedrückt - "schick" aussehen soll...

Hmm es geht nicht um schick, es geht um Lesbarkeit. Wenn ich Ext Validator bei .com wäre, würde ich auch auf einem Standard bestehen. Sonst müsste man sich permanent an zig verschiedene Stile gewöhnen, da wird man ja nicht fertig. Und wenn sich alle daran halten, hat das auch für uns Ext Coder einen grossen Vorteil wenn wir uns andere Exts anschauen müssen. Zum Beispiel wenn ein Kollege Hilfe braucht oder wenn wir selbst mal schauen wollen, wie Kollegen dieses oder jenes realisieren und sei es nur um zu optimieren. Wenn da jeder macht was er will, haben wir Chaos. Beispiel Inline-Control was nicht erlaubt ist. Es gibt durchaus Situationen wo das die Lesbarkeit sogar verbessert, aber in den meisten Fällen verschlechtert es eben die Lesbarkeit, darum nicht erlaubt.

Und der phpBB Standard ist übrigens absolut human, geradezu die Warmduscher-Version mit zusätzlichem Schattenparker-Effekt. Schau dir mal den PSR2 Standard an, DAS ist penibel. Es gab auf .com auch schon mal eine Diskussion darüber, das man PSR2 bei phpBB generell einführt, was dann auch für die Ext Coder gelten würde. Glücklicherweise wurde das abgelehnt, weil dann würde ich nichts mehr zur Validierung einreichen.

edit: Schau mal ob deine IDE den EditorConfig Standard unterstützt, da kannst du dir schon mal eine ganze Menge Kleinkram von der Backe nehmen. Wir haben dafür auch einen KB: Knowledge Base - Editor-übergreifende Format-Vorgaben mit EditorConfig. Dort habe ich einen Regelsatz für den phpBB Standard definiert, Dr.Death zum Beispiel nutzt ihn auch.

Wenn es eine Möglichkeit geben sollte, bzw. das auch anderweitig Sinn macht, die VA noch mal anzupassen, würde ich das begrüßen.
Ansonsten muss ich dann mit den zahlreichen "false positive" leben.

Ich habe mir vorhin mal die Parameter für VA angeschaut. Es gibt u.a. die Möglichkeit bestimmte Variablennamen auszuschliessen, das nutze ich im EC Source, weil ich da im Hauptmodul eine Menge Variablen deklariere, die VA als "unbenutzt" meldet. Sprich, ich nutze EC um EC selbst zu prüfen. Des Weiteren gibt es eine Möglichkeit Variablennamen sogar anhand RegEx bei VA auschliessen zu können. Dazu müssten deine Array-Namen aber ein einheitliches Präfix oder Suffix haben.

Das wäre jedoch dann nur fallweise, ich kann (und will) das nicht generell einbauen, weil es einfach von der Situation (Source) abhängt, ob das sinnvoll ist oder nicht. Fallweise heisst: es gibt bei CS die Möglichkeit Parameter per Kommentar(e) an CS selbst oder einen der Regelsätze zu übergeben. Aber das ist eben nur interessant, wenn deine Array-Namen ein Muster aufweisen, den man gezielt mit RegEx ansprechen kann. Und du müsstest das dann in jede Datei wo es benötigt wird als Kommentar einfügen.

Nicht ideal, aber ich sehe keine andere Möglichkeit. Es scheint bei VA keine Möglichkeit zu geben, gezielt eine bestimmte Regel-Kategorie (wie nicht-deklarierte Arrays) ausschliessen zu können.

[oxpus]

Die Coding-Regeln des phpBB entsprechen eher keinem aktuellen Standard und ja, sie dienen oberflächlich betrachtet nur der "Lesbarkeit".
Ich will diese Regeln absolut nicht in Frage stellen oder gar aufheben, denn sie sind ja sinnvoll.
PSR2 würde mir persönlich allerdings schon deutlich besser gefallen, gerade was auch die Verwendbarkeit des Codes in anderen Systemen anbelangt, wo eben andere Regeln gelten.
Aber bleiben wir bei den bestehenden Regeln, denn ich gebe Dir vollkommen Recht, dass eine Umstellung auf PSR2 die meisten phpBB-Entwickler vergraulen könnte.

Glücklicherweise hatte ich vorhin doch noch eine Möglichkeit gefunden, einen phpBB-Coding-Regelsatz in meiner IDE zu integrieren, wodurch jetzt durchgängig danach geprüft werden kann.
Jetzt wäre nur noch die enthaltene "PHP Intelligence" davon zu überzeugen, die phpBB-eigenen Container ebenfalls als "korrekt" zu markieren, damit ich meine Extensions auch einzeln validieren kann. Da zickt der betreffende Linter noch etwas rum, da bislang leider nur innerhalb eines Projektes validiert wird, unabhängig, worin sich dieses Projekt befindet oder zu welchem "größeren" Projekt es gehören könnte. Da gibt es noch Verbesserungspotential...

Lass bitte die VA, wie sie ist.
Sollte mal jemand irgendwann zufällig über eine Methode stolpern, eine tatsächlich fehlende Array-Initialisierung nicht mehr als Fehler/Warnung ausweisen zu müssen, dann okay. Ansonsten war mein Ansinnen zunächst nur als Diskussionsgrundlage gedacht.
Deine Idee, Variablen-/Array-Präfixe über Kommentare zu definieren und dadurch alle festgestellten Initiierungen zu ignorieren, scheidet für mich aber auch aus, da damit tatsächlich fehlende Initiierungen eher nicht mehr erkannt werden. Dann lebe ich lieber mit den false positive Meldungen, bevor ein echter Fehler übersehen wird.
Ein schwieriges Thema, gebe ich zu, aber wirklich keines, welches mit Gewalt gelöst werden muss.

An dieser Stelle aber noch einmal ein großes Lob für den umfangreichen Ext Check. Das ist wirklich eine große Hilfe für alle Entwickler und kann nicht oft genug bestätigt werden.
Mir wird es jedenfalls zukünftig sehr viel helfen, was ich lange Zeit idiotischerweise ignoriert hatte.

[LukeWCS]

PSR2 würde mir persönlich allerdings schon deutlich besser gefallen, gerade was auch die Verwendbarkeit des Codes in anderen Systemen anbelangt, wo eben andere Regeln gelten.
Aber bleiben wir bei den bestehenden Regeln, denn ich gebe Dir vollkommen Recht, dass eine Umstellung auf PSR2 die meisten phpBB-Entwickler vergraulen könnte.

Es hat eh fast jedes Web-Projekt seine ganz eigenen Regeln. Wenn man natürlich Dinge aufgrund langer Erfahrung anders gewohnt war, ist es immer erstmal mühselig, sich an die Projekt-spezifischen Vorgaben anzupassen. Ging mir zumindest so, als ich im Sommer 2018 anfing mich mit phpBB Ext zu beschäftigen und auch überhaupt erst mit PHP anfing. Da war zuerst mal kräftig umdenken angesagt. Nicht nur was PHP angeht, auch was die phpBB Richtlinien angeht. Ich komme eigentlich aus der Compiler und IDE Welt, mit Interpreter-Sprachen habe beruflich nur am Rande zu tun. Und da war für mich der Einstieg in PHP und phpBB ein regelrechter Kulturschock. Allerdings, wenn man schon ein paar Jahrzehnte Software Entwicklung auf dem Buckel hat, ist beim Einlernen in ein neues Projekt und/oder eine neue Programmiersprache die anfängliche "Schmerzphase" relativ kurz. Inzwischen liebe ich es, in PHP zu programmieren. Unter anderem deswegen, das gebe ich offen zu, weil ich mich um eine ganze Menge nerviger Details gar nicht kümmern muss.

Glücklicherweise hatte ich vorhin doch noch eine Möglichkeit gefunden, einen phpBB-Coding-Regelsatz in meiner IDE zu integrieren, wodurch jetzt durchgängig danach geprüft werden kann.
Jetzt wäre nur noch die enthaltene "PHP Intelligence" davon zu überzeugen, die phpBB-eigenen Container ebenfalls als "korrekt" zu markieren, damit ich meine Extensions auch einzeln validieren kann. Da zickt der betreffende Linter noch etwas rum, da bislang leider nur innerhalb eines Projektes validiert wird, unabhängig, worin sich dieses Projekt befindet oder zu welchem "größeren" Projekt es gehören könnte. Da gibt es noch Verbesserungspotential...

Das hört sich doch schon mal gut an. Was IDEs und deren Möglichkeiten angeht: Du musst berücksichtigen, dass die meisten Ext Coder wohl eher keine ausgewachsene IDE benutzen, sondern einen guten Editor. Und einige der EC Nutzer auch gar nicht vorhaben, offiziell validieren zu lassen. Und ebenso nutzt nicht jeder Ext Coder automatisch auch Github. Ein "Gelegenheits-Programmierer" kann/will sich nicht mit Github oder IDEs auseinandersetzen. Aber um z.B. per EPV und PSSE prüfen zu können, müsste man mindestens Github nutzen. Es sei denn, man richtet sich alle benötigten Analysetools lokal ein, das setzt aber auch schon wieder einiges Grundwissen voraus. Für eben diese Zielgruppe wurde EC ursprünglich geschaffen, es schliesst eine Lücke bei den Werkzeugen für diese Gruppe.

Wenn man eine IDE nutzt, hat man eh ganz andere Möglichkeiten der Prüfung und je nach IDE kann man sogar CS gleich direkt integrieren. Ich habe zwar eine IDE installiert, aber noch komme ich prima mit NP++ zurande. Wenn ich mal solche Mammut-Projekte ähnlich wie deine DL Ext anfange, werde ich auf jeden Fall die IDE benutzen.

Dann lebe ich lieber mit den false positive Meldungen, bevor ein echter Fehler übersehen wird.
Ein schwieriges Thema, gebe ich zu, aber wirklich keines, welches mit Gewalt gelöst werden muss.

So sehe ich das auch. Schlussendlich ist EC ja für niemand eine Pflicht, das ist völlig wahlfrei. Es soll Ext Coder bei ihrer Arbeit unterstützen und gleichzeitig auch den phpBB Coding Standard etablieren und die Code Qualität verbessern. Und davon profitieren schlussendlich alle Ext Coder.

An dieser Stelle aber noch einmal ein großes Lob für den umfangreichen Ext Check. Das ist wirklich eine große Hilfe für alle Entwickler und kann nicht oft genug bestätigt werden.
Mir wird es jedenfalls zukünftig sehr viel helfen, was ich lange Zeit idiotischerweise ignoriert hatte.

Danke für dein Lob! Es steckt auch sehr viel Arbeit und Herzblut darin. Schlussendlich sind auch immer wieder Vorschläge von den EC Benutzern eingeflossen, siehe Changelog.

Und hier mal eine Vorschau wie das dann aussieht, wenn phpc-auto aktiviert ist:
[ externes Bild ]

Wenn phpc-auto deaktiviert ist, sieht es klassich so aus:
[ externes Bild ]

Ich nutze dazu bereits vorhandene Funktionalität mit der ich eine ganze Prüf-Kategorie deaktivieren kann. Diese musste ich jetzt nur geringfügig anpassen. Gestern Abend bin ich noch von einem grösseren Aufwand ausgegangen, als ich das quick-and-dirty umgesetzt habe. Aber tatsächlich war eigentlich das meiste für phpc-auto schon vorhanden.

Oben links ist ein Debug. Dort lasse ich mir im Prototyp die bereinigte PHP-Mindestversion anzeigen. Bereinigt heisst: wenn ich die Version extrahiert habe, prüfe ich sie auf einen gültigen Bereich. Ist sie geringer als 5.3 oder schlicht ungültig, wird 5.3 gesetzt. Ist sie höher als 8.0 wird eben 8.0 gesetzt. PHPCompatibility erwartet bei den Parametern die Version im Format <major>.<minor>. Wenn also in der composer.json z.B. >=7,<8 stünde, würde das von EC als ungültig behandelt und auf 5.3 gesetzt werden.