Add Namespace Autoloader

[pifou25]

Description

Bonjour,
Je propose d'ajouter la gestion de namespaces via l'autoloader de Composer. Le namespace permet d'éviter des conflits de classes qui auraient le même nom, en particulier avec tous les plugins. Cela permettra à chaque plugin de faire ce qu'il veut dans son propre namespace. Et autre avantage, toutes les classes des plugins seront automatiquement chargées.
En plus, la fonction autoloader est grandement simplifiée

classmap: scanne automatiquement les répertoires indiqués et ajoute les classes détectées dans un autoloader vendor/composer/autoload_classmap.php
files: ajoute tous les fichiers requis sur chaque requête
psr-4: comme détaillé par kwizer un peu plus bas ça permettra de déplacer les classes du core dans de nouvelles classes namespacées, dans /src avec un namespace Jeedom\Core ... Et pour chaque plugin qui le souhaite d'utiliser son propre namespace, il suffit pour cela de créer nos classe dans le namespace suivant: Jeedom\plugins\idplugin\src par exemple :)

Point d'attention:

• dans translate.class.php on a une fonction en dehors de la classe (la fameuse fonction __ !! ) je la déplace donc dans utils.inc.php
• dans files il faut placer utils.inc.php avant jeedom.config.php car ce fichier de config utilise la fonction de traduction __
• l'autoloader spécifique jeedom n'est plus utile que pour la classe *Cmd de chaque plugin, que Composer ne voit pas forcément s'il est dans le même fichier qu'une autre classe.
• il faudrait refaire un composer dump-autoload pour regénérer l'autoloader après chaque installation / suppression de plugin

Il y a encore plein de code php en dehors de toute classe, qui gagnerait à être déplacé dans de nouvelles classes. Ceci est géré par la méthode include_file dans utils.inc.php ça ne change pas.

Suggested changelog entry

migration to Composer autoloader

Related issues/external references

From #2910
Fixes #3108

[kwizer15]

Salut, quelques soucis sur ce shell_exec :

1. $WEBSERVER_HOME entre guillemets simples n'est pas interprété par PHP. C'est le shell qui va essayer de lire la variable d'environnement, mais elle n'est pas définie côté Apache/nginx au runtime (seulement pendant install.sh). Donc --working-dir sera vide et composer tournera au mauvais endroit.
2. Il manque COMPOSER_ALLOW_SUPERUSER=1, comme dans system.class.php:432, sinon composer peut râler en root.
3. Si composer plante, on ne saura pas pourquoi (pas de 2>&1, pas de try/catch).

Je te propose ça :

Suggested change

	// re generate composer autoloader
	$result = shell_exec(system::getCmdSudo() . ' composer dump-autoload --optimize --working-dir $WEBSERVER_HOME');
	log::add(__CLASS__, 'debug', "Composer autoloader dump result:\n$result");
	try {
	$workingDir = realpath(__DIR__ . '/../..');
	$cmd = 'export COMPOSER_ALLOW_SUPERUSER=1; '
	. system::getCmdSudo() . ' composer dump-autoload --optimize --working-dir '
	. escapeshellarg($workingDir) . ' 2>&1';
	$result = shell_exec($cmd);
	log::add(__CLASS__, 'debug', "Composer autoloader dump result:\n" . (string) $result);
	} catch (Throwable $e) {
	log::add(__CLASS__, 'error', 'Composer dump-autoload failed: ' . $e->getMessage());
	}

Comme ça : chemin calculé en PHP, path échappé, erreurs capturées, et si ça plante ça casse pas la suite de l'update.

[kwizer15]

Les 3 catch (Throwable $e) ajoutés loguent tous le même message 'Log (level|timezone) configuration failed', y compris celui dans jeedomAutoload() qui charge des classes de plugins — rien à voir avec la config log/timezone. Ça va embrouiller le debug le jour où il y a un souci.

En plus, appeler log::add() ici est risqué : on est en plein bootstrap, la config log n'est pas forcément encore chargée, et si log::add plante à son tour on part en récursion. Mieux vaut error_log() (qui écrit dans le log PHP système) pendant cette phase.

Proposition pour les 3 blocs :

Ligne 27-29 (timezone) :

} catch (Throwable $e) {
    error_log('Jeedom: timezone configuration failed: ' . $e->getMessage());
}

