phpBB Ext Check - Diskussion bezüglich Prozedur und Reports

[HJW]

Gute Sache, ist bei mir auch installiert.
Ich bin auch schon etwas weiter. Ich bekomme aber zwei Warnungen des PHP-Code-Sniffers angezeigt, für die main-listener.php und für die constants.php. Die Meldungen sind gleich, ich habe hier mal die für die constans.php eingefügt:

Code: Alles auswählen

FILE: hjw/calendar/includes/constants.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 1 | WARNING | A file should declare new symbols (classes, functions,
   |         | constants, etc.) and cause no other side effects, or it should
   |         | execute logic with side effects, but should not do both. The
   |         | first symbol is defined on line 18 and the first side effect is
   |         | on line 11.
--------------------------------------------------------------------------------

Der entsprechende Ausschnitt der constants.php:

Code: Alles auswählen

<?php
/**
*
* @package hjw calendar Extension
* @copyright (c) 2020 hjw
* @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License v2
*
*/
if (!defined('IN_PHPBB'))
{
(Zeile 11)	exit;
}

namespace hjw\calendar\includes;

if (!defined('CALENDAR_TABLE'))
{
(Zeile 18)	define('CALENDAR_TABLE', $this->table_prefix . 'calendar');
}if (!defined('CALENDAR_PARTICIPANTS_TABLE'))
{
	define('CALENDAR_PARTICIPANTS_TABLE', $this->table_prefix . 'calendar_participants');
}
if (!defined('CALENDAR_EVENT_TABLE'))
{
	define('CALENDAR_EVENT_TABLE', $this->table_prefix . 'calendar_event');
}
if (!defined('CALENDAR_EVENT_LIST_TABLE'))
{
	define('CALENDAR_EVENT_LIST_TABLE', $this->table_prefix . 'calendar_event_list');
}
if (!defined('CALENDAR_SPECIAL_DAYS_TABLE'))
{
	define('CALENDAR_SPECIAL_DAYS_TABLE', $this->table_prefix . 'calendar_special_days');
}
if (!defined('CALENDAR_FORUMS_TABLE'))
{
	define('CALENDAR_FORUMS_TABLE', $this->table_prefix . 'calendar_forums');
}
if (!defined('FOOTB_MATCHES_TABLE'))
{
	define('FOOTB_MATCHES_TABLE', $this->table_prefix . 'footb_matches');
}
if (!defined('FOOTB_TEAMS_TABLE'))
{
	define('FOOTB_TEAMS_TABLE', $this->table_prefix . 'footb_teams');
}

Ich weiß nicht, wie ich den Code (Zeile 17 - 20) so gestalte, damit ich keine Warnung bekomme. Jemand eine Idee?

[LukeWCS]

Gut das du das ansprichst, denn über genau diese Warnung bin ich bei EC auch schon gestolpert. Und zwar habe ich mal mein eigenes Ext Check durch Ext Check prüfen lassen. Genau dabei wurde mir dann die gleiche Warnung ausgegeben.

Ich bin da "noch" nicht so ganz schlau draus geworden. Im Prinzip geht es wohl darum, das man den (Haupt)-Programmcode von den Funktionen getrennt notieren soll. Gelöst habe ich es also dadurch, dass ich den Haupt-Code von den Funktionen getrennt habe und diese in einer eigenen Datei ausgelagert habe, welche ich anschliessend mit require_once wieder einbinde.

Bei dir handelt es sich ja aber um eine phpBB Ext und deswegen dürfte sich meine Methode vermutlich nicht direkt auf Calendar anwenden lassen. Was wir hier bräuchten, wäre ein PHP Fachmann wie gn#36. Ich schau mal, ob ich ihn überreden kann, sich das anzuschauen.

Auf der anderen Seite: es handelt sich "lediglich" um eine Warnung. Ich weiss nicht, ob wir das zwingend lösen/beheben müssen. Trotzdem wäre es natürlich hilfreich, wenn wir wüssten was es mit dieser Regel auf sich hat und was das für Auswirkungen haben "könnte", wenn wir diese Warnung ignorieren.

edit: Ich denke das ich bei meiner Lösung das eigentliche Problem lediglich umgangen, aber nicht gelöst habe. Denn es ist mir ein Rätsel, warum ein ausgelagerter Code der mit require_once wieder eingebunden wird, einen Unterschied machen soll.

[LukeWCS]

