Delete functie werkt niet
Ik ben bezig met een delete functie waarbij ik uit 3 tabellen gegevens wil wissen. Eerst heb ik het ID van de bepaalde speler opgezocht in mijn database en daarna wil ik alle gegevens met die ID verwijderen. Dit is mijn code maar het werkt niet, kan iemand mij helpen?
Code (php)
1
2
3
4
5
6
7
8
9
2
3
4
5
6
7
8
9
<?php
require('connection.php');
$player = $_POST['player_name'];
$query = "SELECT player_id FROM player WHERE player_name = '$player';";
$resultaat = mysqli_query($connectie, $query);
$rij = mysqli_fetch_array($resultaat);
$queryverwijderplayer = "DELETE FROM player WHERE player_id = $rij['player_id'];";
echo $queryverwijderplayer;
?>
require('connection.php');
$player = $_POST['player_name'];
$query = "SELECT player_id FROM player WHERE player_name = '$player';";
$resultaat = mysqli_query($connectie, $query);
$rij = mysqli_fetch_array($resultaat);
$queryverwijderplayer = "DELETE FROM player WHERE player_id = $rij['player_id'];";
echo $queryverwijderplayer;
?>
Code (php)
1
2
3
4
5
6
7
8
9
2
3
4
5
6
7
8
9
<?php
require('connection.php');
$player = $_POST['player_name'];
$query = "SELECT player_id FROM player WHERE player_name = '".$player."'";
$resultaat = mysqli_query($connectie, $query);
$rij = mysqli_fetch_array($resultaat);
$queryverwijderplayer = "DELETE FROM player WHERE player_id = '".$rij['player_id']."'";
echo $queryverwijderplayer;
?>
require('connection.php');
$player = $_POST['player_name'];
$query = "SELECT player_id FROM player WHERE player_name = '".$player."'";
$resultaat = mysqli_query($connectie, $query);
$rij = mysqli_fetch_array($resultaat);
$queryverwijderplayer = "DELETE FROM player WHERE player_id = '".$rij['player_id']."'";
echo $queryverwijderplayer;
?>
Gewijzigd op 06/11/2018 14:59:48 door Bart V B
Zorg er ook voor dat je mysqli_real_escape_string() gebruikt.
- input wordt niet gefilterd/gevalideerd, ook zou het beter zijn om direct met een id te werken (zie toelichting verderop)
- output zou ge-escaped moeten worden zoals @Ariën aanhaalt, op dit moment is SQL-injectie mogelijk; misschien is het met wat kunst- en vliegwerk zelfs mogelijk om alle of willekeurige spelers op deze manier te verwijderen
- dit zou voor de goede orde misschien in een transactie moeten staan
- je controleert niet eens of de eerste query een resultaat oplevert maar je gaat er blindelings vanuit dat die query een resultaat heeft
Het enige wat effectief is veranderd is de verwijdering van punt-komma's in de queries en het toevoegen van quotes om het player id?
Maar los daarvan, de hele opzet is logisch gezien... niet erg logisch. Ik bedoel, als je dan toch een selectie doet op spelersnaam, en vervolgens die speler dan op grond van het bijbehorende id verwijdert, waarom verwijder je dan niet meteen een speler op basis van spelersnaam? Of dus beter, door alleen te werken met speler ids, zodat er nooit een naam aan te pas komt, wat de kans op het verwijderen van een verkeerde speler verder wegneemt.
En dan het verwijderen zelf, misschien is dat ook geen goed idee. Voor wanneer er toch per ongeluk de verkeerde speler wordt verwijderd. Zou het niet beter zijn om een status "deleted" bij te houden, zodat "verwijderde" spelers eventueel ook weer teruggehaald kunnen worden?
Quote:
Het enige wat effectief is veranderd is de verwijdering van punt-komma's in de queries en het toevoegen van quotes om het player id?
Volgens mij vroeg Max toch echt waarom het niet werkte.
Om de code te laten werken was dat nodig. Dus, what is your point?
Ik geef er nog bij aan dat hij eigenlijk eerst een controle moet doen om te kijken of er überhaupt is gepost, en ook nog dat het lek is. Ja, het kan allemaal beter. Is het goed om advies te geven? Zeker.
Maar je ziet toch ook wel dat Max daar nog niet zover in is....
Bart V B op 06/11/2018 19:15:34:
Volgens mij vroeg Max toch echt waarom het niet werkte.
Om de code te laten werken was dat nodig. Dus, what is your point?
Om de code te laten werken was dat nodig. Dus, what is your point?
Nou, je gaf alleen een nieuw codefragment zonder enige toelichting. Wat kan de topicstarter daar van leren? Dit is toch geen "zoek de 10 verschillen" website?
Bart V B op 06/11/2018 19:15:34:
Ik geef er nog bij aan dat hij eigenlijk eerst een controle moet doen om te kijken of er überhaupt is gepost, en ook nog dat het lek is. Ja, het kan allemaal beter. Is het goed om advies te geven? Zeker.
Maar je ziet toch ook wel dat Max daar nog niet zover in is....
Maar je ziet toch ook wel dat Max daar nog niet zover in is....
Reden te meer om te onderbouwen wat je doet, en nog belangrijker, waarom. En ook om op een gegeven moment een waardeoordeel te geven over de code waar je mee bezig bent. In dit geval lijkt het mij ofwel verstandiger om opnieuw te beginnen, of dit maar te laten rusten. Als dit slechts een fragment is van een veel grotere applicatie, grote kans dat daar ook enorme gaten in zitten, wat heeft het dan voor zin om dit stukje op te lappen?
Gewijzigd op 06/11/2018 21:16:15 door Thomas van den Heuvel
Code (php)
1
2
3
4
5
6
7
8
9
10
11
12
2
3
4
5
6
7
8
9
10
11
12
<?php
require('connection.php');
$player = mysqli_real_escape_string( $connectie, $_POST['player_name']);
$queryverwijderplayer = "DELETE FROM player WHERE player_id = $player;";
$queryverwijderclub = "DELETE FROM club WHERE player_id = $player;";
$queryverwijdertime = "DELETE FROM time WHERE player_id = $player;";
$resultaatplayer = mysqli_query($connectie, $queryverwijderplayer);
$resultaatclub = mysqli_query($connectie, $queryverwijderclub);
$resultaattime = mysqli_query($connectie, $queryverwijdertime);
header('Location: beveiligdepagina.php');
exit();
?>
require('connection.php');
$player = mysqli_real_escape_string( $connectie, $_POST['player_name']);
$queryverwijderplayer = "DELETE FROM player WHERE player_id = $player;";
$queryverwijderclub = "DELETE FROM club WHERE player_id = $player;";
$queryverwijdertime = "DELETE FROM time WHERE player_id = $player;";
$resultaatplayer = mysqli_query($connectie, $queryverwijderplayer);
$resultaatclub = mysqli_query($connectie, $queryverwijderclub);
$resultaattime = mysqli_query($connectie, $queryverwijdertime);
header('Location: beveiligdepagina.php');
exit();
?>
- Ariën - op 07/11/2018 08:45:15:
Foutafhandeling hoort eigenlijk op elke query en connectie.Elk van die kan fout gaan.
Dus je bedoelt dat ik bij elke query erbij moet zetten:
Wij sterven toch ook niet als we wat fout doen? ;-)
Ikzelf gebruik de Object-georienteerde versie van MySQLi (met pijltjes, waaronder: $db->query(.....) ). In een MySQL extended class overschrijf ik de query() functie en voeg daar standaard foutafhandeling toe (als Exception).
Vanaf dan heb ik bij elke query een foutafhandeling zonder dat ik ze hoef te verpakken met if-statements.
Gewijzigd op 07/11/2018 09:11:33 door - Ariën -
Max Amelsfort op 07/11/2018 07:41:55:
Ik heb nu gezorgd dat de value van de vorige pagina het ID van de speler is in plaats van de naam, daarmee heb ik dit gekregen.
Okay, maar waarom heeft dat id nog de naam player_name? Dat is op zijn zachtst gezegd misleidend. Geef alles zelf-uitleggende namen.
Code (php)
1
2
3
4
5
6
7
8
9
10
11
12
2
3
4
5
6
7
8
9
10
11
12
<?php
require('connection.php');
$player = mysqli_real_escape_string( $connectie, $_POST['player_name']);
$queryverwijderplayer = "DELETE FROM player WHERE player_id = $player;";
$queryverwijderclub = "DELETE FROM club WHERE player_id = $player;";
$queryverwijdertime = "DELETE FROM time WHERE player_id = $player;";
$resultaatplayer = mysqli_query($connectie, $queryverwijderplayer);
$resultaatclub = mysqli_query($connectie, $queryverwijderclub);
$resultaattime = mysqli_query($connectie, $queryverwijdertime);
header('Location: beveiligdepagina.php');
exit();
?>
require('connection.php');
$player = mysqli_real_escape_string( $connectie, $_POST['player_name']);
$queryverwijderplayer = "DELETE FROM player WHERE player_id = $player;";
$queryverwijderclub = "DELETE FROM club WHERE player_id = $player;";
$queryverwijdertime = "DELETE FROM time WHERE player_id = $player;";
$resultaatplayer = mysqli_query($connectie, $queryverwijderplayer);
$resultaatclub = mysqli_query($connectie, $queryverwijderclub);
$resultaattime = mysqli_query($connectie, $queryverwijdertime);
header('Location: beveiligdepagina.php');
exit();
?>
Uh oh. Je gebruikt hier weliswaar real_escape_string(), maar dit is niet veilig zonder dit te combineren met quotes.
real_escape_string() is géén wondermiddel. Als er niets te escapen valt, wordt er ook niets ge-escaped. Stel dat je user 6 wilt verwijderen, maar dit combineert met "OR 1=1". Voer je hier real_escape_string() op uit dan blijft dit "6 OR 1=1". Ik hoef je waarschijnlijk niet te vertellen wat er gebeurt als je deze query uitvoert:
Code (php)
1
DELETE FROM player WHERE player_id = 6 OR 1=1
(spoiler: alle players worden verwijderd)
Het feit dat je daarna(ast) allerlei data uit losse tabellen moet verwijderen baart mij ook zorgen. Dit impliceert min of meer dat dit geen echte relationele database is, en alle tabellen als los zand aan elkaar hangen :/.
Thomas van den Heuvel op 07/11/2018 15:48:35:
Okay, maar waarom heeft dat id nog de naam player_name? Dat is op zijn zachtst gezegd misleidend. Geef alles zelf-uitleggende namen.
Uh oh. Je gebruikt hier weliswaar real_escape_string(), maar dit is niet veilig zonder dit te combineren met quotes.
Max Amelsfort op 07/11/2018 07:41:55:
Ik heb nu gezorgd dat de value van de vorige pagina het ID van de speler is in plaats van de naam, daarmee heb ik dit gekregen.
Okay, maar waarom heeft dat id nog de naam player_name? Dat is op zijn zachtst gezegd misleidend. Geef alles zelf-uitleggende namen.
Uh oh. Je gebruikt hier weliswaar real_escape_string(), maar dit is niet veilig zonder dit te combineren met quotes.
Hoe moeten de quotes er dan omheen?
Toevoeging op 07/11/2018 16:17:50:
- Ariën - op 07/11/2018 09:08:31:
Ja, maar gebruik van die() is niet netjes.
Wij sterven toch ook niet als we wat fout doen? ;-)
Ikzelf gebruik de Object-georienteerde versie van MySQLi (met pijltjes, waaronder: $db->query(.....) ). In een MySQL extended class overschrijf ik de query() functie en voeg daar standaard foutafhandeling toe (als Exception).
Vanaf dan heb ik bij elke query een foutafhandeling zonder dat ik ze hoef te verpakken met if-statements.
Wij sterven toch ook niet als we wat fout doen? ;-)
Ikzelf gebruik de Object-georienteerde versie van MySQLi (met pijltjes, waaronder: $db->query(.....) ). In een MySQL extended class overschrijf ik de query() functie en voeg daar standaard foutafhandeling toe (als Exception).
Vanaf dan heb ik bij elke query een foutafhandeling zonder dat ik ze hoef te verpakken met if-statements.
Ik heb "die" overal gebruikt omdat ik daarmee zorg dat de rest van de code ook niet wordt uitgevoerd als de verbinding of query niet goed is. Zo heb ik dat ook geleerd
Dus de query wordt uiteindelijk bijv.
DELETE FROM player WHERE player_id = '8'
Code (php)
1
2
3
4
2
3
4
<?php
$player = mysqli_real_escape_string( $connectie, $_POST['player_name']);
$queryverwijderplayer = "DELETE FROM player WHERE player_id = '".$player."'";
?>
$player = mysqli_real_escape_string( $connectie, $_POST['player_name']);
$queryverwijderplayer = "DELETE FROM player WHERE player_id = '".$player."'";
?>
Zelf raad ik aan om de mysqli_real_escape_string() toe te voegen in de opdracht zelf. Dan heb je direct een goed overzicht of je niks vergeten bent. Het zou vervelend zijn als je toch ergens overheen leest, een rotte querie hebt die misbruik wordt, en dat eventuele persoonsgegevens op straat ligt.
Code (php)
1
2
3
4
2
3
4
<?php
$queryverwijderplayer = "DELETE FROM player
WHERE player_id = '".mysqli_real_escape_string($connectie, $_POST['player_name'])."'";
?>
$queryverwijderplayer = "DELETE FROM player
WHERE player_id = '".mysqli_real_escape_string($connectie, $_POST['player_name'])."'";
?>
En over je die(). Die moet je nooit gebruiken. De functie die() laat je script direct stoppen, en dat is nooit de oplossing. Je layout wordt opeens afgebroken, en eventuele belangrijke script daarna worden ook niet meer geladen.
Ikzelf gebruik de object-georiënteerde versie van MySQLi, en pas de foutafhandeling zo toe:
Code (php)
Waarna ik dus De class initieer en de queries uitvoer met:
Code (php)
1
2
3
4
2
3
4
<?php
$db = new Database(DB_HOST, DB_USER, DB_PASS, DB_NAME);
$db->query("SELECT dit, dat, FROM dingen");
?>
$db = new Database(DB_HOST, DB_USER, DB_PASS, DB_NAME);
$db->query("SELECT dit, dat, FROM dingen");
?>
Als er wat fout gaat in de query, dan krijg je mooi een exception voor de kiezen die je met de exception_handler kan aanpassen (of je werkt met try-catch blokken)
Gewijzigd op 07/11/2018 16:31:47 door - Ariën -
Quote:
Execution will stop after the exception_handler is called.
En een try-catch eigenlijk ook, immers:
Quote:
When an exception is thrown, code following the statement will not be executed, and PHP will attempt to find the first matching catch block.
Gewijzigd op 07/11/2018 16:46:08 door Thomas van den Heuvel
Maar goed, in mijn voorbeeld is de afhandeling sowieso centraal geregeld, zonder dat je je hele applicatie hoeft om te bouwen naar statements om je queries.
Dit kun je met geen fatsoen in gaan bouwen in een verzameling van scripts die deze aanpak niet volgt, tenzij je dus de hele opbouw hier op afstemt, in welk geval je gewoon beter opnieuw kunt beginnen.
En al helemaal als het fundament (de database) nogal krakkemikkig is.
Code (php)
1
2
3
4
5
2
3
4
5
<?php
$queryverwijderplayer = "DELETE FROM player WHERE player_id = $player;";
$queryverwijderclub = "DELETE FROM club WHERE player_id = $player;";
$queryverwijdertime = "DELETE FROM time WHERE player_id = $player;";
?>
$queryverwijderplayer = "DELETE FROM player WHERE player_id = $player;";
$queryverwijderclub = "DELETE FROM club WHERE player_id = $player;";
$queryverwijdertime = "DELETE FROM time WHERE player_id = $player;";
?>
Ikzelf raad aan om alles te flaggen als verwijderd. Zo voorkom je ook een 'f*ck up' als je per ongeluk het verkeerde verwijdert.
- Ariën - op 07/11/2018 18:38:30:
Ikzelf raad aan om alles te flaggen als verwijderd. Zo voorkom je ook een 'f*ck up' als je per ongeluk het verkeerde verwijdert.
Inderdaad, dat had ik hierboven ook al aangehaald.