Wat kan beter aan dit script?

DarkFeather · 16 jun 2004

(foutje)

DarkFeather · 16 jun 2004

Zou iemand (ah, JPeetje, alsjeblieft?) ook nog even naar dit script willen kijken? Is deze zo een beetje goed?

connect.php (toch even voor de zekerheid

)

PHP:

<?
$host = "localhost";
$user = "root";
$pass = "";
$db= "test";
$tabel = "test";

$connect = mysql_connect($host,$user,$pass) or die("Kon geen verbinding maken met MySQL Server: " . mysql_error()); 
mysql_select_db($db,$connect) or die("Kon de database niet selecteren: " . mysql_error());
?>

verwijderen.php

PHP:

<?php 
  if((isset($_POST["id"]) == TRUE))
  {
  include("connect.php");

   $query = "DELETE FROM $tabel WHERE id = '$_POST[id]'";
  $result = @mysql_query($query);
    if($result == FALSE)
    {
    echo ("Kon de opdracht niet uitvoeren: " . mysql_error());
    }
    elseif(mysql_affected_rows() == 0)
    {
    echo "De opdracht werd succesvol uitgevoerd, maar om onbekende redenen werd er de pagina niet uit de database verwijderd";
    }
    else
    {
    echo "De pagina is verwijderd";
    }
  }
  else
  {
if ($_GET[id]==FALSE)
{
echo "Er is geen pagina geselecteerd.";
}
elseif ($_GET[id]=="1")
{
echo "Dit is de indexpagina. Deze kunt u niet verwijderen.";
}
else
{
include "connect.php";
  $sql = "SELECT naam FROM $tabel WHERE id = '$_GET[id]'";
  $result = mysql_query($sql); 
    if($result == FALSE)
    {
    echo "Kon query niet uitvoeren: " . mysql_error();
    }
    else
    {
    while($rij = mysql_fetch_object($result))
    {
    $naam = $rij->naam;
    }
    if ($naam == FALSE)
    {
        echo "Geen bestaande pagina geselecteerd";
    }
    else
    {
?>

<form name="submit" method="post" action="verwijderen.php">
<input type="hidden" name="id" value="<? echo $_GET[id]; ?>">
<p />
Weet u zeker dat u de pagina met de naam: <? echo $naam; ?> wilt verwijderen?
<br />
  <input type="submit" value="Ja, verwijder maar!">
</form>
<?
}
}
}
}
?>

masterprut · 16 jun 2004

Je weet dat het script niet werkt?

DarkFeather · 16 jun 2004

Geplaatst door masterprut
Je weet dat het script niet werkt?

Nu moet ie het wel doen!

masterprut · 16 jun 2004

PHP:

<?

if(isset($_POST["id"]) == TRUE) // 1
{
  include("connect.php");
  
  $query = "DELETE FROM " . $tabel . " WHERE id = '" . ((int)$_POST['id']) . "'"; // 2, 4, 5 & 9
  $result = @mysql_query($query);
  
  if($result == FALSE)
  {
    echo("Kon de opdracht niet uitvoeren: " . mysql_error()); // 6
  }
  elseif(mysql_affected_rows($result) == 0)
  {
    echo "De opdracht werd succesvol uitgevoerd, maar om onbekende redenen werd er de pagina niet uit de database verwijderd"; // 7
  }
  else
  {
    echo "De pagina is verwijderd"; // 6
  }
}
else
{
  if(isset($_GET['id']) == FALSE) // 7
  {
    echo "Er is geen pagina geselecteerd."; // 6
  }
  elseif($_GET['id'] == "1")
  {
    echo "Dit is de indexpagina. Deze kunt u niet verwijderen."; // 6
  }
  else
  {
    include "connect.php";
    $sql = "SELECT naam FROM " . $tabel . " WHERE id = '" . ((int)$_POST['id']) . "'"; // 3, 9 & 2
    $result = mysql_query($sql); 
    
    if($result == FALSE)
    {
      echo "Kon query niet uitvoeren: " . mysql_error(); // 6
    }
    else
    {
    	while($rij = mysql_fetch_object($result))
    	{
    	  $naam = $rij->naam; // 9
    	}
    	
    	if ($naam == FALSE) // 9
    	{
        echo "Geen bestaande pagina geselecteerd"; // 6
    	}
    	else
    	{ ?>
        <form name="submit" method="post" action="verwijderen.php">
        <input type="hidden" name="id" value="<? echo $_GET[id]; ?>">
        <p />
        Weet u zeker dat u de pagina met de naam: <? echo $naam; ?> wilt verwijderen?
        <br />
          <input type="submit" value="Ja, verwijder maar!">
        </form>
   <? }
    }
  }
}
?>

