[PHP/MySQL] Geen vat op MySQL entries

[CoolJuul]

Ik ben nog relatief nieuw met PHP en zeker wanneer het OO-programmeren erin betreft. Meestal los ik m'n problemen na kortere of langere tijd altijd zelf wel op. Maar nu zit ik redelijk met de handen in het haar; ik heb geen idee hoe onderstaand probleem tot stand komt en hoe dit op te lossen...

Het ding is namelijk dat ik een PHP script heb opgebouwd als een grote klasse die telkens geinstantieerd wordt vanuit een index-file. Elke nieuwe pagina als het ware in de applicatie maakt gebruik van diezelfde klasse en index. Nu ben ik nog maar net begonnen met het bouwen ervan en wilde nu net een methode toevoegen voor het wegschrijven van data naar een database. Het vreemde is al meteen dat ik telkens duplicaat entries krijg. Toch wanneer ik het het pad van de code naloop lijkt het er gewoon op dat de "INSERT" query slechts eenmaal aangeroepen wordt. Tegen dit probleem ben ik een tijdje geleden ook aangelopen in een andere context, dus misschien heeft het te maken met m'n gebrekkige manier van (OO?) programmeren?

Maar het meest aparte moet nog komen, tenminste zo zie ik het. Wanneer ik het PHP script op een gegeven moment open laat staan, terwijl de executie daarvan zelf beeindigd lijkt te zijn (geen loops of dergelijke), blijven er MySQL entries verschijnen in m'n database met telkens dezelfde inhoud (uitgezonderd van een auto increment ID). Het lijkt dus alsof er in de achtergrond steeds iets in de weer blijft gaan.

Is dit een typisch iets van OO-progammeren wat ik niet begrijp? Als ik het zo zie wordt de "insert"-command sowieso telkens twee keer uitgevoerd en daarna blijft 'ie maar doorgaan als ik de pagina open laat staan...

Ik kan me niet herinneren dat ik dat ooit eerder heb gehad met PHP. Ik werk overigens in een MAMP omgeving (Mac OSX). Code wil ik trouwens best plaatsen, alleen is daar weinig bijzonders aan te zien volgens mij.

EDIT: Toch maar even de code van de klasse toegevoegd.

PHP:

<?php
    class app {
        private $host;
        private $username;
        private $password;
        private $database;
        
        function __construct() {
            // database credentials
            $this->host = 'localhost';
            $this->username = 'root';
            $this->password = 'root';
            $this->database = 'medb';
        
            // connect to database
            $this->connect();
            
            $this->showPage();
        }
        
        private function connect() {
            mysql_connect($this->host, $this->username, $this->password) or die('Could not connect. ' . mysql_error());
            mysql_select_db($this->database) or die('Could not select database. ' . mysql_error());
        }
        
        private function showPage() {
            $page = $_SESSION['page'];
            
            if (isset($_POST['new'])) { $page = 'new'; }
            if (isset($_POST['submitCreationGoal'])) { $this->save(); unset($_POST['submitCreationGoal']); $page = 'collabSettings'; }
            
            switch ($page) {
                case 'new':
                    include('inc/view/new.view.php');
                    break;
                case 'collabSettings':
                    include('inc/view/collabsettings.view.php');
                    break;
                default:
                    include('inc/view/newopen.view.php');
            }
        }
        
        private function save() {
            $creationGoal = $_POST['creationGoal'];
        
            if (isset($_POST['submitCreationGoal'])) {
                mysql_query("INSERT INTO creationgoals (goal) VALUES ('$creationGoal')");
            }
        }
    }
?>

[ Voor 27% gewijzigd door CoolJuul op 04-06-2009 10:38 ]

[Voutloos]

Begin maar eens met het controleren van return values van system calls. Zonder (onder andere) de controle van de return value van bijvoorbeeld elke mysql_*() functie kan je enkel debuggen als een kip zonder kop.

En veerdeel omwille van leesbaarheid aub de regel met de save() call en een aantal andere statements over meerdere regels.

