So this coding critique is about my PHP class file. Essentially I use this to figure out my visitor's preferences on my site and then assign those preferences to classes.
Keep in mind I'm no where near the guru level of a lot of people on this board so I'm looking for two types of comments...
1.) Your honest opinions in the long term in regards to the new and improved way I've coded this.
2.) Your opinions about anything absolutely critical that should be changed in regards to security or if you perceive something won't work in the way that you think might not work as I desire it to. Because I'm essentially going to use this almost as-is for Version 2.8 Preview VI of my website which will probably debut sometime in mid-June (it's currently Preview V ATM).
So first the old way I originally had setup (this part is just for comparison of what I started with, commentary in regards to this will be useless because it's not what I'm going to use)...
The Old
Code: Select all
<?php
class settings {
// class stuff defined here
}
if ($_GET['audio'] == "0") {$trueaudio = 0;}// if (!headers_sent()) {setcookie('audio','0',time()+2592000,'/');}}
else if ($_GET['audio'] == "1") {$trueaudio = 1;}// if (!headers_sent()) {setcookie('audio','1',time()+2592000,'/');}}
else if ($_GET['audio'] == "2") {$trueaudio = 2;}// if (!headers_sent()) {setcookie('audio','2',time()+2592000,'/');}}
else if (isset($_GET['audio'])) {$error = audio; $trueaudio = 0;}
else if ($_COOKIE['audio'] == "0") {$trueaudio = 0;}
else if ($_COOKIE['audio'] == "1") {$trueaudio = 1;}
else if ($_COOKIE['audio'] == "2") {$trueaudio = 2;}
else {$trueaudio = 0;}
if ($_GET['connection'] == "1") {$trueconnection = 1;}// if (!headers_sent()) {setcookie('connection','1',time()+2592000,'/');}}
else if ($_GET['connection'] == "2") {$trueconnection = 2;}// if (!headers_sent()) {setcookie('connection','2',time()+2592000,'/');}}
else if ($_GET['connection'] == "r") {$trueconnection = r;}// if (!headers_sent()) {setcookie('connection','r',time()-2592000,'/');}}
else if (isset($_GET['connection'])) {$error = connection; $trueconnection = 0;}
else if ($_COOKIE['connection'] == "1") {$trueconnection = 1;}// if (!headers_sent()) {setcookie('connection','1',time()+2592000,'/');}}
else if ($_COOKIE['connection'] == "2") {$trueconnection = 2;}// if (!headers_sent()) {setcookie('connection','2',time()+2592000,'/');}}
else {$trueconnection = 0;}
//keep in mind that those two large chunks of procedural programming...there are other large chunks for EVERY preference that can be currently set on my website.
$settings = new settings();
$settings->set('audio',$audio);
$settings->set('connection',$connection);
?>Ok so you can imagine that with well over a dozen preferences based on those two larger chunks of procedural coding that this file was an eyesore. I even just discovered I forgot to add cookie support for the initial focus option on my website so when you changed the preference it would be effective only on the request where you updated the preference, but not the second page after presuming it wasn't defined in the second request which would fall back to the cookie.
Any way below is the new version of my class file which again keep in mind I'm more interested in anything absolutely critical (and if there are vulnerabilities ways I can test for them to reproduce the results so I know what I'm dealing with).
So here is my current step up with commentary explaining what each part of the file does...
The New
Code: Select all
class settings {
// class stuff defined here
}
// Defaults (overridden later on if preferences DO exist so this is just a safety-net)
$audio = '0';
$backgroundimages = '0';
$browserpatch = '1';
$chatroom = '0';
$connection = '0';
$css3 = '0';
$cursors = '0';
$dhtmleffects = '0';
$dtd = '0';
$ieccss = '1';
$keyboardlayout = 'developer';
$mediatype = '';
$personality = '0';
$powerkeys = '0';
$sounds = '0';
$theme = 'classic';
// Take cookie's properties and values and turn them in to variables with values!!!
/*
HUGE Credit to VladSun at devnetwork.net!
*/
$pairs = split('[._]', $_COOKIE['settings']);
for ($i = 0; $i < count($pairs) / 2; $i++) ${$pairs[$i*2]} = $pairs[$i*2 + 1];
// Function called to determine GET, POST, or COOKIE value, then set to it's respective $variable.
function example($property)
{
global $$property;
if (isset($_POST['formis']))
{
if (isset($_POST[$property])) {$$property = $_POST[$property];}
}
else if ($_SERVER['REQUEST_METHOD'] == 'GET')
{
if (isset($_GET[$property])) {$$property = $_GET[$property];}
}
else {$$property = $_GET[$property];}
}
// List of existing preferences, simply add a new string to the array and it will be allowed to be saved as part of the cookie string (or will be once I code it in hahaha)
$cookie_old = array(
'audio',
'backgroundimages',
'browserpatch',
'chatroom',
'connection',
'css3',
'cursors',
'dhtmleffects',
'dtd',
'ieccss',
'keyboardlayout',
'mediatype',
'personality',
'powerkeys',
'sounds',
'theme'
);
// Umm, I forgot... haha
$found = 0;
// For every string above call the function and set it's value!
foreach ($cookie_old as $key => $value) {
example($value);
// Transition Preview IV cookies to Preview V
if (isset($_COOKIE[$value])) {setcookie($value, '', time()-60); $found = 1;}
example($value);
}
// Set the cookie using the updated values...
$cookie_value = 'audio.'."$audio".'_backgroundimages.'."$backgroundimages".'_browserpatch.'."$browserpatch".'_chatroom.'."$chatroom".'_connection.'."$connection".'_css3.'."$css3".'_cursors.'."$cursors".'_dhtmleffects.'."$dhtmleffects".'_dtd.'."$dtd".'_ieccss.'."$ieccss".'_initialfocus.'."$initialfocus".'_keyboardlayout.'."$keyboardlayout".'_mediatype.'."$mediatype".'_personality.'."$personality".'_powerkeys.'."$powerkeys".'_sounds.'."$sounds".'_theme.'."$theme";
setcookie('settings',$cookie_value,time()+2592000,'/');
// Set the $variables to classes to be used throughout the website...
$settings = new settings();
$settings->set('audio',$audio);
$settings->set('backgroundimages',$backgroundimages);
$settings->set('browserpatch',$browserpatch);
$settings->set('chatroom',$chatroom);
$settings->set('css3',$css3);
$settings->set('connection',$connection);
$settings->set('cursors',$cursors);
$settings->set('dhtmleffects',$dhtmleffects);
$settings->set('dtd',$dtd);
$settings->set('ieccss',$ieccss);
$settings->set('initialfocus',$initialfocus);
$settings->set('keyboardlayout',$keyboardlayout);
$settings->set('mediatype',$mediatype);
$settings->set('personality',$personality);
$settings->set('powerkeys',$powerkeys);
$settings->set('scrollbars',$scrollbars);
$settings->set('sounds',$sounds);
$settings->set('theme',$theme);Please remember I'm open to all criticisms but that I will concentrate on anything more critical as this will go live almost as-is in about a month. If there is something open to an attack please actually demonstrate how I could emulate that attack.
I'm actually thinking now at this last moment I could probably throw the last part of the script in to a foreach loop. Oh well...no matter how good you get there's always room for improvement I suppose.