1: Je gebruikt dubbele haakjes alleen als je meerdere waardes / variablen controleert.
2: functies gescheiden van variables.
3: Zie 2.
4: Van $_POST[id], $_POST['id'] gemaakt, anders werkt 't niet

5: Waarom heb je ech("bericht"); gedaan?
6: Waarom hier nu ineens geen echo("bericht");
7: van if ($_GET['id']==FALSE), if(isset($_GET['id']) == FALSE) gemaakt. Je kan moeilijk iets checken op een waarde als er geen waarde aan zit

8: met ((int)$var) zorg je ervoor dat een getal word afgerond naar een heel getal
9: Wat wil je hier doen?

masterprut · 16 jun 2004

Geplaatst door DarkFeather

Nu moet ie het wel doen!

Waarom nu wel?

DarkFeather · 16 jun 2004

De bedoeling is dat het script een pagina (is een rij in de database) uit de database verwijdert. Nu kom je door middel van een link (die er bijv. zo uitziet: verwijderen.php?id=5). Je krijgt nu de vraag of je de pagina echt wilt verwijderen. Als je op de submit-knop hebt gedrukt moet de rij uit de database worden verwijderd.

DarkFeather · 16 jun 2004

Geplaatst door masterprut

Waarom nu wel?

Had op twee plaatsen in het script de verkeerde superglobals gebruikt ipv $_GET had ik $_POST gebruikt, terwijl de ID daar middel van de link gekozen werd.

(Heb het bericht met het script aangepast!!!)

Geplaatst door masterprut

8: met ((int)$var) zorg je ervoor dat een getal word afgerond naar een heel getal

Is niet nodig.

Geplaatst door masterprut

9: Wat wil je hier doen?

Controleren of de ID die ingegeven is (door de link, maar een link kan je aanpassen) wel een bestaande pagina is.

Heb nu heel iets veranderd, heb nu het volgende:

PHP:

<?php 
  if((isset($_POST['id']) == TRUE))
  {
  include "connect.php";

   $query = "DELETE FROM ". $tabel ." WHERE id = '" .$_POST['id']. "'";
   $result = @mysql_query($query);
    if($result == FALSE)
    {
    echo "Kon de opdracht niet uitvoeren: " . mysql_error();
    }
    elseif(mysql_affected_rows() == 0)
    {
    echo "De opdracht werd succesvol uitgevoerd, maar om onbekende redenen werd er geen pagina niet uit de database verwijderd";
    }
    else
    {
    echo "De pagina is verwijderd";
    }
  }
  else
  {
if(isset($_GET['id']) == FALSE) 
{
echo "Er is geen pagina geselecteerd.";
}
elseif ($_GET['id']=="1")
{
echo "Dit is de indexpagina. Deze kunt u niet verwijderen.";
}
else
{
include "connect.php";
  $sql = "SELECT naam FROM " . $tabel ." WHERE id = '" . $_GET['id'] . "'";
  $result = mysql_query($sql); 
    if($result == FALSE)
    {
    echo "Kon query niet uitvoeren: " . mysql_error();
    }
    else
    {
    while($rij = mysql_fetch_object($result))
    {
    $naam = $rij->naam;
    }
    if ($naam == FALSE)
    {
        echo "Geen bestaande pagina geselecteerd";
    }
    else
    {
?>

<form name="submit" method="post" action="verwijderen.php">
<input type="hidden" name="id" value="<? echo $_GET[id]; ?>">
<p />
Weet u zeker dat u de pagina met de naam: <? echo $naam; ?> wilt verwijderen?
<br />
  <input type="submit" value="Ja, verwijder maar!">
</form>
<?
}
}
}
}
?>

Wat kan er nu nog anders?

masterprut · 16 jun 2004

Nou, eitje toch?

