Custom phpstan config - Githubissues

staabm commented 2 years ago

Ermöglichen dass man eine projekt phpstan settings datei pflegen kann die update sicher ist

christophboecker commented 2 years ago

Das könnte man machen wie in Geolocation. Eigene Datei in Data/Addon/rexstan mit gleichem Namen. Die wird mit der default-Datei gemerged, so dass die individuellen Einträge aus Data Priorität haben. In der individuellen Datei fehlende Werte werden aus der Default-Datei genommen.

staabm commented 2 years ago

Jop so dachte ich mir. Auf wechem weg bearbeitet ihr die datei? Nur via editor im filesystem? Reicht das aus?

christophboecker commented 2 years ago

Na, der Unterschied ist, dass bei Geolocation die Datei (config.yml) die Installation steuert. Daher kann man die data/addons/geolocation/config.yml bereits vor der Installation per Editor anlegen. Der Editor bleibt auch die einzige Bearbeitungsmöglichkeit. Alles Weitere läuft konventionell mit Config-Page.

christophboecker commented 1 year ago

Um das wieder aufzunehmen. ich bin mal tiefer eingestiegen (Auslöser: PHP-Version in den Settings auswählen) und hab das Problem mit den Neon-Dateien verstanden.

Heute:

Datei	Verwendung
redaxo/addons/rexstan/phpstan.neon.tpl	Wird mitgeliefert, enthält die Vorgabewerte für alles, was nicht user-config ist.
redaxo/addons/rexstan/phpstan.neon.tpl	Wird bei der Installation angelegt als Kopie von phpstan.neon.tpl; einzige Änderung: inkludiert die user-config aus dem data-Verzeichnis
redaxo/data/addons/rexstan/user-config.neon	Die User-Config enthält drei Angaben, die in der phpstan.neon fehlen, von der die Settings-Seite verwaltet und bei der Installation mit Defaults vorgelegt werden.

Die individuellen Eintragungen sind erstmal durch Abspeichern in user-config.neon im Data-Verzeichnis per se update-sicher und zweites werden sie, aber ohne das es wer nutzt, in die Tabelle rex_config geschrieben, weil das Formular mit rex_config_form gebaut ist.

Das Problem dabei

Eigentlich sind es mehrere Probleme.

Das Kernproblem ist die Reihenfolge. In phpstan.neon wird die User-Konfig am Anfang inkludiert. Dann kommen die Defaults. Die späteren Defaults überschreiben zuvor gemachte Einstellungen. Konkretes Beispiel: in user-config.neon wird parameters: {phpVersion: 80100} gesetzt, in phpstan.neon wird parameters: {phpVersion: 70300} gesetzt. Als Resultat wird gegen PHP 7.3 getestet.
Das zweite Problem ist die harte Verdrahtung der Parameterabrufe via RexStanUserConfig. Das ist recht unflexibel so mit konkreten Methoden je Wert, der in den Settings vorkommt.
Das dritte Problem(chen) wäre die Settings-Seite. Sie basiert auf rex_config_form, ohne die Features konsequent zu nutzen.
neon-Dateien sind keine yaml-Dateien. ich kann die aktuelle phpstan.neon nur einlesen, wenn diverse String in ' ' gesetzt wären.

(1) Lösung bzgl. neon-Dateien

Das ist relativ einfach hinzubekommen. Wie schon gesagt ist es ein Reihenfolge-Problem. Wie wäre es damit, diese drei Dateien einzusetzen:

Datei	Verwendung
redaxo/addons/rexstan/default-config.neon	Wird mitgeliefert, enthält die Vorgabewerte für alles, auch wenn es später in der user-config landet. Die Datei wird bei Updates überschrieben.
redaxo/addons/rexstan/phpstan.neon	Wird bei der Installation angelegt und enthält nur zwei include-Zeilen. Zuerst die default-config.neon und dann die user-config.neon. Der Effekt: Einträge in der user-config überschreiben Einträge in der default-config
redaxo/data/addons/rexstan/user-config.neon	Die User-Config enthält die Angaben, die via Settings-Seite verwaltet werden. Sie überschreiben faktisch die default-Werte.Bei Installation/Update wird nur sichergestellt, dass die Datei ggf. leer existiert.

phpstan.neon.tpl wird nicht mehr benötigt und entfällt.