[ Voor 23% gewijzigd door Voutloos op 04-06-2009 10:46 ]

[storeman]

Lastig om zo te zeggen, heb je ook je index.php erbij?

Doe voor de gein eens een echo na of voor de query en kijk hoe vaak deze terugkomt in je pagina. Op die manier kun je zien hoe vaak deze wordt uitgevoerd. Het lijkt me toch dat daar het probleem ligt.

Daarnaast is dit niet een erg mooie OO methode. Alle functies staan in een class, wat op zich voor orde zorgt. De manier waarop je het nu doet is niet erg handig. Je bent een soort database wrapper aan het maken (tip: kijk eens naar de Zend_Db component van Zend Framework).

Je gebruikt nu niet echt de kracht van OO programmeren, aangezien de class magisch alles afhandelt als deze geinstantieerd is.

PHP:

$theApp = new App();
$theApp->setLoginCredentials( ..... );
$theApp->connect();

$theApp->buildPage( $_POST['page_id'] );
echo $theApp->getPage();

Hoop dat je wat kunt met mijn opbouwende kritiek. (tip: probeer je class nooit iets te laten echoën, tenzij de class hier specifiek voor bedoeld is).

[ Voor 0% gewijzigd door storeman op 04-06-2009 11:54 . Reden: code tag ]

[CodeCaster]

offtopic:
Zo te zien denk jij dat "OO Programmeren" hetzelfde is als "alles in een klasse flikkeren omdat dat hip is", no flame intended.

Ik zie hier drie dingen die geen hol met elkaar te maken hebben in één klasse staan, dat is dus het tegenovergestelde van low cohesion, high coupling. Verder doe je niet aan escaping.

Maar, ontopic: ik denk dat je een stuk code niet laat zien terwijl dat wel uitgevoerd wordt. Je zal wel een redirect loop hebben. Print telkens na de insert eens een regel tekst, dan zal de redirect ook stoppen (PHP kan geen headers versturen na output).

[Verwijderd]

Duplicate entries kan je natuurlijk al voorkomen door een goede primaire sleutel te setten in je database ontwerp. Sowieso kan je kijken wanneer welke queries uitgevoerd worden door een serie echo's te plaatsen zoals hierboven idd ook al vermeldt is.

En ik zie een security issue:

CoolJuul schreef op donderdag 04 juni 2009 @ 10:34:

PHP:

        private function save() {
            $creationGoal = $_POST['creationGoal'];
        
            if (isset($_POST['submitCreationGoal'])) {
                mysql_query("INSERT INTO creationgoals (goal) VALUES ('$creationGoal')");
            }
        }
?>

Je post variabele stop je zo in een query, is vrijgemakkelijk om zogenaamde sql injection uit te voeren.

[CoolJuul]

Ten eerste bedankt voor jullie tijd en goede adviezen!

Jullie hebben natuurlijk helemaal gelijk als de klasse wat rommelig en a-OO in elkaar steekt. Helaas is het (nog) geen tweede natuur van me en zal er zeker in het vervolg extra aandacht aan besteden.

SQL injection tegengaan heeft geen prioriteit want het gaat sowieso niet online.

Ik ben net weer terug achter de computer en zal zometeen eens kijken of ik het opgelost krijg. Ik heb al eerder met echo's geprobeerd te achterhalen of er ergens een ongewenste loop in zit, maar naar mijn weten wordt echt alles maar een enkele keer uitgevoerd (in het script dan). Redirect loop lijkt me ook sterk. Maar zal er nog eens goed doorheen lopen.

[cariolive23]

SQL injection tegengaan heeft geen prioriteit want het gaat sowieso niet online.

Dit is kinderachtig eenvoudig te implementeren, wen jezelf eraan dat je dit altijd doet. Het is eenvoudiger om dat altijd te doen, dan met regelmaat te moeten debuggen en dit alsnog in te moeten bouwen. Ook in een probeerseltje.

Gebruik bv. sprintf(), werkt snel en eenvoudig, of de MySQLi (improved) functie om een prepared statement aan te maken.