Wie ich gestern im Log gesehen habe, kann es u.U. dazu kommen, das schwerwiegende Fehler bei EPV im Nirvana landen. Das sind Fehler die verhindern das eine Prüfung überhaupt ausgeführt werden kann. Prinzipiell kann das nur bei EPV passieren, weil dieses Tool das einzige ist, welches auf eine ganz bestimmte Datei im Ext Archiv zwingend angewiesen ist, nämlich auf die composer.json. Wenn nun diese Datei selber schwere Fehler (Strukturfehler) hat, ist EPV nicht in der Lage die Prüfung der Ext durchzuführen und es erscheint in der Übersicht ein grauer Haken bei EPV, während im Report lediglich das folgende steht:

Running Extension Pre Validator on directory ext.

Ich habe das Ganze inzwischen nachgestellt und kann den Fehler reproduzieren. Ich suche jetzt nach einer Lösung dafür. Wenn ihr bei einer Prüfung auf das gleiche Problem stossen solltet, also ein grauer Haken bei EPV angezeigt wird und nur die oben gezeigte Zeile im EPV Report stehen sollte, dann hebt das hochgeladene ZIP bitte auf und lasst es mir bei Gelegenheit zukommen.

edit: Screen hochgeladen, damit man sieht, wie sich der besagte Fehler darstellt: [ externes Bild ]

[Dr.Death]

Zum Calender von HJW:

Anstelle mit den alten DEFINE Techniken zu arbeiten kannst du deine verwendeten Tabellen auch in der service.yml bekannt machen:

Erste Zeile in der service.yml:

Code: Alles auswählen

imports:
    - { resource: tables.yml }

Dann eine Datei tables.yml anlegen mit folgendem Inhalt:

Code: Alles auswählen

parameters:
    tables.calendar: 			'%core.table_prefix%calendar'
    tables.calendar_participants:	'%core.table_prefix%calendar_participants'
    tables.calendar_event: 		'%core.table_prefix%calendar_event'
    tables.calendar_event_list:		'%core.table_prefix%calendar_event_list'
    tables.calendar_special_days:	'%core.table_prefix%calendar_special_days'
    tables.calendar_forums:		'%core.table_prefix%calendar_forums'
    tables.footb_matches:		'%core.table_prefix%footb_matches'
    tables.footb_teams:			'%core.table_prefix%footb_teams'

Du müsstest jetzt natürlich noch jede einzelne Tabelle in eine Variable packen.... z.B. so:

Code: Alles auswählen

$calendar_table	= $this->phpbb_container->getParameter('tables.calendar');

oder

Code: Alles auswählen

$calendar_table	= $phpbb_container->getParameter('tables.calendar');