Aber: was ist mit includes in default-config.neon (vendor/spaze/phpstan-disallowed-calls/extension.neon) und in user-config.neon (Addon-Liste)? Werden die zusammengeführt? Das müsste man noch testen. Nur wie?

Einige der Einträge musste ich in ' ' setzen, das neon eben kein yml ist. Entweder es gibt einen Weg, die DAteien als Neon einzulesen (das ist doch irgendwo in phpstan drin ...) oder es braucht den neon-vendor :-( oder eben auf ' ' achten.

Grundsätzlich scheint die Sache tadellos zu funktionieren.

(2) Lösung bzgl. der harten Verdrahtung der Parameterabrufe

Da müsste RexStanUserConfig flexibler werden. Hier mal meine Vorschläge:

Daten Abrufen oder Setzen mittels universeller Set/Get-Methode
```
$value = RexStanUserConfig::getValue( 'parameters|phpVersion', PHP_VERSION_ID);
RexStanUserConfig::setValue( 'parameters|phpVersion',80000);
```
Ja, damit könnte man ja was auch immer in der Konfiguration unterbringen. Das lässt sich aber auch abfangen: wenn es den Schlüssel nicht in der default-config gibt: Exception. Der Entwickler wird es lernen.

Man könnte auch mit Magic Funktion arbeiten .... mein aktueller POC macht es halt so wie oben beschrieben.
getValue würde zunächst versuchen, den Wert aus der user-config zu laden und dann aus der default-config. Das hat einen schicken Nebeneffekt. Wenn was neu in die Settings übernommen wird, steht weiterhin der aktuelle Wert aus der default-config zur Verfügung - für phpstan ebenso wie beim Aufruf der Settings. Der geänderte Wert würde in der user-config landen.
Beide .neon werden bei der ersten Abfrage in eine statische Klassenvariable geladen. Alle weiteren getValue und SetValue laufen gegen diese Variablen. Es müsste also sowas wie save() oder commit() geben, um die user-config zu schreiben
Für die alten Methoden (getLevel, setLevel, save, ...) müsste man entscheiden: entweder nach außen so beibehalten und intern auf die neuen Aufrufe umbiegen oder ganz rauswerfen. Ich plädiere für ganz raus weil überflüssig, BC hin oder her.

(3) Lösung bzgl. Settings-Page

Hier plädiere ich dafür, die rex_config_form-Klasse konsequent einzusetzen. Das bedeutet:

Eine eigene class RexStanSettings extends \ rex_config_form zu spendieren, die die Save-Methode der Basisklasse überschreibt.
In der RexStanSettings->save() werden die jeweiligen Werte mit RexStanUserConfig::setValue( $fieldName,$fieldValue);gespeichert und am Ende mit RexStanUserConfig::commit() auf die Platte nach user-config.neon geschrieben.
Die Feldnamen im Formular müssen dann den Identifiern der Config-Einträge entsprechen, also z.B. parameters|phpVersion.
Perspektivisch könnten man festlegen, dass Feldnamen der Form rex_config|xyz ganz normal in die rex-config gespeichert werden. Wozu braucht man das? Z.B für Parameter zur Steuerung der Ergebnisanzeige (Alle aufgeblendet, also Collapsed, ...)

Und sonst ...

Wenn man es macht wie beschrieben, könnte man konsequent noch einen Schritt weite gehen. Eigentlich ist RexStanUserConfig überflüssig, wenn es eine Klasse RexStanSettings gibt. Die kann auch die nötigen statischen Methoden zum Abruf und Speichern der Daten enthalten - inkl. rex_config-Daten. Oder man macht 'RexStanUserConfig' zur Formularklasse und führt die Funktionalitäten unter dem Namen zusammen.

That's my 25 cent. Ist schon in allen wesentlichen Teilen weit programmiert und getestet. Feinschliff fehlt noch und eben ein paar Grundsatzentscheidungen vom "RexStan-Produktmanager". Ohne die mache ich auf Verdacht hin erst mal nicht weiter.

Daher: Feedback and decisions wellcome

staabm commented 1 year ago

danke für die sehr ausführliche Analyse.

ich würde das ganze nach und nach angehen (separate PRs).

PHP Version einstellbar machen.. finde ich nen wichtigen use-case, der via settings möglich sein sollte. dass man dazu die "include" reihenfolge anpasst, sodass die userconfig die einstellungen aus der phpstan.neon überschreibt macht total sinn.

redaxo/addons/rexstan/default-config.neon Wird mitgeliefert, enthält die Vorgabewerte für alles, auch wenn es später in der user-config landet. Die Datei wird bei Updates überschrieben. redaxo/addons/rexstan/phpstan.neon Wird bei der Installation angelegt und enthält nur zwei include-Zeilen. Zuerst die default-config.neon und dann die user-config.neon. Der Effekt: Einträge in der user-config überschreiben Einträge in der default-config redaxo/data/addons/rexstan/user-config.neon Die User-Config enthält die Angaben, die via Settings-Seite verwaltet werden. Sie überschreiben faktisch die default-Werte.Bei Installation/Update wird nur sichergestellt, dass die Datei ggf. leer existiert.

phpstan.neon.tpl wird nicht mehr benötigt und entfällt.

die aufteilung find ich gut.

redaxo/addons/rexstan/default-config.neon würde also sozusagen alles enthalten was heute phpstan.neon.tpl ist, außer
```
includes:
- %REXSTAN_USERCONFIG%
- vendor/spaze/phpstan-disallowed-calls/extension.neon
```

diese zeilen stehen in redaxo/addons/rexstan/phpstan.neon muss aber entweder analog phpstan.neon.tpl wg. %REXSTAN_USERCONFIG% gespeist werden, oder man erzeugt die datei ähnlich der user-config via in https://github.com/FriendsOfREDAXO/rexstan/blob/d673bf3e7a3420a7d039548530d5c19cdf364e6b/lib/RexStanUserConfig.php#L20

die redaxo/data/addons/rexstan/user-config.neon bleibt also wie sie heute ist, korrekt?

die andere Probleme würde ich besprechen wollen wenn das erste gefixed ist, damit wir nix durcheinander werfen

christophboecker commented 1 year ago

Kein Problem, ich mache nen PR dafür. Aber warum soll - vendor/spaze/phpstan-disallowed-calls/extension.neon raus? ist das überflüssig?

staabm commented 1 year ago

Nö das soll nicht raus

christophboecker commented 1 year ago

Hier mein Vorschlag für die nächsten Arbeitsblöcke, jeweils als eigenen PR, damit es übersichtlich bleibt.

1) Kleinere Umbauten im Settings-Formular settings.php

