Color

[nick-mariani-nbcuni]

No description provided.

[nick-mariani-nbcuni]

@turnerhusa

I reviewed most of the scouting site code and didn’t see any particular issues. The functionality looks to be working as best as I can tell which is awesome. The only suggestions I have are more Best Practices type updates, some of which may be overkill for your site. Up to you if you want to implement them or not but figured I’d pass them on for learning purposes. We can talk more about it if you have any questions.

SQL Injection Security
The first and biggest is guarding against SQL Injection. Currently you directly imbed user input values into your sql statements which can allow a malicious user to destroy your DB. For example in the below query someone could use a name like “Bob’; Drop table users;”

SELECT * FROM users WHERE `username`='$login_username';

This would effectively append the second statement Drop table users; and would obviously be catastrophic. To prevent this a better practice would be to use prepared statements which essentially makes your query a 2 part transaction to prevent SQL injection. You define the base query and then apply the input parameters like below.

// prepare and bind
$stmt = $conn->prepare("INSERT INTO MyGuests (firstname, lastname, email) VALUES (?, ?, ?)");
$stmt->bind_param("sss", $firstname, $lastname, $email);

// set parameters and execute
$firstname = "John";
$lastname = "Doe";
$email = "john@example.com";
$stmt->execute();

I’m not personally very familiar with PHP/MySQL syntax so you can google for more resources on how to do this properly. I pulled the above from https://www.w3schools.com/php/php_mysql_prepared_statements.asp

Try/Catch Block
I would consider this less important for your site but still a good practice to wrap db connections and statements in a try / catch block. This allows your code to better handle exceptions. I see you handle some error conditions like if the user given doesn’t exist but what if you DB is unreachable or the connection was lost? Your page would crash due to an uncaught exception which isn’t a huge deal because the user could just reload the page. But if you get to the point where you are writing backend application code you probably wouldn’t want the entire backend to crash because of one connection issue. So you can place your db connection and sql execution statements within a try/catch block like the below. I believe you would have to switch to using the PDO extension, I don’t think MySQLi supports exception throwing. It would allow you to handle exception better especially if you wanted different behavior for different exception types (e.g Connection exception vs SQL syntax exception). It may be overkill to make this change now but figured I’d bring up in case you would want to utilize it for any future projects you work on. At the very least you might want to add a connection check/die statement to help handle db connections better.

<?php
try {
    $dbh = new PDO('mysql:host=localhost;dbname=test', $user, $pass);
    foreach($dbh->query('SELECT * from FOO') as $row) {
        print_r($row);
    }
    $dbh = null;
} catch (exception $e) {
    print "Error!: " . $e->getMessage() . "<br/>";
    die();
}
?>

General Functionality Suggestions
• Competition values are hard coded but there is a competitions table. You may want to make that a dynamic value driven off of the db.
o I’m not really sure of the difference between competition and match so this may not be applicable
• Use suggested freeform fields. You have a lot of fields that are restricted by the values that exist in the DB but there are a lot of potential options so a dropdown probably wouldn’t be much better. Suggested freeform fields could be a good compromise
• Just general error handling to help users if they put bad values into your forms (e.g. Bad username/password message)