Guidelines de développement PHP

Edit me

Introduction

La pratique de la revue de code est répandue chez Epiconcept. Cet article à pour but d’homogénéiser les pratiques des différentes équipes sur le sujet.

Les guidelines ci-dessous sont issues de réunions. Elles n’ont pas l’ambition d’être des vérités absolues. Leur objectif est simplement de s’assurer qu’une même pratique sera acceptée ou refusée de la même manière dans toutes les équipes lors de la revue de code.

Guidelines sur les commentaires

Les PHPDoc de fichiers, classes et méthodes

On parle de ça par exemple :

/**
* Démarre une session de codage.
*
* @param boolean $intensive Signale si la session est intesive.
* @param MyObject $myObject Instance de MyObject
*
* @throws MyBiduleNotFoundException
*
* @return array Un tableau de résultats
*/
public function startCodingSession($intensive, MyObject $myObject)
{
  ...
}

Ces commentaires servent à l’autocomplétion dans les IDE.

Les commentaires dans le code

On parle de ça par exemple :

// Vérification que le dataset demandé correspond bien au dataset renvoyé par le provider
if ( $oDataset->Id() !== $sDatasetId ) {
  throw new Core_Library_Exception( sprintf( 'Provider dataset id mismatch (%s != %s)!', $oDataset->Id(), $sDatasetId ) );
}

Le code commenté

On parle de ça par exemple :

/*
$iMailingModelId = $this->getRequest()->getParam( 'id_mailingmodel' );
if ( $iMailingModelId === null ) {
  if ( isset( $this->GetProject()->Account()->Session()->iMailingModelId ) ) {
    $iMailingModelId = $this->GetProject()->Account()->Session()->iMailingModelId;
  } else {
    throw new Core_Library_Exception( 'Parameter id_mailingmodel not found!' );
  }
}
*/

Soit le code est utile et doit être décommenté, soit il ne l’est pas et il doit être supprimé. Le laisser commenté laisse planer un doute. Et de plus, on a un outil de gestion de version pour gérer les suppressions de code et leur revert si nécessaire.

Guidelines sur les errorlog

On parle de ça :

foreach ( $aEmails as $sEmail ) {
  error_log($sEmail . " " . $titre);
  ...
  }
}

On distingue trois types de logs :

  • Les logs d’erreurs : issus du catch d’une exception par exemple
  • Les logs d’activités : plus métiers (envoi d’un mail, appel à un service externe, etc.)
  • Les logs de debug : utiles pendant le développement mais pas sur les serveurs

Les logs d’erreur

Exemple :

error_log( print_r( sprintf( 'Erreur de reconciliation à la cloture de la lecture %s : ', $oLecture1->GetDbId() ) . $ex->getMessage(), true ) );

Les logs d’activités

Exemples :

error_log( sprintf( 'Envoi du mail %s à l’adresse %s', $sSubject, $sAddress) ), 3, Core_Library_Options::get('voozanoo.activity.log.directory') . "/email.log" );
error_log( sprintf ( 'Lancement de l’import depuis le LDAP pour l’identifiant %s', $iIdLdap ), 3, Core_Library_Options::get('voozanoo.activity.log.directory') . "/ldap_import.log" );

Les logs de debug

Guidelines sur les @todo

On parle de ça :

/**
 * function datafunction
 * @todo handle exception properly
 */
function datafunction()
{
  ...
}

En effet :

  • Si le @todo n’est pas compréhensible, il faut le retirer.
  • Si le @todo est dans le périmètre du ticket, il faut faire le développement avant de faire la PR (car laisser un @todo voudrait dire que le travail n’est pas terminé).
  • Si le @todo est hors du périmètre du ticket, il faut créer un ticket autre Jira pour que ce soit planifié.

Guidelines sur l’usage des constantes

On parle de ça :

class Ma_Classe
{
  const VARSET_USER = 'user';
  const VARSET_PAT  = 'pat';
  const VARSET_CAS  = 'cas';
  
  ...
}

Elles permettent d’éviter de répéter une même valeur plein de fois dans le code (risque d’erreur et manque de factorisation).

Il n’y a pas de guidelines claires sur la préférence à avoir entre des constantes comme celles-ci et l’utilisation de propriétés de classe, mais quoi qu’il en soit :

Guidelines sur les méthodes statiques

On parle de ça :

class Foo_Helper
{
  public static function Bar()
  {
    ...
  }
  
  ...
}

Ces méthodes permettent de ne pas instancier leur classe, mais sont souvent un raccourci vers de la conception “moins” orientée objet. Elles peuvent être justifiées par des questions de performances ou de lisibilité du code, mais ne sont globalement pas recommandées.

Plus précisement :

Guidelines sur l’usage des classes de Zend pour l’accès aux données

On parle de ça :

$oProject = Core_Library_Account::GetInstance()->GetCurrentProject() ;
if ( $sValidationType == 'T' ) {
  $oProject->Db()->update( "sigl_04_data", array("statut" => 254 ), "id_data = " . $iIdAnalysis ) ;
}
elseif( $sValidationType == 'B' ) {
  $oProject->Db()->update( "sigl_04_data", array("statut" => 256 ), "id_data = " . $iIdAnalysis ) ;
}

Cette façon de procéder, que ce soit pour un INSERT, un UPDATE, ou autre, est globalement interdite.

En effet, contrairement aux méthodes du noyau comme SimpleUpdateData(), elle présente deux problèmes :

  • les droits de l’utilisateur courant ne sont pas appliqués ;
  • les actions faites de cette manière ne sont pas tracées dans les tables evtlog et varsetmonitor de Voozanoo 4.

Or, ces deux points sont des engagements que nous avons vis-à-vis de nos clients et des autorités.

Ainsi :