Liste der extensions nach vorne ziehen, das finde ich übersichtlicher bei Änderungen. Und dabei von realpath auf rex_path umstellen

    $phpstanExtensions = [
        rex_path::addon('rexstan','vendor/phpstan/phpstan/conf/bleedingEdge.neon') => 'Bleeding-Edge',
        rex_path::addon('rexstan','vendor/phpstan/phpstan-strict-rules/rules.neon') => 'Strict-Mode',
        rex_path::addon('rexstan','vendor/phpstan/phpstan-deprecation-rules/rules.neon') => 'Deprecation Warnings',
        rex_path::addon('rexstan','vendor/phpstan/phpstan-phpunit/rules.neon') => 'PHPUnit',
        rex_path::addon('rexstan','lib/phpstan-dba.neon') => 'phpstan-dba',
    ];

für das Paths-Feld auf "empty" prüfen. Das passiert jetzt erst wenn man die Analyse startet. Sollte man (usability) aber m.E. schon im Formular machen; Attribut "Required" oder mit Form-Validator.

2) Settings-Verwaltung "as is", also noch ohne phpVersion, umbauen auf ein eigenes rex_config_form

Klasse RexStanSettings extends rex_config_form mit der kompletten Logik zum Verwalten der Settings. Das beinhaltet sowohl die angepasste Speicherung als auch den Datenabruf entweder aus der user-config oder - wenn da noch nichts steht - aus der default-config.
RexStanSettings bringt auch schon die Speicherung in der rex_config mit (brauchen wir für 4.)
settings.php auf $form=RexStanSettings::factory() ändern
Aus settings.php die direkte, nun überflüssige Auswertung von $_REQUEST entfernen

3) Auswahl der phpVersion einführen

settings.php vorne um die VersionsListe erweitern und im Formular die Auswahl dazu einbauen
In RexStanUserConfig wie gewünscht zusätzliche get/set und den vierten Parameter bei Save. Aber: RexStanUserConfig würde sich faktisch bei RexStanSettings bedienen.