Die SQL Anweisungen ALT anpassen (suchen und ersetzen der alten DEFINE Variablen:

Code: Alles auswählen

		$sql = 'SELECT *
				FROM ' . CALENDAR_TABLE . '
				WHERE post_id = ' . $post_id;

Neu:

Code: Alles auswählen

		$sql = 'SELECT *
				FROM ' . $calendar_table . '
				WHERE post_id = ' . $post_id;

[HJW]

Danke Doc,

werde ich machen.

[LukeWCS]

Neue Version mit dem Fix für die Ausnahmefehler bei EPV plus einem weiteren Fix ist online. Details sind wie immer im Changelog zu finden. Danke an chris1278 für die Infos.

[gn#36]

Ich würde sagen, die Lösung vom Doc ist gut, alternativ kannst du die neu definierten Parameter auch über den Konstruktor an deine Klasse weiterreichen, genau wie das Datenbankobjekt und ähnliche andere Abhängigkeiten: (Beispiel aus https://github.com/gn36/phpbb-ext-hooku ... ter/config)

Code: Alles auswählen

imports:
    - { resource: parameters.yml }

services:
    # ------- HELPER/FUNCTIONS ------
    gn36.hookup.functions.hookup:
        class: gn36\hookup\functions\hookup
        arguments:
            - '@dbal.conn'
            - '%tables.gn36.hookup.members%'
            - '%tables.gn36.hookup.dates%'
            - '%tables.gn36.hookup.available%'

Die Klasse gn36\hookup\functions\hookup hat dann so einen Konstruktor:

Code: Alles auswählen

__construct($db, $table_members, $table_dates, $table_available)

Die parameters.yml ist wie vom doc beschrieben, die Tabellennamen bekommst du dann im Konstruktor übergeben und kannst sie dir so wie du möchtest zwischenspeichern um sie dann in der Klasse weiterzuverwenden.

edit: Ich denke das ich bei meiner Lösung das eigentliche Problem lediglich umgangen, aber nicht gelöst habe. Denn es ist mir ein Rätsel, warum ein ausgelagerter Code der mit require_once wieder eingebunden wird, einen Unterschied machen soll.

Letztlich ist die Warnung vom Codesniffer dazu gedacht, den Entwickler vor Angriffspunkten in seinem Code zu schützen und außerdem Überraschungen bei der Wiederverwendung des Codes zu vermeiden. Ersteres macht auch das

Code: Alles auswählen

if (!defined('IN_PHPBB'))
{
	exit;
}

was in älteren phpBB Versionen praktisch überall verwendet wurde.

Hauptsächlich problematisch sind dabei Konstrukte, die man früher mal öfters gesehen hat:

Code: Alles auswählen

<?php
class abc 
{
    function __construct() {}
    function a() {}
}
// Neue Klasse auch gleich als Objekt anlegen zum verwenden:
$a = new abc();

Wenn du das jetzt in einer bestehenden Datei verwendest kann das Probleme verursachen:

Code: Alles auswählen

<?php
$a = 1;
include "dateivonoben.php";
$b = new abc();
$a++; // $a ist jetzt ein Objekt, kein int mehr...

Das ist natürlich konstruiert, aber wenn man z.B. zwei include Dateien hat, die beide die selbe Variable befüllen geht es genauso schief. Angreifbar wäre es evtl. dann, wenn die Nebeneffekte z.B. in die Datenbank schreiben würden oder Dateien anlegen/löschen. Hier kann es dann leicht passieren, dass die Absicherung gegen den direkten Aufruf im Browser fehlt. Da register globals inzwischen nicht mehr aktiv ist ist die Gefahr zwar kleiner geworden, aber 0 ist sie nicht, wenn das Skript evtl. Daten direkt aus den Superglobalen verwendet.

Die Definitionen von direkt ausgeführtem Code zu trennen ist insbesondere dann wichtig, wenn du Autoloading verwendest (also PHP bei der ersten Verwendung einer Klasse diese für dich automatisch läd statt dass du irgendwo includes einfügen musst). Autoloading mit Nebeneffekten in der automatisch eingebundenen Datei kann zu unerwartetem Verhalten führen.

Wenn du dir beispielsweise vorstellst, dass die obige Datei per Autoloading eingebunden wird, dann tut

Code: Alles auswählen

$a = 1;
$b = new abc(); // hierfür wird jetzt die Datei oben automatisch vor dieser Zeile eingebunden
echo $a; // Oh je, $a ist jetzt ein Objekt... -> Fehler

etwas anderes als

Code: Alles auswählen

$b = 1;
$c = new abc(); // Wieder wird die Datei oben hiervor automatisch eingebunden
echo $b; // Kein Problem, denn in der Datei schreiben wir ja $a, $b ist also wie vorher...

obwohl ich nur Variablennamen geändert habe.

Da auch phpBB Autoloading verwendet ist es ratsam, sich an diese Trennungsregel zu halten.

[LukeWCS]

@gn#36

Erstmal danke an dich und Doc!

Ach damit hat das zu tun, also Codehärtung. In diese Richtung habe ich schlicht nicht gedacht.

Das heisst auch, das wir die CS Warnung keinesfalls ignorieren dürfen, sondern lösen müssen.

Dann wäre meine Trennung die ich durchgeführt habe, ja durchaus richtig. Mir war bis eben einfach nur nicht klar, warum der Code von den Funktionen getrennt werden muss/soll. Ich komme beruflich aus einer ganz anderen Ecke bezüglich Programmierung, da habe ich mit PHP nichts zu tun. Und daher kenne ich auch solche Besonderheiten nicht.

Okay, ich bin schon ziemlich müde und muss mir deine Antwort morgen bei nem Pott Kaffee nochmal genau durchlesen und auch ein paar Dinge ausprobieren was du geschrieben hast. Und mit "Autoloading" hatte ich auch erst jetzt bei der Entwicklung von EC das erste Mal zu tun, nämlich für das Tool "AnsiConverter". Ich verwende dafür vendor/autoload.php im EPV Ordner, denn das ist ja eh schon vorhanden.

[LukeWCS]

Im Startbeitrag gibt es jetzt einen eigenen Abschnitt, der sich den Abweichungen gegenüber dem Travis CI Dienst widmet. Auch sonst wurden kleinre Dinge überarbeitet. Screenshot der Report Demo auf den aktuellen Stand von EC gebracht.

[Mike-on-Tour]

Durfte die Ergebnisse des ExtCheck ja auch nutzen und bin begeistert, hier bekommt man konzentriert und übersichtlich alles angezeigt, was EPV und CodeSniffer an Fehlern und Warnungen ausgeben und kann sich dann ans Abarbeiten der notwendigen Korrekturen machen.
Super Arbeit !! (Hier fehlt ein "Daumen hoch"-Smilie in der Auswahl, sonst würde ich den hier setzen)