login  Naam:   Wachtwoord: 
Registreer je!
 Forum

kan een foutje niet terug vinden

Offline Radio247 - 10/01/2015 20:04
Avatar van Radio247Lid Ik gebruik onderstaand script om een bestand aan te roepen omzodoende een record te kunnen verwijderen na bevestiging.
Het benodigde id wordt meegegeven zie ik in de urlbalk van de pop-up, maar het lijkt of er verder niets mee wordt gedaan.
Ook krijg ik de melding:
Warning: mysqli_fetch_row() expects parameter 1 to be mysqli_result, boolean given on line 51.


Het script om de pop-up te openen is:
  1. <a href="verwijder.php" onclick="window.open('verwijder.php?id=<?php echo $row['artid'] ?>', 'Verwijder advertentie', 'width=400,height=150'); return false" title="<?php echo htmlspecialchars($row['tekst_kort'], ENT_QUOTES)?>! verwijderen?"></a>


Het verwijderscript hieronder:

  1. <?php
  2. if(!isset($_SESSION['Username'])){
  3. header("Location: ../member/login.php");
  4. }
  5.  
  6. include '../database_connection.php';
  7.  
  8. class JScript
  9. {
  10. function close_window() {
  11. echo '<script language="JavaScript">window.close();</script>';
  12. }
  13. function refresh_parent() {
  14. echo '<script language="JavaScript">window.opener.location = window.opener.location;</script>';
  15. }
  16. }
  17. $eigenaarid = (isset($_SESSION['Memberid']));
  18.  
  19. echo "<body bgcolor=#E0DFE3>";
  20.  
  21. if(isset($_POST) && array_key_exists('Ja',$_POST))
  22. {
  23. # IMG weghalen!
  24. $locatiebestand = "images/" . $_POST['foto1'];
  25. unlink($locatiebestand);
  26.  
  27. # Record weghalen
  28. $sql = "DELETE FROM artikelen WHERE artid=$_POST[id] AND eigenaar_id = '$eigenaarid'";
  29.  
  30. if (mysqli_query($dbc, $sql)) {
  31. echo "Advertentie succesvol verwijderd";
  32. } else {
  33. echo "Het verwijderen is niet gelukt: " . mysqli_error($dbc);
  34. }
  35.  
  36. mysqli_close($dbc);
  37.  
  38. echo <<<EOD
  39. <script language="JavaScript">
  40. window.opener.location = window.opener.location;
  41. setTimeout("window.close()", 250);
  42. </script>
  43. EOD;
  44. }
  45.  
  46. else
  47. {
  48. if ($result = mysqli_query($dbc, "SELECT * FROM artikelen WHERE artid = '$_GET[id]"));
  49. while ($row = mysqli_fetch_row($result));
  50.  
  51. echo "<form action=verwijder.php method=POST>";
  52. echo "<input type=HIDDEN name=artid value=$row[artid]>";
  53. echo "<input type=HIDDEN name=foto1 value=$row[foto1]>";
  54. echo "<input type=HIDDEN name=tekst_kort value=$row[foto1]>";
  55.  
  56. echo "<table cellspacing=0 cellpadding=0 border=0 width=100%>";
  57. echo "<tr>";
  58.  
  59. echo "<td>";
  60. echo "<img src=../images/_alertDel.jpg border=0>";
  61. echo "</td>";
  62.  
  63. echo "<td>";
  64. echo "<font face=Verdana size=1 color=#000000>";
  65. echo "Weet u zeker dat u <b>$row[tekst_kort]</b> wilt verwijderen?";
  66. echo "</td>";
  67.  
  68. echo "</tr>";
  69. echo "</table>";
  70.  
  71. echo "<center>";
  72. echo "<input type=submit name=confirm value=Ja> ";
  73. echo "<input type=submit name=confirm value=Nee onClick=window.close()> ";
  74.  
  75. }
  76.  
  77. ?>

2 antwoorden

Gesponsorde links
Offline BigBug - 11/01/2015 13:45
Avatar van BigBug PHP expert Ik snap nog niet heel erg veel van je code omdat je html, js en php door elkaar gebruikt, maar in elk geval zijn de volgende zaken niet helemaal lekker: (nu wel)

  1. $result = mysqli_query($dbc, "SELECT * FROM artikelen WHERE artid =" .$_GET["id"])


  1. $_POST["id"]


  1. $row["tekst_kort"]


Als je dan toch zo graag php binnen je html wilt printen, sluit dan je php af voor je html blok, en open deze daarna weer, en gebruikt php binnen de elementen die je wilt printen....
Offline Thomas - 11/01/2015 15:37 (laatste wijziging 11/01/2015 16:24)
Avatar van Thomas Moderator Ik zou dit ook anders aanpakken.