Ligne 35-37 (log level) :

} catch (Throwable $e) {
    error_log('Jeedom: log level configuration failed: ' . $e->getMessage());
}

Ligne 59-61 (autoload plugin) :

} catch (Throwable $e) {
    error_log('Jeedom: plugin class autoload failed for ' . $classname . ': ' . $e->getMessage());
}

[kwizer15]

Deux petites choses :

1. Le catch (Exception $e) ne rattrape pas les Error (ex. erreur fatale côté DB). Dans le reste de ta PR tu es passé à Throwable, ce serait cohérent de faire pareil ici.
2. Si le catch avale silencieusement, on ne saura jamais qu'on tourne avec une config log vide → très pénible à diagnostiquer.

Proposition :

Suggested change

	try {
	self::$config = array_merge(config::getLogLevelPlugin(), config::byKeys(array('log::engine', 'log::formatter', 'log::level', 'addMessageForErrorLog', 'maxLineLog', 'maxSizeLog')));
	} catch (Exception $e) {
	self::$config = array();
	}
	try {
	self::$config = array_merge(config::getLogLevelPlugin(), config::byKeys(array('log::engine', 'log::formatter', 'log::level', 'addMessageForErrorLog', 'maxLineLog', 'maxSizeLog')));
	} catch (Throwable $e) {
	self::$config = array();
	error_log('Jeedom: log::getConfig failed, using empty config: ' . $e->getMessage());
	}

[kwizer15]

Un point gênant sur common.config.php dans files : en mode Docker, ce fichier n'existe pas au moment du docker build.

Le Dockerfile enchaîne step 9 puis step 10 de install.sh. Or step 9 (la copie common.config.sample.php → common.config.php) est skip en mode docker (install.sh:307, if [ "${INSTALLATION_TYPE}" != "docker" ]). Elle est faite plus tard par install/OS_specific/Docker/init.sh:98 — mais c'est au démarrage du conteneur, pas au build.

Du coup step 10 (qui lance composer install) va planter au build de l'image officielle parce que composer ne trouvera pas le fichier listé dans files.

Même souci pour le workflow CI, c'est d'ailleurs pour ça que tu as dû patcher phpstan.yaml.

Je propose de sortir common.config.php de files et de garder un require_once explicite dans core.inc.php avant vendor/autoload.php :

   "files": [
-     "core/config/common.config.php",
      "core/php/utils.inc.php",
      "core/config/compatibility.config.php"
   ],

 date_default_timezone_set('Europe/Brussels');
+require_once __DIR__ . '/../config/common.config.php';
 require_once __DIR__ . '/../../vendor/autoload.php';

Comme ça tu peux aussi annuler la modif du workflow phpstan.yaml.

[Mips2648]

Juste pour gérer les attentes: je pense que que comme dit sur la PR précédente (#2910 (comment)), c'est peut-être une bonne idée à investiguer mais à ma connaissance l'issue n'a pas encore été validée par l'équipe donc ne vous attendez pas à ce que ceci soit merge rapidement,
A mon avis il y a d'autres prio à mettre en place avant.

Et faire une PR (pull REQUEST) ne veut pas obligatoirement dire qu'un jour ca sera validé, il est toujours possible que la réponse soit "non" ou "plus tard" etc

Ne soyez donc pas déçus ni lassés qu'elle reste longtemps en l'état et qu'il y ait du boulot pour la maintenir à jour avec le core qui évoluera entre temps...
Si vous voulez pas faire du boulot inutile, attendez le feu vert ;-)

[pifou25]

Déçu mais je persiste :) comme ma PR était sur la mauvaise branche je l'ai juste refaite sur la nouvelle branche develop, ça ne me coutait pas cher.
C'est un débat de fond, il faudrait avancer, le sujet est revenu plusieurs fois sur le forum (ici et là par exemple) mais personne ne prend la peine de répondre, alors que ça n'a que des avantages. On peut déjà statuer sur l'issue #3108 , pour savoir dans quelle direction on souhaite avancer. Ma solution a l'avantage de ne pas être destructrice, ça reste compatible avec l'existant, mais on peut aussi aller vers quelque chose de plus radical pour avoir plus de liberté.

[kwizer15]

Je suis assez d'accord avec @pifou25 , je pense que le sujet mériterai d'être abordé et discuté rapidement.