Différents points concernant le code

Michel_F · Août 18, 2024, 6:54

Bonjour,

Il y a quelques jours je me suis permis de faire un PR avec des corrections de fautes de français dans le core ainsi que des reformulations pour remplacer des textes compliqués à comprendre. Bref, j’ai fait un tour non exhaustif (et de loin !) du code. Durant ce périple, je suis tombé sur des choses pour lesquelles je me suis posé la question : « je fais un PR pour ça ou c’est voulu ? ». Je ne veux pas casser quelque chose et générer des effets de bord involontaires.

Attention, je pinaille !

1. Autorisation, droit et droits

En fonction des cas, on peut lire les messages d’exception suivants :

Vous n’avez pas les droits de faire cette action
Vous n’êtes pas autorisé à …
Vous n’avez pas le droit …

Pour le 1. et le 3. :on ne dit pas « avoir les droits de … » mais « avoir le droit de … » ou « avoir les droits pour … ».
Pour le 2. : je n’en suis pas un fan, mais si l’utilisateur est une utilisatrice, elle aimerait peut-être lire « Vous n’êtes pas autorisée à … ». Peut-être faudrait-il utiliser la forme « autorisé(e) »

Quelle distinction est faite entre « avoir le droit de … » « avoir les droits pour … » et « être autorisé(e) » ?

2. Fonction de traduction utilisée

Dans tous les fichiers .php du répertoire desktop/php/ au début on trouve ce code :

<?php
if (!isConnect('admin')) {
	throw new Exception('{{401 - Accès non autorisé}}');
}

Sauf pour desktop/php/reboot.php et desktop/php/shutdown.php ou on a :

<?php
if (!isConnect('admin')) {
	throw new Exception(__('401 - Accès non autorisé', __FILE__));
}

Quelle est la fonction de traduction à utiliser ?

3. Jeedom ou `config::byKey('product_name')`

Il est rare de trouver le nom « Jeedom » écrit en dur, une référence est faite sur config::byKey('product_name') comma dans desktop/php/administration.php par exemple. Or dans desktop/php/interact.php ligne 233, on peut voir que « Jeedom » est écrit en dur :

<label class="col-xs-2 control-label">{{Autoriser Jeedom à supprimer les demandes syntaxiquement incorrectes}}</label>

4. Traductions avec un terme remplacé par une variable

Exemple desktop/php/administration.php ligne 58 :

<label class="col-xs-4 control-label">{{Nom de votre}} <?php echo $productName; ?>

Comme on le sait tous, l’ordre des mots n’est pas forcément le même dans toutes les langues et si déjà on s’est évertué à prévoir des traductions autant tenir compte de cet état de fait. Ne faudrait-il pas utiliser une syntaxe du style :

<label class="col-xs-4 control-label"><?php echo sprintf(__("Nom de votre %s", __FILE__), $productName); ?>

Et ce afin que les traductions mettre ce « %s » là où il doit être ? J’ai même vu des traductions en 2 fois (je n’ai pas d’exemple) du type (exemple faut où il manque les espaces, mais c’est pour la forme) :

<label class="col-xs-4 control-label">{{Nom de votre}} <?php echo $productName; ?> {{préféré}}

5. Version php vérifiée : < 5.5 !!

core/api/proApi.php lignes 152 à 159

github.com

jeedom/core/blob/2961a3d646ef3eede77b2e877dec8335d217f2b8/core/api/proApi.php#L152C1-L159C130


      
          			$defaut = 0;
          			$result = phpversion();
          			$advice = '';
          			if (version_compare(phpversion(), '5.5', '<')) {
          				$defaut = 1;
          				$advice = __('Si vous êtes en version 5.4.x, on vous indiquera quand la version 5.5 sera obligatoire', __FILE__);
          			}
          			$health[] = array('plugin' => 'core', 'type' => 'Version PHP', 'defaut' => $defaut, 'result' => $result, 'advice' => $advice);

version php < 5.5 ? C’est pourtant la version alpha de Jeedom qui requière php 7.4+

6. Code fonctionnel ?

Pour ce point je ne suis pas trop sûr de moi, je suis tombé sur ces bouts de code par hasard. J’ai hésité à faire une issue sur le repo github, mais j’en parle préférablement ici.
core/api/jeeApi.php lignes 908 à 924

github.com