Steuerung der Analyse-Anzeige, Anzeigeoptionen
- in settings.php zwei Fieldsets bilden: PHPSTAN-Parameter und Anzeige-Parameter
- Als Anzeige-Parameter
  - Select mit vier Anzeigevarianten
  - Minimiert: alle Dateien initial minimiert anzeigen
  - Maximiert: alle Dateien initial maximiert anzeigen (wie jetzt)
  - Smart: wird gegen ein "Anzeigegröße-Budget" gerechnet (jede Datei, jedes Finding ein Punkt). Werden mehr Punkte benötigt als gegeben, werden halt die größten Dateien minimiert)
  - Dateiauswahl: Freitextangabe von Dateinamen, die aufgeblendet werden sollen (z.B. wenn man gezielt einzelne Dateien in einem Addon beobachtet und nicht alle)
  - Eingabefeld für das "Anzeigegröße-Budget"
  - Eingabefeld für Dateinamen.
- In analysis.php die jeweiligen Anpassungen, um die Anzeigevarianten umzusetzen.

Optional:

Die Darstellungsvarianten 3 und 4 weil etwas komplexer in jeweils eigenen PRs
Texte nach de_de.lang überführen

staabm commented 1 year ago

Liste der extensions nach vorne ziehen, das finde ich übersichtlicher bei Änderungen. Und dabei von realpath auf rex_path umstellen

solche kleinen dinge gerne direkt als separater PR vorschlagen. ich finde es ist unnötig kompliziert über solche dinge hin und her zu schreiben.. wenn konkreter code vorliegt ist es einfach zu mergen oder mit begründung zu schließen

für das Paths-Feld auf "empty" prüfen. Das passiert jetzt erst wenn man die Analyse startet. Sollte man (usability) aber m.E. schon im Formular machen; Attribut "Required" oder mit Form-Validator.

damit ist die "AddOn" auswahl gemeint? klingt nach einer kleinigkeit, gerne direkt vorschlagen als separater PR

2. Settings-Verwaltung "as is", also noch ohne phpVersion, umbauen auf ein eigenes rex_config_form

dafür sehe ich noch keinen grund/verbesserung zum status quo. anhand von konkretem code / PR wäre es einfacher zu bewerten

3. Auswahl der phpVersion einführen

jep gerne

4. Steuerung der Analyse-Anzeige, Anzeigeoptionen

diesen punkt und details dazu würd ich erst besprechen wenn die anderen gelöst/gemergt sind

staabm commented 1 year ago

Klasse RexStanSettings extends rex_config_form mit der kompletten Logik zum Verwalten der Settings

obacht: https://github.com/FriendsOfREDAXO/rexstan/pull/85#issuecomment-1211786053

christophboecker commented 1 year ago

obacht: https://github.com/FriendsOfREDAXO/rexstan/pull/85#issuecomment-1211786053

Den Hinweis verstehe ich nicht? Das eine hat doch mit den anderen nix zu tun. Wo bin ich grade auf dem Holzweg?

rex_config_form schreibt von Hause aus mittels $this->save() die aktuellen Feldwerte beim Absenden in die Tabelle rex_config bzw. liest die Feldwerte mit $this->getValue($name) aus der Tabelle rex_config. Will man andere Quellen nutzen (xxx.neon) muss man die beiden Methoden überschreiben, damit die Werte dort abgeholt bzw. abgelegt werden. Also eigene Klasse bauen; ist ja kein Hexenwerk, aber am Ende die saubere Lösung.

Ich wiß gar nicht mehr, wo ich das schon gesehen habe. Es war jedenfalls der Auslöser, es so in Geolocation als eigene Klasse zu bauen, die übrigens in der init-Methode auch gleich den Formularcode zusammenstöpselt - alles an einem Platz.

(Da RexStanUserConfig faktisch nichts anderes macht, als das Datei-Interface für das Formular zu liefern, kann das auch direkt in einer Klasse abgehandelt werden.)

FriendsOfREDAXO / rexstan

Custom phpstan config #28

Heute:

Das Problem dabei

(1) Lösung bzgl. neon-Dateien

(2) Lösung bzgl. der harten Verdrahtung der Parameterabrufe

(3) Lösung bzgl. Settings-Page

Und sonst ...