Wat je uiteindelijk wilt doen (wat het effect van de verwijder-operatie zou moeten zijn) is het verwijderen van een record en een daarmee geassocieerd geupload bestand. Dit zouden de enige twee handelingen moeten zijn, ik zou dit niet combineren met output of wat dan ook, dat maakt het alleen maar minder goed leesbaar.

Daarnaast wil je waarschijnlijk dat deze operatie in zijn geheel, of in zijn geheel niet uitgevoerd wordt. Daarom is het misschien niet zo'n verstandig idee om eerst de afbeelding te verwijderen, en dan pas (proberen) het record te verwijderen... Als je database-tabel de InnoDB-engine gebruikt, kun je gebruik maken van een transactie. De enige informatie die je hoeft te versturen om een record te verwijderen is het id - de rest van de informatie kun je bij het verwijderen eerst uit de database uitlezen. Een betere volgorde zou dus zijn (mits je tabel van de InnoDB-engine gebruik maakt):
- start transactie
- lees record met id X uit met de toevoeging FOR UPDATE, en sla deze info op in een variabele
- verwijder het record met id X, als de query om wat voor reden fout gaat zou het script gestaakt moeten worden (exception, die(), exit)
- indien de query succesvol was, voer nog andere acties uit om e.e.a. op te ruimen met behulp van de eerder onthouden record-informatie
- commit transactie

Dit zorgt ervoor dat het bestand alleen verwijderd wordt als de query slaagt, en als er ergens onderweg iets fout gaat tijdens de transactie wordt de transactie teruggedraaid en wordt er dus ook niets verwijderd.

Dan nog iets anders: je code in je huidige vorm is niet echt veilig omdat je geen exit na je header() aanroep zet.

Beschouw de volgende abstracte code:
  1. <?php
  2. if ($controle === false) {
  3. header('Location: login.php');
  4. }
  5.  
  6. $query = '...';
  7. mysqli_query($con, $query);
  8. ?>


Zelfs wanneer $controle de waarde false heeft, wordt de query uitgevoerd! Probeer dit maar eens uit.

Headers zijn output, dus tenzij je script iets afdrukt, worden deze pas aan het einde van je script (als output) verstuurd. En in jouw geval buffer je alle output (ob_start()), dus dan wordt sowieso het versturen van output uitgesteld tot het einde van je script.
Tenzij je dus direct na je header() aanroep aangeeft dat je script "klaar" is door middel van een exit statement, wordt de rest van het script gewoon doorlopen. Het is een (wijdverbreide) misconceptie dat header('Location: ...') je direct fysiek doorstuurt. Dit gebeurt normaal pas aan het einde van je script, tenzij je dus direct "ophangt" met exit.

EDIT: Tot slot is je query vatbaar voor SQL-injectie omdat je niet controleert of $_GET['id'] (of $_POST['id']) een getal bevat. Dit valt onder input filtering. Waarschijnlijk is het een goede gewoonte om alle DATA in je SQL te escapen met de daarvoor bestemde _real_escape_string() functie (al heeft dit geen effect voor numerieke waarden). Dit valt onder output escaping.

Ofwel dat (input filtering + gebruikmaking van een _real_escape_string() functie), of je maakt gebruik van prepared statements, maar die zijn in MySQLi nogal brak naar mijn mening. (En zelfs dan zou je eigenlijk -waar van toepassing- je input moeten filteren!)

In onze MySQLi tutorial wordt (onder andere) uitgebreid aandacht besteed aan security (maar ook aan transacties en dergelijke). Ik stel voor dat je deze eens aandachtig doorneemt. Hier staat heel veel informatie in.

EDIT2: Declaraties van classes zou je in aparte bestanden moeten zetten. In plaats van het includen/requiren van al deze bestanden kun je wellicht beter gebruik maken van een soort van autoloader. Dit zorgt ervoor dat enkel die bestanden worden betrokken bij een script die het script nodig heeft. Niet meer, niet minder. Daarnaast wordt je code korter (geen lijst van requires of wat dan ook).

Ook zou je wat meer object georienteerd kunnen gaan werken, hiermee kun je een script beter compartimenteren in acties in plaats van (geneste) if-elseif-else statement, wat je code gigantisch onoverzichtelijk maakt (hiermee kun je ook code en html (output) beter scheiden).
Gesponsorde links
Je moet ingelogd zijn om een reactie te kunnen posten.
Actieve forumberichten
© 2002-2019 Sitemasters.be - Regels - Laadtijd: 0.282s