Critique de syntaxe ou de méthode, j'attend vos retours

22 oct. 2015, 10:12

Bonjour à tous,

Dans le cadre d'un forum scientifique j'ai décidé de me mettre au PHP mais sous PDO, du coup j'aurai voulu avoir vos avis sur ma façon de coder, la syntaxe des requête ou encore les possibles critiques d'un point de vue sécurité sur mes pages.

Je me permets donc de vous laisser ma première page, une page toute simple, la page d'inscription, elle est fonctionnelle je voudrais juste savoir si on peut l'améliorer en PDO php :

<?php session_start();
include_once("includes/connect.php");
include_once("includes/cookieconnect.php");
?>
<!DOCTYPE html>
<html>
<head>
    <meta charset='utf-8' />
    <title></title>
    <meta name="" content="">
    <link rel="stylesheet" type="text/css" href="">
    <link rel="icon" type="image/png" href="" />
</head>
<body>
    <div>
        <?php
        if(isset($_POST['register']))
        {
            $pseudo = $_POST['pseudo'];
            $mail = $_POST['mail'];
            $mdp = sha1($_POST['mdp']);
 
            if(!empty($_POST['pseudo']) AND !empty($_POST['mail']) AND !empty($_POST['mdp']))
            {
                $pseudolength = strlen($pseudo);
 
                if($pseudolength <= 25)
                {
                    if(filter_var($mail, FILTER_VALIDATE_EMAIL))
                    {
                        $reqmail = $bdd->prepare('SELECT * FROM utilisateurs WHERE mail = :mail');
                        $reqmail->execute(array(
                            'mail' => $mail
                            ));
 
                        $mailexist = $reqmail->rowCount();
 
                        if($mailexist == 0)
                        {
                            $reqpseudo = $bdd->prepare('SELECT * FROM utilisateurs WHERE pseudo = :pseudo');
                            $reqpseudo->execute(array(
                                'pseudo' => $pseudo
                                ));
 
                            $pseudoexist = $reqpseudo->rowCount();
                             
                            if($pseudoexist == 0)
                            {
                                $insertmbr = $bdd->prepare('INSERT INTO utilisateurs (pseudo, mail, mdp, avatar, date_inscription) VALUES(:pseudo, :mail, :mdp, :avatar, now())');
                                $insertmbr->execute(array(
                                    'pseudo' => $pseudo,
                                    'mdp' => $mdp,
                                    'mail' => $mail,
                                    'avatar' => "default.jpg"
                                    ));
 
                                $lastInsertId = $bdd->lastInsertId();
                                header("location:index.php?id_user='.$lastInsertId.");
                            }
                            else
                            {
                                $erreur = "pseudo déjà utilisée !";
                            }
                        }
                        else
                        {
                            $erreur = "Adresse mail déjà utilisée !";
                        }
                    }
                    else
                    {
                        $erreur = "Votre adresse mail n'est pas valide !";
                    }
                }
                else
                {
                    $erreur = "Votre pseudo ne doit pas dépasser 25 caractères !";
                }
            }
            else
            {
                $erreur = "Tous les champs doivent être complétés !";
            }
        }
        ?>
        <form action="register.php" method="POST">
        <input type='text' name='pseudo' placeholder='Pseudo'/>&nbsp;
        <input type='mail' name='mail' placeholder='Adresse mail'/>&nbsp;
        <input type='mdp' name='mdp' placeholder='Mot de passe'/>&nbsp;
        <input type='submit'  name='register' value="S'enregister" />&nbsp;
        </form>
        <?php
        if(isset($erreur))
        {
            echo "<p class='content_error'>".$erreur."</p>";
        }
        ?>
    </div>
</body>
</html>

On m'a typiquement parlé des binds comme ce genre de syntaxe de requête vous en pensez quoi ?

$insertmbr = $bdd->prepare('INSERT INTO utilisateurs (pseudo, mail, mdp, avatar, date_inscription) VALUES(:pseudo, :mail, :mdp, :avatar, now())');
                        $insertmbr->bindParam(':pseudo',$pseudo,PDO::PARAM_STR);
                        $insertmbr->bindParam(':mail',$mail,PDO::PARAM_STR);
                        $insertmbr->bindParam(':mdp',$mdp,PDO::PARAM_STR,40);
                        $insertmbr->bindParam(':avatar','default.jpg',PDO::PARAM_STR,11);
                        $insertmbr->execute();