jeedom/core/blob/2961a3d646ef3eede77b2e877dec8335d217f2b8/core/api/jeeApi.php#L900C1-L925C3


      
          	if ($jsonrpc->getMethod() == 'scenario::changeState') {
          		$scenario = scenario::byId($params['id']);
          		if (!is_object($scenario)) {
          			throw new Exception(__('Scénario introuvable :', __FILE__) . ' ' . secureXSS($params['id']), -32702);
          		}
          		if (is_object($_USER_GLOBAL) && !$scenario->hasRight('w', $_USER_GLOBAL)) {
          			throw new Exception(__('Vous n\'avez pas les droits sur ce scénario', __FILE__), -32701);
          		}
          		if ($params['state'] == 'stop') {
          			$jsonrpc->makeSuccess($scenario->stop());
          		}
          		if ($params['state'] == 'run') {
          			$scenario->addTag('trigger','api');
          					$scenario->addTag('message',__('Scénario exécuté sur appel API', __FILE__));
          			$jsonrpc->makeSuccess($scenario->launch());
          		}
          		if ($params['state'] == 'enable') {
          			$scenario->setIsActive(1);
          			$jsonrpc->makeSuccess($scenario->save());
          		}

This file has been truncated. show original

core/api/proApi.php lignes 563 à 584

github.com

jeedom/core/blob/2961a3d646ef3eede77b2e877dec8335d217f2b8/core/api/proApi.php#L563C1-L585C4


      
          		if ($jsonrpc->getMethod() == 'scenario::changeState') {
          			$scenario = scenario::byId($params['id']);
          			if (!is_object($scenario)) {
          				throw new Exception('Scénario introuvable : ' . secureXSS($params['id']), -32702);
          			}
          			if ($params['state'] == 'stop') {
          				$jsonrpc->makeSuccess($scenario->stop());
          			}
          			if ($params['state'] == 'run') {
          				$scenario->addTag('trigger','api');
          				$scenario->addTag('message',__('Scénario exécuté sur appel API', __FILE__));
          				$jsonrpc->makeSuccess($scenario->launch());
          			}
          			if ($params['state'] == 'enable') {
          				$scenario->setIsActive(1);
          				$jsonrpc->makeSuccess($scenario->save());
          			}
          			if ($params['state'] == 'disable') {
          				$scenario->setIsActive(0);
          				$jsonrpc->makeSuccess($scenario->save());

This file has been truncated. show original

Je pense que dans ces 2 cas, il faudrait un switch/case sur $params['state'] avec l’exception dans default parce qu’avec ce code l’exception est toujours levée. D’ailleurs si personne ne s’en est plaint, c’est que cet appel API n’est pas vraiment utilisé… ou alors j’ai mal compris le code.

Voilà !

A+
Michel

Phpvarious · Août 18, 2024, 9:59

Hello,

Je vais essayé de répondre a certaines questions, car certains points ont révélés des bugs.

Je sais pas .
sur les accès fichiers on a tendance a dire « des droits », ca vient peut-être de là.
Mon terme préféré pour uniformiser serai : Vous n’êtes pas autorisé à effectuer cette action

L’utilisation de {{ }} est plus souvent réservé pour le html/js, mais il existe des cas ou cela peut-être utilisé en php :

github.com

jeedom/core/blob/2961a3d646ef3eede77b2e877dec8335d217f2b8/desktop/php/object.php#L14-L19


      
          $synthToActions = array(
          	'synthToDashboard' => '{{Dashboard}}',
          	'synthToView' => '{{Vue}}',
          	'synthToPlan' => '{{Design}}',
          	'synthToPlan3d' => '{{Design 3D}}',
          );

en revanche, je vient de tester et que ce soit en utilisant __() ou {{ }}, la trad ne semble pas passer sur un throw new Exception du début d’un fichier. (ex: log.php)

« Jeedom » étant une marque blanche, je suppose que c’est un oublie.

if (version_compare(phpversion(), '5.5', '<')) {
Comparer la version php a la version de Jeedom… Sans doute un erreur, je pense que ce advice est censé être dans la partie jeedom::version()

Non car a chaque appel a makeSuccess le code s’arrête :

github.com

jeedom/core/blob/alpha/core/class/jsonrpc.class.php#L86


      
          			'jsonrpc' => '2.0',
          			'id' => $this->id,
          			'result' => $_result,
          		);
          		$return = array_merge($return, $this->getAdditionnalParams());
          		if (init('callback') != '') {
          			echo init('callback') . '(' . json_encode($return) . ')';
          		} else {
          			echo json_encode($return);
          		}
          		exit;
          	}
          	
          	/*     * ********Getteur Setteur******************* */
          	
          	public function getStartTime() {
          		return $this->startTime;
          	}
          	
          	public function getApplicationName() {
          		return $this->applicationName;

Michel_F · Août 20, 2024, 12:31

Salut,

Merci pour ton retour.

Je vais proposer quelque chose, les commentaires et remarques se feront dans le PR.
Je laisse ça à un spécialiste si un spécialiste juge que ce problème vaut le coup d’être corrigé. Je fais une issue, on verra bien.
Je vais faire un PR.
Je vais faire une issue.
Je vais faire une issue.
Merci d’avoir regardé de plus près. Rien à faire pour ce point, c’est moi qui n’ai pas regardé assez loin.

A+
Michel

Différents points concernant le code

1. Autorisation, droit et droits

2. Fonction de traduction utilisée

3. Jeedom ou config::byKey('product_name')

4. Traductions avec un terme remplacé par une variable

5. Version php vérifiée : < 5.5 !!

6. Code fonctionnel ?

3. Jeedom ou `config::byKey('product_name')`