Aside from the database connection, which always throws an exception for an error, you have no error handling for database statements that can fail. You are also using emulated prepared queries. Add the following two lines of code at the same location where you are setting the default fetch mode to assoc -
$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); // set the error mode to exceptions
$pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES,false); // run real prepared queries
pbismad Oh cool, thanks I didn't know I could do that. Thanks
I should put everything in TRY{} I guess?
No. The try/catch logic is for cases when there's an error that your application can deal with and recover from. Most database errors are fatal errors, due to programming mistakes, database servers not running, ... that the visitor to a site cannot do anything about or should even know that they have specifically occurred. The visitor should only be told that the page isn't working, i.g. a http 500 error page. Putting try/catch logic in these cases doesn't add any value, and is just cluttering up your code with unnecessary logic. The type of database errors that the visitor can do something about would be submitting and inserting/updating duplicate or out of range values. For these type of errors, your code should have try/catch logic, then detect if the error number is for something that your code is designed to handle, set up a message telling the visitor what was wrong with the data they submitted, and let them try again. If the error number is for anything else, just re-throw the exception and let php handle it.
Oh right hm something like this would be better?
<?php
require('Database.php');
class IndexPage extends Database{
function drawIndex(){
$sql = "SELECT * FROM restaurant";
$stmt = $this->connect()->query($sql);
if (!$stmt){
echo "error"; //make error page here?
}else{
while($row = $stmt->fetch()){
echo $row['name'];
}
}
}
}
cluelessPHP like this would be better?
No. When using exceptions, that conditional logic won't even be executed. One of the points of using exceptions is that your code only deals with error free execution, since execution transfers to the exception handling upon an error. This simplifies your code.
Just let php catch and handle the exception, where it will use its error related settings to control what happens with the actual error information (database errors will 'automatically' get displayed/logged the same as php errors.) When php catches the database exception, if you are displaying all php errors, which will now include database errors, you will see the database error information. If you are logging all php errors, such as when on a live/public server, the database error information will get logged too and a http 500 error page will automatically get produced.
Short-answer - after setting the error mode to exceptions, do nothing in your code except for those cases where the visitor can recover from the database error. Keep It Simple (KISS.)
pbismad So these in an htaccess file should
ErrorDocument 400 /error.php
ErrorDocument 401 /error.php
ErrorDocument 403 /error.php
ErrorDocument 404 /error.php
ErrorDocument 500 /error.php
If I understood you right
Only if you want a custom error page.
pbismad It's something I should probably learn how to do?
You could use https://php.net/set_exception_handler to set a function you want to run on any exception, which can be useful to ensure that things get logged where you want them to be logged, things do not get displayed to the web user that shouldn't (e.e. error message describing database table/column names), what HTTP response code you want to send, etc.
NogDog Oh i'll try it too, right now I've changed my code a little to this
class IndexPage extends Database{
function drawIndex(){
$sql = "SELECT name, id, description, imagePath, imageName FROM restaurant";
$stmt = $this->connect()->query($sql);
$result = $stmt->fetch(PDO::FETCH_ASSOC);
return $result;
}
}
useage
$test = new IndexPage();
$something = $test->drawIndex();
foreach($something as $row){
echo $row;
}
But that's not great and when I try
foreach($something as $row){
echo $row['name'];
}
I get
Warning: Illegal string offset 'name' in C:\Apache24\htdocs\rest\index.php on line 20
A
Notice: Trying to access array offset on value of type int in C:\Apache24\htdocs\rest\index.php on line 20
Warning: Illegal string offset 'name' in C:\Apache24\htdocs\rest\index.php on line 20
I
Warning: Illegal string offset 'name' in C:\Apache24\htdocs\rest\index.php on line 20
u
Warning: Illegal string offset 'name' in C:\Apache24\htdocs\rest\index.php on line 20
p
$result = $stmt->fetch(PDO::FETCH_ASSOC); is only returning one row, so you are looping on each value of that row, each of which is a scalar value, not an array. If you want that method to return all rows, then use fetchAll() instead of just fetch(), and then doing your foreach() would make more sense.
If it's possible it could be a lot of rows (for some undefined value of "a lot"), it might be better to return the actual PDOStatement object, and let the client code do its own loop on it via a series of fetch() calls (instead of populating a huge (for some undefined value of "huge") PHP array.
NogDog fetchAll() worked, so I've successfully separated business logic and display now?