Processing user input for a form

Question

I'm still learning PHP OOP, so I'd like to seek for some expert advice or criticism regarding this code I created.

In this script, I created the objects I need to process the user input from a form. While the $homepage object is responsible for displaying the whole webpage.

My questions are:

1. Is this consider clean code?
2. Can I completely hide the fact that I actually include the autoload and the configuration files?
3. Any recommendation or tips to improve my code?

<?php

/**
*
* OOP practice
* @author Joey
*/

require_once 'config/autoload.php';
require_once 'config/config.php';

// Database credentials

$host = $config['host'];
$dbname = $config['dbname'];
$username = $config['username'];
$password = $config['password'];

// Short variable names

$fname = isset($_POST['fname']) ? clean_data($_POST['fname']) : null;
$lname = isset($_POST['lname']) ? clean_data($_POST['lname']) : null;
$your_question = isset($_POST['question']) ? clean_data($_POST['question']) : null;

try {

    // Page object

    $homepage = new Page('Questions and Answers', 'QA', '', 'You ask, we answer');

    // Author object

    $author = new Author($fname, $lname);

    // Question object

    $question = new Question($your_question, $author);

    // Database object

    $pdo = new DbConnect($host, $dbname, $username, $password);

    // Connect to the database

    $pdo->open_connection();
} catch (Exception $e) {
    echo $e->getMessage(), '<br>';
    exit;
} catch (PDOException $e) {
    echo $e->GetMessage(), '<br>';
    exit;
}

if (isset($_POST['submit'])) {
    // Insert the user input into the database

    $pdo->execute_query('INSERT INTO author (first_name, last_name) VALUES (?, ?)', array($author->getFirstName(), $author->GetLastName()));
    $pdo->close_connection();
    $homepage->content = '<h3>Your question</h3><br><p>Hello ' . $author->getFirstName() . ' ' . $author->getLastName() . '!' . '</p><br>';
    $homepage->content .= '<p>You ask: ' . $question->getQuestion() . '</p><br>';
    $homepage->content .= 'Do you want to ';
    $homepage->content .= '<a href="index.php">Ask another question?</a>' . ' ' . '<br>';
} else {
    // Display the form

    $homepage->content = <<<_END

    <h3>Your question here</h3><br>
    <form action="{$_SERVER['PHP_SELF']}" method="post" role="form">
        <label for="fname">Enter your first name</label><br>
     <input type="text" id="fname" name="fname" size="30"><br>
        <label for="lname">Enter your last name</label><br>
        <input type="text" id="lname" name="lname" size="30"><br>
        <label for="question">What is your question?</label><br>
        <textarea id="question" name="question" rows="10" cols="70"></textarea><br>
        <input type="submit" name="submit" value="Submit">
    </form>
_END;
}

// Display the webpage using the Page class

$homepage->display();

function clean_data($data)
{
    $data = trim($data);
    $data = strip_tags($data);
    $data = addslashes($data);
    return $data;
}

Answer 1

A short list of issues I've noticed after a first glance at your code:

• Pokemon exception handling (gotta catch 'em all): wrapping everything in a huge try-catch block is considered bad practice. What makes your code stand out even more is the pointless second catch
• pointless second catch: a PDOException is a custom exception class, which is to say: PDOException extends Exception, therefore, your catch (Exception $e) block will catch instances of PDOExceptions, too. As a result, the second catch block is obsolete. Pointless. Code-smell. Noise. Get rid of it.
• clean_data is not great. It's bad even. I can understand the strip_tags call, but addslashes is really rather pointless here. And a possible source of bugs. If you forget to call addslashes when inserting a password, or in some other piece of code that handles login information, the passwords in the DB won't match, even though the user has filled out the form correctly. You're using prepared statements, no need to add slashes. If anything, it does more harm than good
• From the variable name ($pdo), it's quite obvious what kind of class DbConnect is. And, like I've posted on this site so many times (links below): it's evil. PDO offers a clean, well balanced, easy to use and powerful, fully object oriented API. You can't wrap it into something and end up with a better tool. Far from it. The only thing that you can do with it is to use it as a base in a fully-fledged DBAL like doctrine. And since doctrine already exists, writing it yourself is, basically, reinventing the wheel.
• Generally, I don't like PHP code that strings together markup. It's error prone, and hard to maintain. That's why I didn't pay much attention to the HTML bit, but as tim pointed out: $_SERVER['PHP_SELF'] is a bad idea. If the form should be processed by the same script (or on the same url) that spawned it, just don't specify an action attribute. The default behavior is to post the form back to the same url anyway.

Links to previous answers explaining why you shouldn't try to wrap PDO:

• Class for reducing development time
• OOP PDO Database Connection
• A PDO connection classto initiate multiple connection and easy access
• Extendable PDO Wrapper Class
• Check my profile, I'm pretty sure there are a lot more answers that deal with the matter, but I can't be bothered to look for every single one of them.

Answer 2

XSS

Your clean_data currently protects you against XSS via $fname, $your_question, etc. But it is hard to see that , because the cleaning happens so far away from the printing. And if you follow the advice of @Elias Van Ootegem and remove the clean_data method (which you should), then you are open to XSS. So you should prevent this directly when printing user input.

The other (and a lot more dangerous) XSS attack is $_SERVER['PHP_SELF']. You should definitely filter this with htmlspecialchars (or use an empty form action, or hardcode it).

Naming

• be consistent. See for example getFirstName vs GetLastName (the first version with a lowercase starting letter is more conventional).
• don't shorten (too much): For example, firstName isn't that much longer than fname, but it is clearer.

Misc

• Having a password in a PHP file inside the web root could leak your password in some instances.
• your indentation is off for the HTML part