Maar iets bouwen zonder de basis, een huis kun je ook bouwen zonder fundering, blijft ook wel een paar dagen staan...

[Patriot]

cariolive23 schreef op donderdag 04 juni 2009 @ 14:16:
[...]


Dit is kinderachtig eenvoudig te implementeren, wen jezelf eraan dat je dit altijd doet. Het is eenvoudiger om dat altijd te doen, dan met regelmaat te moeten debuggen en dit alsnog in te moeten bouwen. Ook in een probeerseltje.

Wat een onzin. Als je weet dat het eigenlijk hoort maar je er voor het gemak vanaf ziet omdat het niet nodig is, dan maakt dat echt niets uit hoor.

[CodeCaster]

Patriot schreef op donderdag 04 juni 2009 @ 16:17:
[...]


Wat een onzin. Als je weet dat het eigenlijk hoort maar je er voor het gemak vanaf ziet omdat het niet nodig is, dan maakt dat echt niets uit hoor.

Totdat je bezig bent met een project dat wèl live moet, en denkt: "Hé, die code had ik al eens geschreven" en vervolgens de onaangepaste code online gooit.

[cariolive23]

maar je er voor het gemak vanaf ziet

Welk gemak? Zodra je gaat testen kan een test mislukken omdat een query mislukt dankzij het ontbreken van het escapen van quotes e.d. De resultaten van deze test zijn dan ook waardeloos, je test iets waarvan je bij voorbaat al weet dat het nog niet klaar is voor een test.

Het veilig verwerken van userinput kost je geen enkele moeite, het is hooguit anders. Zelf gebruik ik altijd prepared statements in PostgreSQL, nog veel eenvoudiger kan het niet worden.

PHP:

<?php
$result = pg_query_params($conn, 'SELECT * FROM tabel WHERE x= $1 and y = $2;', array($_POST['x'], $_POST['y']));
?>

Wanneer dit al teveel moeite is, kun je beter een andere hobby gaan zoeken... Maak van dit soort constructies een gewoonte, dan kun je het nooit vergeten. Net als het gebruik van een gordel in een auto, dat is ook de gewoonste zaak van de wereld. Het voelt raar aan om zonder gordel rond te rijden.

Met de MySQLi-functies kun je bij mijn weten iets soortgelijks maken voor MySQL-queries.

Maar goed, het raakt erg offtopic.

[Voutloos]

Patriot schreef op donderdag 04 juni 2009 @ 16:17:
[...]
Wat een onzin. Als je weet dat het eigenlijk hoort maar je er voor het gemak vanaf ziet omdat het niet nodig is, dan maakt dat echt niets uit hoor.

Die beslissing kan je gewoon nooit voor eeuwig juist maken en bovendien bespaar je er weinig mee. Zeer slechte trade-off. Dusdanig slecht dat een aantal van dergelijke opmerkingen wat mij betreft er echt voor mogen zorgen dat je niet als collega aangenomen wordt.

[Patriot]

Voutloos schreef op donderdag 04 juni 2009 @ 17:56:
[...]
Die beslissing kan je gewoon nooit voor eeuwig juist maken en bovendien bespaar je er weinig mee. Zeer slechte trade-off. Dusdanig slecht dat een aantal van dergelijke opmerkingen wat mij betreft er echt voor mogen zorgen dat je niet als collega aangenomen wordt.

Mijn hele punt was dat wanneer je puur voor jezelf even wat aan het experimenteren bent, het echt geen probleem is om iets even op een 'vieze' manier te doen. En dat het vervolgens zeker niet nodig is om wéér die eeuwige preek over SQL injection voor te dragen. Nu heb ik effectief natuurlijk het tegenovergestelde bereikt (want we zijn enorm offtopic bezig).

[creator1988]

Wat Patriot zegt,

we moeten hier eens wat meer gaan focussen op het probleem in plaats van elke keer blaat blaat sql injection te roepen als iemand een stuk code post.