Merci à vous,

AlexCode

22 oct. 2015, 11:15

Bonjour,

Tu pourrais faire une fonction de contrôle qui "return" un message en cas d'erreur. Ca te permettrai d'avoir moins de if imbriqués qui complique la lecture. Certains préconise de ne faire qu'un return dans une fonction, mais dans le cas de fonction de contrôle, je trouve que ca reste lisible et bien plus clair que les if imbriqués.

Je préfère gérer l'affichage en fin de traitement. Ca permet par exemple de faire une redirection vers une autre page ou de traiter les erreurs. En cas de plantage d'une requête sql, tu as déjà afficher le début de ta page et ca peut compliquer l'affichage de l'erreur en plein milieu.

Tu pourrais également utiliser des fonctions pour afficher les différentes parties de ta page ou isoler des parties de traitement.

22 oct. 2015, 11:24

Bonjour,

Quelques remarques/conseils en vrac

Pour vérifier si un email ou un pseudo existe, tu fais un SELECT *. Cette requête te retourne toutes les données présentes en base pour le ou les enregistrements trouvés, alors que tout ce que tu veux savoir c'est s'il y a déjà des enregistrements ou non.
Il serait préférable de faire un SELECT COUNT(*) qui va simplement compter le nombre d'enregistrements sans te renvoyer de valeur. Du coup ce n'est plus un rowCount() qu'il faudra faire pour contrôler le nombre d'enregistrement retourné, mais juste vérifier la valeur du count().

