avis sur code MVC

Version imprimable

avis sur code MVC

Bonjour à tous,
j'aimerai avoir votre avis sur mon code avant que je ne continue plus loin pour savoir si j'ai mal assimilé les concepts de la POO et du MVC.

J'ai un routeur qui appel mes controlleurs et action de ce type:
Code:

1 2 3 4 5 6 7 8 9 10 11 DEFINE("GET",$_GET['url']??'/identification'); $router = New \Game\Router\Router(GET); $router->get('/',["action"=>"Game\Controllers\HomeController@show"]); $router->get('/identification',["action"=>"Game\Controllers\UsersController@login"]); $router->get('/deconnexion',["action"=>"Game\Controllers\UsersController@logout"]); $router->post('/identification',[ "action"=>'Game\Controllers\UsersController@login', "post"=>$_POST ]); $router->run();
Si j'ai bien compris le concept, le contrôleur se charge de faire la liaison entre les modelés et la vue et de ce fait, il ne doit pas y avoir du tout de connexion à la base de donnée dans ma vue. J'ai donc un contrôleur principal qui se charge de passer les données à ma vue via la méthode render()
et notamment avec un extract($datas)
Code:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 class Controller { private $viewPath; public function __construct($viewPath) { $this->viewPath = $viewPath; } protected function render(string $page, array $datas=[]) { ob_start(); extract($datas); require "../Game/Views/".$this->viewPath."/".$page.".php"; $content=ob_get_clean(); require "../Game/Views/template/layout.php"; } }
Pour ma partie identification par exemple, j'envoie donc les données postées par l'utilisateur à mon UserControlleur qui se charge d'appeler le UserManager pour vérifier la combinaison login/pass et de register ma session si les infos correspondent. ensuite ce contrôleur envoie juste à ma vue l'information pour dire que la vérification login/passe a réussi ou échoué (via $check qui est true ou false).
Code:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 class UsersController extends Controller { public function __construct($viewPath='user') { parent::__construct($viewPath); } public function login(array $post=null) { if($post) { $UserManager = \Game\classes\users\UserManager::getInstance(); $check=$UserManager->verifPassword($post); if($check) { Session::getSession()->register($post['login']); } return $this->render('login',["title"=>"SCORE JEUX: identification","check"=>$check]); } return $this->render('login',["title"=>"SCORE JEUX: identification"]); } public function logout() { Session::getSession(); $firstname=$_SESSION['firstname']; $this->render('logout',["title"=>"A bientot $firstname"]); } public function show(array $args) { $this->render('show',$args); } }
Du côté de ma vue je n'ai plus qu'a afficher le message correspondant et de rediriger vers une autre page en cas de réussite.
Code:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 echo "<h2>Identification</h2>"; if(isset($check) and $check) { if($check) { echo "<div class='alert success'><h4>Bienvenue !</h4><p>Votre identification a réussi. Veuillez patienter SVP...</p></div>"; header("Refresh:3;url=/"); } else { echo "<div class='alert danger'><h4>Connexion refusée !</h4><p>Votre identification a échoué. Merci de vérifier votre combinaison identifiant/mot de passe SVP...</p></div>"; } } $forms=new \Game\classes\html\Form('','POST','userLogin'); $forms->input('Identifiant:','login','login','text'); $forms->input('Mot de passe:','password','password','password'); foreach($forms->show('Se connecter') as $form) { echo $form; }
.
J'aimerai avoir votre avis sur ce code en terme de sécurité d'une part et en terme de concept POO et MVC.
Ce code fonctionne bien mais comme déjà lu sur les tutos, ce n'est pas parce que ça fonctionne que c'est optimal !
Je n'ai volontairement pas surchargé le message en mettant le code des classes Session, Routeur, Form et UserManager.

D'avance merci pour le temps que vous voudrez bien m'accorder et l'avis constructif que vous pourrez m'apporter.

19/12/2023, 09h08
ProgElecT

Salut

DEFINE :nono:
define:ccool:
19/12/2023, 09h43
unanonyme

Bonjour,

c'est bien. Il y a ce foreach que je ne comprend pas foreach($forms->show('Se connecter') as $form)C'est une instance de Form \Game\classes\html\Form, mais c'est une variable de forms ?

Éventuellement, cela manque d'une classe Application, au dessus du routeur, l'intl, la config, etc

Bon après vous ré inventez des roues, pas d'avis.

Bonne journée.

Bonjour,

Impossible de donner un avis sur la sécurité car on ne voit pas comment les données sont utilisées.
Est-ce que tu utilises des requêtes préparées pour exécuter les requêtes sql avec paramètres ?
Est-ce que les variables php sont bien échappées dans la vue (via htmlspecialchars ou htmlentities) ?
Les formulaires sont-ils protégés contre les failles CSRF ?

Concernant la POO, la méthode UserManager::getInstance() correspond au design pattern singleton : https://refactoring.guru/fr/design-patterns/singleton
C'est un patron de conception pratique mais aussi critiqué car il peut poser des problèmes notamment pour les tests unitaires et/ou dans un environnement multithread.
On recommande alors l'injection de dépendances à la place de l'utilisation de singleton : https://php.developpez.com/tutoriels...de-dependances

Quelques remarques concernant le code :
- Préfères l'utilisation des opérateurs && et || à la place de and et or dans tes conditions php (voir le premier commentaire sur cette doc)
- Ajoute un exit() après une redirection ou un refesh appliqué via header()
- Les conditions if(isset($check) and $check) puis if($check) font doublon, la première condition est suffisante :
Code:

1 2 3 4 5 6 7 if(isset($check) && $check) { // équivalent à if (!empty($check)) echo "<div class='alert success'><h4>Bienvenue !</h4><p>Votre identification a réussi. Veuillez patienter SVP...</p></div>"; header("Refresh:3;url=/"); exit(); } else { echo "<div class='alert danger'><h4>Connexion refusée !</h4><p>Votre identification a échoué. Merci de vérifier votre combinaison identifiant/mot de passe SVP...</p></div>"; }
- la variable intermédiaire $check dans la méthode UsersController::login n'est pas réutilisée et semble donc inutile :
Code:

1 2 3 4 5 $UserManager = \Game\classes\users\UserManager::getInstance(); if($UserManager->verifPassword($post)) { Session::getSession()->register($post['login']); }
- Essaye de typer toutes les propriétés des classes ainsi que les paramètres et valeur de retour des méthodes pour améliorer ton code (utilise par exemple PHPStan pour ne pas en oublier)

20/12/2023, 14h40
laurentSc

Bonjour,
ayant parcouru cette discussion, je suis convaincu que tu aurais fort à gagner à utiliser pour ton affichage (les vues) la classe PhpEcho https://github.com/rawsrc/PhpEcho et pour gérer la bdd (dans le modèle) la classe PDOPlusPlus https://github.com/rawsrc/PDOPlusPlus. Le code de ces 2 classes est d'un niveau très élevé mais leur utilisation nécessite un niveau très faible...
Notamment Pytet parle de la sécurité et des requêtes préparées et tout cela est pris en compte si on utilise PDOPlusPlus.