- Eerst kijk je of er een id (?id= ..) opgegeven.
- Daarna check je of er al op de submit knop gedrukt is
- Als er op de knop gedrukt is, verwijder de rij
- Als er niet op de knop gedrukt is, laat de knop zien

Klein voorbeeldje:

PHP:

<?

include("connect.php"); // Zorg ervoor dat er verbinding is met de database

if(isset($_GET['id']) == FALSE) // Als er geen id opgegeven is, dan =>
{
  echo "Je moet wel een id opgegeven!"; // Vermeld de error dat er geen ID is opgegeven
}
else // Als er wel een Id is opgegeven dan =>
{
  $query = "SELECT * FROM " . $tabel . " WHERE id = '" . ((int)$_GET['id']) . "'"; // Selecteer de rij uit de database waar de kolom id de waarde $_GET['id'] heeft
  $result = @mysql_query($query); // Voert de query uit

  if($result == FALSE)
  {
    echo "Er is een probleem tijdens het selecteren van de informatie uit de database"; // Vermeld error als het niet gelukt is informatie uit de database te halen
    die();
  }
  
  if(mysql_num_rows($result) == 0) // Als er als resultaat is van de query, geen rijen gevonden zijn => error laten zien
  {
    echo "Geen geldig ID opgegeven!";
  }
  else
  {
    if(isset($_POST['verwijder']) == FALSE) // Er word gekeken of er op de knop 'Verwijder' is gedrukt.
    {
      // Er is niet op de knop gedrukt
      echo "<form method=\"POST\">"; // Formulier starten
      echo "Weet je zeker dat je " . $_GET['id'] . " wilt verwijderen?<br><br>"; // Vraagje
      echo "<input type=\"submit\" name=\"verwijder\" value=\"Ja, verwijder!\">"; // Knopje
      echo "</form>"; // Formulier sluiten
    }
    else // Als er WEL op de knop is gedrukt
    {
      // Er is op de knop gedrukt
      $query = "DELETE FROM " . $tabel . " WHERE id = '" . ((int)$_GET['id']) . "'"; // Query tot verwijderen
      $result = @mysql_query($query); // Query uitvoeren

      if($result == FALSE) // Als er een error is tijdens het uitvoeren van '$result'
      {
        // Er is een error tijdens het uitvoeren van $result
        echo "Er is een error tijdens het uitvoeren van de query";
      }
      else
      {
        echo "De query is succesvol uitgevoert";
      }
    }
  }
}

?>

Tis uit het blote hoofd, hoop dat het werkt. Zoniet, post de error dan mee die je krijgt of probeer het zelf opte lossen

:thumb:

masterprut · 16 jun 2004

Geplaatst door DarkFeather

Had op twee plaatsen in het script de verkeerde superglobals gebruikt ipv $_GET had ik $_POST gebruikt, terwijl de ID daar middel van de link gekozen werd.

(Heb het bericht met het script aangepast!!!)

Top :thumb:

Geplaatst door DarkFeather

Is niet nodig.

Waarom niet? Je zal toch echt een error krijgen als je ?id=blaat opgeeft

Geplaatst door DarkFeather
Heb nu heel iets veranderd, heb nu het volgende:

PHP:

<?php if((isset($_POST['id']) == TRUE)) { include "connect.php"; $query = "DELETE FROM ". $tabel ." WHERE id = '" .$_POST['id']. "'"; $result = @mysql_query($query); if($result == FALSE) { echo "Kon de opdracht niet uitvoeren: " . mysql_error(); } elseif(mysql_affected_rows() == 0) { echo "De opdracht werd succesvol uitgevoerd, maar om onbekende redenen werd er geen pagina niet uit de database verwijderd"; } else { echo "De pagina is verwijderd"; } } else { if(isset($_GET['id']) == FALSE) { echo "Er is geen pagina geselecteerd."; } elseif ($_GET['id']=="1") { echo "Dit is de indexpagina. Deze kunt u niet verwijderen."; } else { include "connect.php"; $sql = "SELECT naam FROM " . $tabel ." WHERE id = '" . $_GET['id'] . "'"; $result = mysql_query($sql); if($result == FALSE) { echo "Kon query niet uitvoeren: " . mysql_error(); } else { while($rij = mysql_fetch_object($result)) { $naam = $rij->naam; } if ($naam == FALSE) { echo "Geen bestaande pagina geselecteerd"; } else { ?> <form name="submit" method="post" action="verwijderen.php"> <input type="hidden" name="id" value="<? echo $_GET[id]; ?>"> <p /> Weet u zeker dat u de pagina met de naam: <? echo $naam; ?> wilt verwijderen? <br /> <input type="submit" value="Ja, verwijder maar!"> </form> <? } } } } ?>