Autre solution, ne retourner que les informations dont tu as besoin. Un " SELECT mail, pseudo FROM utilisateurs WHERE mail = :mail OR pseudo = :pseudo " te permettrait ansi de contrôler les deux informations en un seul appel à la base de données.
En cas de résultat présent en base, il suffira alors de comparer le mail et le pseudo retournés pour savoir lequel des deux existe déjà (voire si les deux n'existent pas déjà)

Petite anomalie au niveau du header, tu concatene une variable à l'intérieur d'une chaine :

header("location:index.php?id_user='.$lastInsertId.");

devrait être

header("location:index.php?id_user=" . $lastInsertId);

Je te recommande également de mettre un exit(); après un header("Location:").
En effet, le header va envoyer l'instruction de redirection, mais php va poursuivre l'exécution du script inutilement et parfois à tort. Avec l'instruction exit() tu dis à php de faire la redirection et de s'arrêter.

Je te suggère également en cas d'erreur de remettre les informations envoyées par l'utilisateur en valeur par défaut de tes champs. Il n'y a rien de plus frustrant que de remplir le formulaire, d'avoir une erreur dans un champ et de devoir tout remplir à nouveau

A noter que concernant les mots de passe il est d'usage de demander à l'utilisateur de saisir une confirmation de celui-ci. Cela évite une faute de frappe lors de la saisie du mot de passe et la création d'un compte qu'il ne pourra jamais utiliser

Pour le reste pas grand chose à redire en ce qui me concerne

22 oct. 2015, 19:31

histoire de chipoter :
$pseudo = $_POST['pseudo'];
$mail = $_POST['mail'];
$mdp = sha1($_POST['mdp']);

if(!empty($_POST['pseudo']) AND !empty($_POST['mail']) AND !empty($_POST['mdp']))
{
cela générera un warning si quelqu'un fait un formulaire post vers ton script sans par exemple le champ mail.

23 oct. 2015, 10:00

Merci à tout les trois !

@or 1 je ne vois pas la différence avec mon code, je suis désolé...

@Mazarini effectivement ça serait plus simple ! Je m'y attarde dès que j'ai finis le reste de la syntaxe !

@Ryle, merci pour cette super réponse je vais mettre en place tout ce que tu me dis ! C'est sympa merci !

Du coup concernant les rowcount on m'a conseillé de faire un fetch avec une requête correctement écrite car le rowcount n'a pas vocation à mon emploie.

Du coup je fais une requête de ce type :

$requsername = $bdd->prepare('SELECT * FROM users WHERE username = :username');
$requsername->execute(['username' => $_POST['username']]);
                            
 if (!$requsername->fetch())
{}
else
{}

Donc ça, ça marche impeccable pi je sécurise quand même le truc sachant que c'est bien écrit, donc j'aurai aimé reprendre la même syntaxe mais cette fois-ci sur un UNION et la je bloque vraiment, car du coup j'arrive pas à déclarer mes valeurs.

Voici ma requête de base :

$sql3 = $bdd->prepare('SELECT article.id as id, article.category_id as category_id, article.type as type, article.article_creator as creator, article.article_creator_id as creator_id, article.article_author as author, article.article_book as book, article.article_content as content, article.article_date as date, article.article_reply_date as reply_date FROM article WHERE article.category_id = ? UNION SELECT reference.id as id, reference.category_id as category_id, reference.type as type, reference.reference_creator as creator, reference.reference_creator_id as creator_id,reference.reference_author as author, reference.reference_book as book, reference.reference_content as creator, reference.reference_date as date, reference.reference_reply_date as reply_date FROM reference WHERE reference.category_id = ? ORDER BY reply_date DESC ');

C'est notamment sur la l'execute que je bloque, faut'il des bindparam... ?

En vous remerciant,

AlexCode

23 oct. 2015, 12:24

Bonjour,

Concernant la remarque de or1, je pense que sa remarque concerne le fait de te voir utiliser les variables envoyées en post et de vérifier ensuite si celles-ci existent/sont vides avec le empty (J'ai failli faire la même

)
En fait, en règle général, la norme est de tester si la variable existe avant de l'utiliser, chose que l'on peut faire avec isSet() (est-ce que la variable existe) ou avec !empty() (est-ce que la variable existe ET n'est pas vide, nulle, ...). Du coup c'est inhabituel de voir le empty() après, mais dans ton cas ce n'est pas l'existence que tu vérifies ici, mais bien le fait que la valeur soit vide ou non. Le test de présence des index est effectué en amont par le contrôle du champ/bouton "register", ce qui implique que tous les champs texte, select et textarea du formulaire ont également bien été envoyés (A noter cependant, ce n'est pas le cas des cases à cochées et des boutons radios qui ne sont envoyés que s'ils sont cochés)

Bref, tout ça pour dire que ce n'est à mon sens pas gênant. Sauf que quitte à utiliser des variables pour stocker les données envoyée en post, autant les réutiliser, ça sera plus simple/lisible :

if(isset($_POST['register']))
        {
            $pseudo = $_POST['pseudo'];
            $mail = $_POST['mail'];
            $mdp = sha1($_POST['mdp']);
 
            if (!empty($pseudo) AND !empty($mail) AND !empty($mdp))
            {

Une remarque également sur les opérateurs de ta condition if(), il est préférable en php d'utiliser les opérateurs "&&" et "||" au lieu de "AND" et "OR". Même si les deux fonctionnent, il y a une légère différence entre les deux dans leur priorité d'exécution et cela peut parfois engendrer des comportement inattendus.
A mon sens il vaut mieux donc réserver les opérateurs php "AND" et "OR" aux (rares) cas où tu sauras exactement pourquoi tu dois les utiliser à la place de && et ||

(en revanche en SQL, pas le choix, c'est bien AND et OR dans les requêtes

)

Pour tes requêtes, il n'y a pas une énorme différence entre le bindParam/bindValue qui te permet de définir tes attributs un par un ou l'execute qui va prendre le tableau de tes valeurs.
Le principal avantage à mon sens est la lisibilité / maintenabilité. Le fait d'associer chaque valeur à son paramètre est plus facile à lire et à maintenir que de devoir compter le nombre de paramètres envoyés pour savoir s'il y a le bon nombre, le bon ordre, etc.
Il y a également un aspect sécurité un peu plus important avec le bindParam : l'execute va considérer que toutes les variables envoyées sont des chaines de caractères (même si les nombres sont correctement gérés). Le bindParam te permet de renforcer le contrôle sur le type de données qui est envoyé (PDO::PARAM_*).

23 oct. 2015, 16:46

si je mets le code suivant :
<form action="http://www.lebondomaine.com/chemindelapage/register.php" method="POST">
<input type='submit' name='register' value="S'enregister" />
</form>
dans une page html que j'ouvre en local dans mon navigateur et que je clique sur le bouton "S'enregister", il y aura bien des warnings car la vérification n'est pas faite au début.

24 oct. 2015, 11:19

Concernant la remarque de or1, je pense que sa remarque concerne le fait de te voir utiliser les variables envoyées en post et de vérifier ensuite si celles-ci existent/sont vides avec le empty (J'ai failli faire la même )
En fait, en règle général, la norme est de tester si la variable existe avant de l'utiliser, chose que l'on peut faire avec isSet() (est-ce que la variable existe) ou avec !empty() (est-ce que la variable existe ET n'est pas vide, nulle, ...). Du coup c'est inhabituel de voir le empty() après, mais dans ton cas ce n'est pas l'existence que tu vérifies ici, mais bien le fait que la valeur soit vide ou non. Le test de présence des index est effectué en amont par le contrôle du champ/bouton "register", ce qui implique que tous les champs texte, select et textarea du formulaire ont également bien été envoyés (A noter cependant, ce n'est pas le cas des cases à cochées et des boutons radios qui ne sont envoyés que s'ils sont cochés)

Oui en faite c'est maladroit dans ma syntaxe de coder ainsi, je vais essayer de reprendre !

Une remarque également sur les opérateurs de ta condition if(), il est préférable en php d'utiliser les opérateurs "&&" et "||" au lieu de "AND" et "OR". Même si les deux fonctionnent, il y a une légère différence entre les deux dans leur priorité d'exécution et cela peut parfois engendrer des comportement inattendus.
A mon sens il vaut mieux donc réserver les opérateurs php "AND" et "OR" aux (rares) cas où tu sauras exactement pourquoi tu dois les utiliser à la place de && et || (en revanche en SQL, pas le choix, c'est bien AND et OR dans les requêtes )

Bon très bonne remarque, du coup j'ai repris tout mes codes et j'ai fais le ménage ! Merci !

Pour tes requêtes, il n'y a pas une énorme différence entre le bindParam/bindValue qui te permet de définir tes attributs un par un ou l'execute qui va prendre le tableau de tes valeurs.
Le principal avantage à mon sens est la lisibilité / maintenabilité. Le fait d'associer chaque valeur à son paramètre est plus facile à lire et à maintenir que de devoir compter le nombre de paramètres envoyés pour savoir s'il y a le bon nombre, le bon ordre, etc.
Il y a également un aspect sécurité un peu plus important avec le bindParam : l'execute va considérer que toutes les variables envoyées sont des chaines de caractères (même si les nombres sont correctement gérés). Le bindParam te permet de renforcer le contrôle sur le type de données qui est envoyé (PDO::PARAM_*).

Ah ok, car je vois beaucoup de l'un comme de l'autre et c'est vrai que c'est beaucoup plus facile de comprendre quand la personne peut se justifier techniquement sur ses choix ! Bon ba du coup je verrai en fonction de ma faciliter à prendre l'un ou l'autre en fonction des cas.

Merci !

Sinon j'ai toujours pas réussi à faire un fetch la dessus, on m'a conseillé ça mais je suis pas sûr du coup surtout du point de vue du rowcount que c'est propre :

$sql3 = $bdd->prepare('SELECT article.id as id, article.category_id as category_id, article.type as type, article.article_creator as creator, article.article_creator_id as creator_id, article.article_author as author, article.article_book as book, article.article_content as content, article.article_date as date, article.article_reply_date as reply_date FROM article WHERE article.category_id = ? UNION SELECT reference.id as id, reference.category_id as category_id, reference.type as type, reference.reference_creator as creator, reference.reference_creator_id as creator_id,reference.reference_author as author, reference.reference_book as book, reference.reference_content as creator, reference.reference_date as date, reference.reference_reply_date as reply_date FROM reference WHERE reference.category_id = ? ORDER BY reply_date DESC ');

if ($sql3->rowcount() > 0)
{
while ($row = $sql3->fetch(PDO::FETCH_ASSOC))
{}
else
}

Critique de syntaxe ou de méthode, j'attend vos retours

Qui est en ligne