Post je er ook bij wat je verandert heb?

, scheelt mij weer zoeken

Geplaatst door DarkFeather

Wat kan er nu nog anders?

Ziet er niet slecht uit, weet niet precies hoelang je bezig bent maar als je een weekje of 1 / 2 bezig bent ziet het er heel redelijk uit

:thumb:

JPeetje · 16 jun 2004

Anders quote je ff een gigantische lap PHP

Btw, dit deel:

PHP:

<form name="submit" method="post" action="verwijderen.php">
<input type="hidden" name="id" value="<? echo $_GET[id]; ?>">
<p />
Weet u zeker dat u de pagina met de naam: <? echo $naam; ?> wilt verwijderen?
<br />
  <input type="submit" value="Ja, verwijder maar!">
</form>

Als $_GET["id"] niet bestaat, krijg je een error (op een goeie server althans). FF controleren met een isset()

Verder haal je het resultaat van de 2e query, de SELECT query, door een while. Volgens mij haal je maar 1 rij op, en is die while() dus niet nodig.
De rest van 't script heeft masterprut al verbeterd, althans ... Ik ga er vanuit dat hij dat goed heeft gedaan ...

connect.php ziet d'r trouwens prima uit :thumb:

DarkFeather · 16 jun 2004

Geplaatst door JPeetje
Anders quote je ff een gigantische lap PHP
Btw, dit deel:
.. knip ...
Als $_GET["id"] niet bestaat, krijg je een error (op een goeie server althans). FF controleren met een isset()

In het script wat ik als laatste heb gepost staat:

PHP:

if(isset($_GET['id']) == FALSE) 
{
echo "Er is geen pagina geselecteerd.";
}

Het lijkt mij niet nodig om het nog een keer te controleren

Geplaatst door JPeetje
Verder haal je het resultaat van de 2e query, de SELECT query, door een while. Volgens mij haal je maar 1 rij op, en is die while() dus niet nodig.

Die moet er nog even uit dus...

Geplaatst door JPeetje
De rest van 't script heeft masterprut al verbeterd, althans ... Ik ga er vanuit dat hij dat goed heeft gedaan ...
connect.php ziet d'r trouwens prima uit :thumb:

M'n script heb ik al verbeterd m.b.v. de tips van masterprut.

@masterprut:

Waarom niet? Je zal toch echt een error krijgen als je ?id=blaat opgeeft

Als je een waarde opgeeft die niet bestaat/ wordt gevonden wordt er dus geen waarde toegekend aan $naam, m.b.v. dit stukje script krijg je dan een "fout"melding:

PHP:

    if ($naam == FALSE)
    {
        echo "Geen bestaande pagina geselecteerd";
    }

Wat ik heb verbeterd zijn een paar kleine dingetjes (zoals alle echo's op dezelfde manier, variabelen buiten aanhalingstekens en dat soort dingen).

Ziet er niet slecht uit, weet niet precies hoelang je bezig bent maar als je een weekje of 1 / 2 bezig bent ziet het er heel redelijk uit

Ben denk ik nu bijna 2 weken bezig met MySQL. Dank je.

Maar opzich is m'n script toch verder goed?
Zit er trouwens kwaliteitsverschil (qua werking) in het script van masterprut en m'n script (behalve dat die van masterprut netter is)?

JPeetje · 16 jun 2004

Geplaatst door DarkFeather
Het lijkt mij niet nodig om het nog een keer te controleren

Fout gelezen

PHP:
Als je een waarde opgeeft die niet bestaat/ wordt gevonden wordt er dus geen waarde toegekend aan $naam, m.b.v. dit stukje script krijg je dan een "fout"melding:

PHP:

if ($naam == FALSE) { echo "Geen bestaande pagina geselecteerd"; }

Wat ik heb verbeterd zijn een paar kleine dingetjes (zoals alle echo's op dezelfde manier, variabelen buiten aanhalingstekens en dat soort dingen).

Een beetje een hele foute manier van controleren. Ga dan met isset() na of $naam bestaat.

Maar opzich is m'n script toch verder goed?

Hij kan beter. Op een server waar magic quotes uitstaan (google maar ff naar

) zal je script query een foutmelding wanneer ik voor $_GET["id"] een ' invul.

Zit er trouwens kwaliteitsverschil (qua werking) in het script van masterprut en m'n script (behalve dat die van masterprut netter is)?

Ja, het is duidelijker, overzichtelijker, veiliger en beter onderhoudbaar (althans, als masterprut goed heeft toegepast wat ik hem geleerd heb ...

)

DarkFeather · 16 jun 2004

Masterprut en JPeetje heeeeeeeeeeel erg bedankt voor de moeite!!!

Nog een laatste paar vraagjes (voor dit topic dan

).

Geplaatst door JPeetje
Fout gelezen

Geeft niet

Een beetje een hele foute manier van controleren. Ga dan met isset() na of $naam bestaat.Hij kan beter.

Dit is dus beter:

PHP:

 if ((isset($naam) == FALSE))

Op een server waar magic quotes uitstaan (google maar ff naar ) zal je script query een foutmelding wanneer ik voor $_GET["id"] een ' invul.

Dus ik moet dit stukje even invoegen aan het begin:

PHP:

if(get_magic_quotes_gpc() == FALSE)
    {
    $id = addslashes($_GET["id"]);
    }

en daarna in het script $_GET["id"] door $id vervangen

Ja, het is duidelijker, overzichtelijker, veiliger en beter onderhoudbaar (althans, als masterprut goed heeft toegepast wat ik hem geleerd heb ... )

Ik heb er een vraagje over zijn script. Als je die() gebruikt stopt de server helemaal met het uitvoeren van het script. Als ik onder het script nog code heb voor bijv. de layout moet ik dan overal waar die() wordt gebruikt gaan werken met if en else (zoals ik eigenlijk in mijn script al doen)? Of is dat niet netjes en bestaat er een mooiere oplossing voor (zelf dacht ik anders aan een iframe)?

masterprut · 16 jun 2004

BIJNA goed over magic quotes. Maar wat als magic quotes aanstaan? dan is er geen $id

DarkFeather · 16 jun 2004

Vooruit dan doen we dit:

PHP:

if(get_magic_quotes_gpc() == FALSE)
    {
    $id = addslashes($_GET["id"]);
    }
else
   {
  $id = $_GET["id"];
  }

DarkFeather · 16 jun 2004

Zou een van jullie nog EEN keer 2 scriptjes (lijken erg op die van masterprutt

) willen "nakijken"? (zie bijlage)

Maar nogmaals heel erg bedankt!!! ik leer op deze manier in een paar dagen namelijk meer dan door allerlei tutorials...

masterprut · 16 jun 2004

Ik heb het even zitten scannen en ziet er tot nu toe goed uit. Maar als het werkt, waarom meoten we het dan nog nakijken

Maar zal het morgen uitgebreid voor je nakijken :evil:

DarkFeather · 16 jun 2004

Geplaatst door masterprut
Ik heb het even zitten scannen en ziet er tot nu toe goed uit. Maar als het werkt, waarom meoten we het dan nog nakijken

Waarom? Van fouten leer je! Ik wil gewoon even weten wat ik allemaal nog fout doe. Oja, zou je ook willen kijken of het gebruik van superglobals goed is? Dan ben ik je voor eeuwig (dat is wel heel lang, maar goed...) dankbaar!

Maar zal het morgen uitgebreid voor je nakijken :evil:

Je bent een schat

! Bedenk wel ik heb grote stukken van jou script overgenomen

!

JPeetje · 17 jun 2004

Geplaatst door masterprut
Ik heb het even zitten scannen en ziet er tot nu toe goed uit. Maar als het werkt, waarom meoten we het dan nog nakijken

Netheid, onderhoudbaarheid, veiligheid, dezelfde dingen waar jij mij op msn mee lastig valt

(volgens mij ben je gewoon op complimentjes uit

)

Geplaatst door DarkFeather
Bedenk wel ik heb grote stukken van jou script overgenomen !

Raad eens waar hij die grote stukken script vandaan heeft ?

Als voor 't script, je hebt daar een hele aparte functie in zitten:

PHP:

function slashes($tekst)
{
  if(get_magic_quotes_gpc() == FALSE)
  {
  $tekst = addslashes($tekst);
  }
  return $tekst;
  }
}

Die functie lijkt errug veel op die ik gebruik ... zelfs de naam is 't zelfde ...

Hou er wel rekening mee dat die gpc niet zomaar leuke lettertjes zijn aan 't einde van de functie, dat is waar ze op slaan: GET, POST en COOKIE waarden.
Enkele aanmerkingen op je script, dit stukje:

PHP:

include("connect.php");

if(get_magic_quotes_gpc() == FALSE)
    {
    $id = addslashes(((int)$_GET['id']));
    }
else
  {
  $id = ((int)$_GET['id']);
  }

Kan vervangen worden door:

PHP:

include("connect.php");

$id = slashes($_GET["id"]);

Aangezien de functie er toch al is. Maar het kan nog korter, aangezien je er toch een integer, oftewel een geheel getal, van maakt, hoef je je geen zorgen te maken over die slashes. Dus simpelweg:

PHP:

$id = ((int) $_GET["id"]);

is genoeg

.
Bij

PHP:

    echo "Er is een probleem tijdens het selecteren van de informatie uit de database";

zou je wel een mysql_error() toe mogen voegen, maar is niet vereist.
Dit daarentegen is wel fout:

PHP:

    if(isset($_POST['bewerk']) == FALSE)

In de else { die daarna volgt, vraag je met $_POST maar liefst een id,naam en tekst op. Ik zie nergens een controle waarbij je checkt of deze waarden ook bestaan.
Verder gebruik je voor $id ook slashes($_POST["id"]);
Hierdoor is het niet zeker dat $id een integer is, aangezien iedereen een HTML formulier op zijn eigen pc kan maken die het formulier naar jouw site verwijst:

Code:

<form action="jouw_website" method="post">

Als men hierbij voor 'id' een tekstveld maakt met de waarde 'blaat', dan komt je query er als volgt uit te zien:

Code:

UPDATE tabel SET naam = 'naam', tekst = 'tekst' WHERE id = blaat

Dat "id = blaat" stukje zal een SQL error opleveren, doordat er geen quotes om "blaat" heen staan. Zou je op $_POST["id"] een (int) toepassen, dan wordt "blaat" veranderd in een getal, dit getal is 0, doordat PHP er gewoonweg niets van kan maken

0 komt dan niet voor in de database, maar de query zal dan geen error opleveren.

Verder heb ik alleen nog een paar performance (snelheid) aanmerkingen, als je die wil horen post je 't maar

Succes :thumb:

// Edit: verwijder.php niet gezien

Wederom, $id hoef je niet slashes() op toe te passen, daar kun je gewoon (int) op los laten.

PHP:

    $naam = stripslashes($rij->naam);

Hm ... of dat nodig is weet ik zonet nog niet ... hangt er vanaf hoe je 't in de database invoert. Als dat echt met \' of \\ of \" in de database staat, dan moet je stripslashes() toepassen, wil je een fatsoenlijk resultaat. Anders niet.
Je beveiliging in verwijder.php is trouwens niet al te best. Hij controleert namelijk alleen of $id gelijk is aan 1 als $_POST["verwijder"] niet bestaat. Als deze wel bestaat maakt het kennelijk niet meer uit. Wederom, ik zet een HTML formulier op m'n eigen pc, verwijs deze door naar jouw script, en voila, de rij met id 1 is verwijderd

De ordening in beide scripts is trouwens niet slecht, houden zo !

// Edit 2: Dat smilie limiet per post mag wel versteld worden naar 15 of 20 ...

Wat kan beter aan dit script?

Gebruiker

Gebruiker

Terugkerende gebruiker

Gebruiker

Terugkerende gebruiker

Terugkerende gebruiker

Gebruiker

Gebruiker

Terugkerende gebruiker

Terugkerende gebruiker

Terugkerende gebruiker

Gebruiker

Terugkerende gebruiker

Gebruiker

Terugkerende gebruiker

Gebruiker

Gebruiker

Bijlagen

Terugkerende gebruiker

Gebruiker

Terugkerende gebruiker

Wij waarderen jouw privacy