I'm new to OOP and PHP. I've made an attempt at a PHP library file that handles account creations, logins, and file uploads with image resizing on the fly. It works so far. I'd like some help with the Obj Oriented part, namely making the code more efficient/succinct.
User class:
<?php
define( 'BASEPATH', realpath( dirname( __FILE__ ) ) );
define( 'AppRoot', 'http://www.eclecticdigital.com/eventfinder' );
/**
* Class user
*/
class user {
function createAcc() {
$userData = new DB();
$userFiles = new fileHandler();
$userID = $userData->createNewUser();
$userpath = BASEPATH . '/users/user_' . $userID . "/";
$userFiles->userDirectory( $userpath );
}
/*END CREATE ACC*/
/**
* @param $postVars
* @return bool
*/
function loginUser( $postVars ) {
$newLogin = new DB();
$user_data = $newLogin->findUser( $postVars );
$session = new userSession( $user_data );
return true;
}//**END loginUsr function
/*function userLogOut{
1. take $userData and session info and unset session
2. redirect to homepage
unset($_SESSION);
}*/
}
Render class:
<?php
/**
* Class render
*/
class render {
/*MAKE STYLESHEETS AND JAVASCRIPT FILES GLOBALLY AVAILABLE TO THE APP*/
public $homePath;
public $stylePath;
public $javascript;
public $header;
public $beginHTML;
public $headerDiv;
/**
*
*/
public function __construct() {
$this->homePath = AppRoot;
$this->stylePath = $this->homePath . '/style.css';
$this->javascript = $this->homePath . '/eventsJS.js';
$this->beginHTML = '<!Doctype HTML>';
$this->headTag = '<head><title>Eventfinder</title><link href =" ' . $this->stylePath . '" type="text/CSS"
rel="stylesheet" /> <link href="http://fonts.googleapis.com/css?family=Playball" rel="stylesheet" type="text/CSS">
<link href="http://fonts.googleapis.com/css?family=Satisfy" rel="stylesheet" type="text/css">
<script type="text/javascript" src=" ' . $this->javascript . ' "></script> </head>';
$this->headerDiv = "<div id=\"header\"> Eventfinder </div>";
}
function index() {
echo $this->beginHTML;
echo $this->headTag;
echo $this->headerDiv;
}
function usrLogin() {
$newform = new html_;
$newform->userLogin();
}
function createAcc() {
$forms = new html_;
$forms->newUserForm();
}
}
HTML Class:
<?php
/**
* Class html_
*/
class html_ {
public $Type = array( 'button' => 'button', 'checkbox' => 'checkbox', '_file' => 'file', 'hidden' => 'hidden', 'radio' => 'radio', 'r_set' => 'reset', 'submit' => 'submit', '_text' => 'text' );
public $formOpen = '<form method="post" action="tryDB.php">';
public $formClose = "</form>";
public $inputText = '<input \'$name\'>';
/**
* @param $method
* @param $action
* @param $formName
*/
function openForm( $method, $action, $formName ) {
echo "<form method=\"$method\" action=\"$action\" name=\"$formName\">";
}
function newUserForm() {
echo "<div id=accform>";
echo "<form method=\"post\" action=\"#\" name=\"newacc\" onsubmit=\"return validateAcc(this)\"> ";
echo "First Name : <input type=\"text\" name=\"fname\" id=\"firstname\" /> <br><br>";
echo "Last Name : <input type=\"text\" name=\"lname\" /> <br><br>";
echo "email : <input type=\"text\" name=\"email\" id=\"email\" /> <br><br>";
echo "password: <input type=\"password\" name=\"password\" id=\"password\" /> <br><br>";
echo "<input type=\"submit\" value=\"create account\" name=\"create\" >";
echo "</form>";
echo "</div>";
echo "<div id=\"errorsDiv\"></div>";
}
function userLogin() {
echo "<div id=login> ";
$this->openForm( 'post', '#', 'loginForm' );
echo "email : <input type=\"text\" name=\"login\" />";
echo "password: <input type=\"text\" name=\"loginPass\" />";
echo "<input type=\"submit\" value=\"login\" name=\"submit\" />";
$this->formClose;
echo "</div>";
}
}
Filehandler class:
<?php
/**
* Class fileHandler
*/
class fileHandler {
/*CREATE FOLDERS FOR NEW USER'S FUTURE UPLOADS*/
/**
* @param $userDir
*/
public function userDirectory( $userDir ) {
$dir = array( $userDir, $userDir . 'images',
$userDir . 'images/width_800',
$userDir . 'images/width_1024',
$userDir . 'images/width_1400',
$userDir . 'images/width_1600',
$userDir . 'images/width_2400' );
for ( $i = 0; $i < count( $dir ); $i++ ) {
mkdir( $dir[$i], 0764, true );
chmod( $dir[$i], 0764 );
}
}
/*DEFINE IMAGE RESIZE FUNCTIONS*/
/**
* @param $image
* @return mixed
*/
public function imgResize( $image ) {
$this->userPath = $_SESSION['userFolder'];
$imgDetails = explode( '/', $image );
$imgName = $imgDetails[2];
$size = getimagesize( $image );
$imgType = $size['mime'];
$reqdWidths = array( '800', '1024', '1400', '1600', '2400' );
$width = $size[0];
$height = $size[1];
$imgRatio = $width / $height;
/*PUT GD 'IMAGECREATE'FUNCTIONS IN A SWITCH-CASE TO DEAL WITH MULTIPLE IMAGE TYPES */
/**
* @param $img
* @return resource
*/
function createImage( $img ) {
$size = getimagesize( $img );
$typeImg = $size['mime'];
switch ( $typeImg ) {
case 'image/png':
$newImage = imagecreatefrompng( $img );
return $newImage;
break;
case 'image/jpeg':
$newImage = imagecreatefromjpeg( $img );
return $newImage;
break;
case 'image/gif' :
$newImage = imagecreatefromgif( $img );
return $newImage;
break;
}
}
/**
* @param $imgType
* @param $dest
* @param $userDestImage
* @return bool
*/
function finalImg( $imgType, $dest, $userDestImage ) {
switch ( $imgType ) {
case 'image/png':
$final = imagepng( $dest, $userDestImage );
return $final;
break;
case 'image/jpeg':
$final = imagejpeg( $dest, $userDestImage );
return $final;
break;
case 'image/gif' :
$final = imagegif( $dest, $userDestImage );
return $final;
break;
}
}
$source = call_user_func( 'createImage', $image );
for ( $i = 0; $i < count( $reqdWidths ); $i++ ) {
echo count( $reqdWidths ) . '<br><br>';
$userDestImage = $this->userPath . '/images/width_' . $reqdWidths[$i] . '/' . $imgName;
$newWidth = intVal( $reqdWidths[$i] );
$newHeight = intVal( $newWidth / $imgRatio );
$dest = imagecreatetruecolor( $newWidth, $newHeight );
echo $userDestImage;
imagecopyresampled( $dest, $source, 0, 0, 0, 0, $newWidth, $newHeight, $width, $height );
$imgFinal = call_user_func( 'finalImg', $imgType, $dest, $userDestImage );
}
return $imgFinal;
}
/*---function to handle file upload. Calls resize when upload is done---*/
/**
* @return string
*/
function file_upload() {
/**
* @return string
*/
function do_upload() {
$userFolder = $_SESSION['userFolder'];
echo $_FILES['upFile']['type'] . '<br>';
$allowedFiles = array( 'image/gif', 'image/jpg', 'image/jpeg', 'image/png' );
if ( !empty( $_FILES ) && in_array( $_FILES['upFile']['type'], $allowedFiles ) ) {
//echo $userFolder.'<hr>';
$tmpFldr = $_FILES['upFile']['tmp_name'];
$fileDest = $userFolder . '/' . $_FILES['upFile']['name'];
if ( move_uploaded_file( $tmpFldr, $fileDest ) ) {
echo 'file(s) uploaded successfully';
} else {
echo 'Your file failed to upload<br><br>';
}
return $fileDest;
} else {
die( 'An error occurred, please make your
files are of the correct type' );
}
}
//END do_upload,
/*Perform upload return path to file location*/
$fileLoc = do_upload();
return $fileLoc;
}
//**END file_upload
}
DB class:
<?php
/**
* Class DB
*/
class DB {
/*Predefined Queries*/
/**
* @return bool|PDO
*/
public function connect() {
$dbConn = new PDO( 'mysql:dbname=eh15118b;host=xxxxxx', 'xxxxx', 'xxxx' );
if ( $dbConn ) {
echo 'connected';
} else {
$dbConn = false;
echo 'failed to make a connection';
}
return $dbConn;
}
//NEW USER CREATION FUNCTION
/**
* @return string
*/
public function createNewUser() {
$find = "Select email from users where email=:email";
$find_param = array( ':email' => $_POST['email'] );
$connDB = $this->connect();
if ( $connDB ) {
try {
$stmt = $connDB->prepare( $find );
$result = $stmt->execute( $find_param );
$row = $stmt->fetch();
} catch ( PDOException $e ) {
die( 'Failed to execute Query: <hr>' . $e->getmessage() );
}
if ( !empty( $row ) ) {
die ( 'There is already an account registered to the address :email .
You can login here or reset your password' );
} else {
/*SALT AND HASH FOR PASSWORD SECURITY*/
$salt = dechex( mt_rand( 0, 2147483678 ) ) . dechex( mt_rand( 0, 2147483678 ) );
$password = hash( 'sha256', $_POST['password'] . $salt );
$query = "INSERT INTO users (fname,lname, password,salt, email)
VALUES (:fname,:lname, :password,:salt,:email)";
for ( $round = 0; $round < 65336; $round++ ) {
$password = hash( 'sha256', $password . $salt );
}
$query_params = array( ':fname' => $_POST['fname'], ':lname' => $_POST['lname'],
':password' => $password, ':salt' => $salt,
':email' => $_POST['email'] );
try {
$stmt = $connDB->prepare( $query );
$result = $stmt->execute( $query_params );
} catch ( PDOException $ex ) {
/*THIS COULD ALSO BE SENT TO A LOG FILE*/
$ex->getmessage();
}
}
}
$id = $connDB->lastinsertid( 'usrID' );
return $id;
}
//END CREATE NEW USER
/**
* @param $vars
* @return array|null
*/
public function findUser( $vars ) {
$sessionVars = null;
$Conn = $this->connect();
if ( $Conn ) {
$user = $vars['user'];
$pass = $vars['pass'];
/*FIND user in DB*/
$usrLogin = $Conn->quote( $user );
$usrFind = 'Select email,password,salt,usrID from users where email=' . $usrLogin;
$stmt = $Conn->query( $usrFind );
$result = $stmt->setFetchMode( PDO::FETCH_NUM );
$row = $stmt->fetch();
if ( !empty( $row ) ) {
$salt = $row[2];
$checkPass = hash( 'sha256', $pass . $salt );
for ( $round = 0; $round < 65336; $round++ ) {
$checkPass = hash( 'sha256', $checkPass . $salt );
}
//Check Password and start session
if ( $checkPass === $row[1] ) {
$sessionVars = array( 'user' => $row[0], 'userID' => $row[3] );
} else {
die( 'your password is incorrect.<br><br>' );
}
} else {
echo 'no data in row';
}
} else {
echo 'failed to connect to the Database';
}
return $sessionVars;
}
//**END FUNCTION findUser
/**
* @param $postVals
*/
function createEvent( $postVals ) {
$uploads = new fileHandler();
$image = $uploads->file_upload();
if ( $uploads->imgResize( $image ) ) {
echo 'The file : ' . $image . ' was uploaded and resized successfully ';
} else {
echo '<HR> !!message or email to admin that an image was not resized successfully';
}
echo 'THE VALUES IN POST VALS ARE <BR><BR><BR>';
print_r( $postVals );
echo '<HR>';
var_dump( $image );
//prepare query to insert into events database
$query = "INSERT INTO Events (evntName,evntVenue,evntType,evntDate,fullDesc,imgpath)
VALUES(:evntName,:evntVenue,:evntType,:evntDate,:fullDesc,:imgpath)";
$params = array( ':evntName' => $postVals['eName'],
':evntVenue' => $postVals['eVenue'], 'evntType' => $postVals['eType'],
':evntDate' => $postVals['eDate'], ':fullDesc' => $postVals['Desc'],
':imgpath' => $image );
$connDB = $this->connect();
if ( $connDB ) {
try {
$stmt = $connDB->prepare( $query );
$result = $stmt->execute( $params );
} catch ( PDOException $err ) {
echo $err->getMessage();
exit;
}
} else {
echo 'NO DATABASE CONNECTION';
exit;
}
}
}
userSession class:
<?php
/**
* Class userSession
*/
class userSession {
/**
* @param $sessionVars
*/
public function __construct( $sessionVars ) {
session_start();
$_SESSION['userEmail'] = $sessionVars['user'];
$_SESSION['userID'] = $sessionVars['userID'];
$_SESSION['userFolder'] = 'users/user_' . $_SESSION['userID'];
$_SESSION['sessID'] = session_id();
}
}
2 Answers 2
Executive summary:
- Pick a naming standard and stick to it (img != image for instance)
- Learn how to write comment blocks ( /* description / /* @var */ is kinda weird)
- SOLID is a good place to start (google it)
- PSR-1 and PSR-2
- Don't store html in strings, just don't.
- Again, don't use a class to represent a 'template'. Php does a really good job as a templating engine
<myhtml><?php echo $data ?></myhtml> - your database isn't really abstracted (more on this later)
- I miss interfaces :( (why? I just like them)
So why bother commenting? - You did a good effort in separating concern - you actually wrote some comments - You are using PDO
Long part
(More text will follow, I simply don't feel like writing everything in 1 go):
FileHandler or no, fileHandler
Naming convention is everything . When using code, you should not have to think about syntax. It should all feel very natural. Your code doesn't. Some examples:
the FileHandler class is actually fileHandler. Always start your class names with a capital letter. Why? because. It's a convention, we all do it, it makes writing code easy.
Now, lets say I want to use your fileHandler class. I copy paste it into my project and initialize it:
$myAwesomeStolenFileHandler = new fileHandler();
$myAwesomeStolenFileHandler->createImage($myImage);
OW sh*@t, function not found. Huh? but it's there in the code allright. Ow, wait, it's not a method of the class, it's a function inside a method. I'm ot supposed to use it, so why not make it private? Instead of creating it everytime I call createImage. wow, thats enoying. uh ok, well, I was planning on adding a watermark but i'll wate for SteveBK to implement it I guess. Ok, well lets use the imageresizer then.
$myNotSoAwesomeStolenFileHandler->resizeImage($image);
Again, method not found.
Huh? confusion
Aaah, it's img not image. and in this class somehow it's imgResize and not resizeImage.
Ok, next try;
$myNotSoAwesomeStolenFileHandler->imgResize($image);
Note that I had to read the method itself to be able to know what @param $image actually is (is a String? is it a binary object, is it, ...)
Recap:
- Comments aren't really helping
- A lot of functionality is cramped into one method (and those strange function creations on the fly in a method, iieeewww)
- naming of the methods could be a lot better.
Rewrite:
What does the name FileHandler tell me? Well, without reading the code it tells me that it's a class that aids me in handling files. I could for instance want to get a list of files. Add a file, upload one, rename it, get a FileInstance to do some extra stuff with the file itself, ... You know, the usual stuuf a FileHandler does. it would look something like this:
<?php
interface FileHandler
{
/**
* Define the root path for the FileHandler
* The FileHandler should not be able
* to access files outside this root folder
*
* @param String $root the root path
*/
public function __construct($root);
/**
* Returns an array of all file and directory names in the current directory
* IF $showDirectries == false
* exlude the directories
*
* @param boolean $showDirectories
* @return String[]
*/
public function listAll($showDirectories=true);
public function changeDirectory();
public function getCurrentDirectory();
public function getFile($name);
}
Yes, Id din't finish all the comments. Complains can be written to /dev/null
Interface?
Yes, I wrote an interface and not a class. why? here is the reason. An interface is a contract, It tells us programmers what a certain class/object can do if it implements this interface. This also goes the other way round, if we need an object of a certain interface. we simply type-hint for that interface and we get really nice error messages that help us in the debug proces (for instance if someone is trying to cramp a MouseHandler to your app instead of a FileHandler)
Back to the FileHandler that actually should have been a ImageInstance or ImageFile
As you see, I now wrote a piece of code that has nothing to do with your app, simply because you didn't need a FileHandler. You need a ImageResizer, or whatever you need.
Handling independent files and/or images
Our FileHandler has a method getFile() and let's for the sake of codereview and my essay I'm writing here go all the way and have it a return an object of type FileInstance.
The FileInstance
A FileInstance is a representation of a file. Lucky for us, we don't have to write this ourself, there is a really good built int class for this, the SplFileInfo class
What about my ImageResize
good question, the ImageResizer is a function/method that needs a FileInstance and then does some really cool stuff to it (like resizing). The ImageResizer needs a File or a link to a file. We us the SplFileInfo object here.
ImageResizer inteface
Here we can go for different approaches, I will go for an ImageResizer that has a resize method that resizes a given image according to a certain set of rules (only width is supported here)
<?php
interface ImageResizer
{
/**
* Constructor, set some defaults
* @param int $width the width of the image
*/
public function __construct($width);
/**
* Resize the image according to a set of rules defined in the __construct
*
* @param SplFileInfo $image the image to resize
* @param SplFileInfo $destination the destination for the resized image
* @return void
* @throws MymeTypeNotSupportedException If myme_type<$image> is not supported
* @throws DestinationFileNotWritable If !$destination->isWritable()
*/
public function resize(SplFileInfo $image, SplFileInfo $destination);
}
And now, lets implement the sh*@t out of this interface
<?php
class MimeTypeNotSupportedException extends Exception {}
class DestinationFileNotWritableException extends Exception {}
class NotAFileException extends Exception {}
class FileNotReadableException extends Exception {}
class AwesomeImageResizer implements ImageResizer
{
private $width;
private $infoReader;
/**
* Constructor, set some defaults
*
* @param int $width the width of the image
*/
public function __construct($width)
{
$this->width = $width;
$this->infoReader = new finfo(FILEINFO_MIME_TYPE);
}
/**
* Resize the image according to a set of rules defined in the __construct
*
* @param SplFileInfo $image the image to resize
* @param SplFileInfo $destination the destination for the resized image
* @return void
* @throws NotAFileException If ! $image->isFile()
* @throws MimeTypeNotSupportedException If myme_type<$image> is not supported
* @throws DestinationFileNotWritableException If !$destination->isWritable()
*/
public function resize(SplFileInfo $image, SplFileInfo $destination)
{
#is it a file?
if ( !$image->isFile() )
{
throw new NotAFileException();
}
#is destination writable?
if ( !$destination->isWritable() )
{
throw new DestinationFileNotWritableException();
}
#get the mimeType
$mimeType = $this->infoReader->file($image);
#get the image resource
$imageResource = $this->getImageResource();
#let's do the fancy resizing work
$width = imagesx($imageResource);
$height = imagesy($imageResource)
$newWidth = $this->width;
$newHeight = $height * $newWidth / $width;
$newResource = imagecreatetruecolor($newWidth, $newHeight);
imagecopyresampled(
$newResource,
$imageResource,
0, 0, 0, 0,
$newWidth,
$newHeight,
$width,
$height
);
#create the new image
$newImage = $this->createImage($newResource, $mimeType);
#write the new image to the file
$file = $destination->openFile('w');
$file->fwrite($newImage);
}
/**
* calls the corresponding php method
* to create a resource from a certain mimtype
*
* @param SplFileInfo $image
* @param string $mimeType
* @return resource
* @throws MimeTypeNotSupportedException If myme_type<$image> is not supported
*/
private function getImageResource(SplFileInfo $image, $mimeType)
{
//is the image readable?
if ( !$image->isReadable() )
{
throw new FileNotReadableException();
}
switch ($mimeType) {
case 'image/png':
return imagecreatefrompng($image->getPathName());
break;
case 'image/jpeg':
return imagecreatefromjpeg($image->getPathName());
break;
case 'image/gif' :
return imagecreatefromgif($image->getPathName());
break;
default:
#sorry, can't help ya
throw new MimeTypeNotSupportedException($mimeType);
break;
}
}
private function createImage($resource, $mimeType)
{
switch ($mimeType) {
case 'image/png':
return imagepng($resource);
break;
case 'image/jpeg':
return imagejpeg($resource);
break;
case 'image/gif' :
return imagegif($resource);
break;
default:
#sorry, can't help ya
throw new MimeTypeNotSupportedException();
break;
}
}
}
Wow, what code, so much wow.
This is however untested and only written for explanation ;)
Some usage examples:
<?php
$mediumResizer = new AwesomeImageResizer(800);
$mediumResizer->resize(
new SplFileInfo('my/current/image.png'),
new SplFileInfo('my/destination/image.png')
);
#and agin with error handling
try {
$mediumResizer->resize(
new SplFileInfo('other/current/image.png'),
new SplFileInfo('other/destination/image.png')
);
} catch ( NotAFileException $e )
{
echo "Oh nows, the image isn't even a file";
die();
} catch( FileNotReadableException $e)
{
echo "Aaahhh, I'm blind. The file is unreadable, aaaahhhhhhhhh!";
} catch ( MimeTypeNotSupportedException $e )
{
echo "Oh nows, ze program has no clue what to with your mime_type: " . $e->getMessage();
die();
} catch ( DestinationFileNotWritableException $e )
{
echo "Oh nows, the destination file doesn't want me to write to it :(";
die();
}
But what about all those throws, and exceptions? Well, when something goes wrong, you should not echo an error message, or die();
See how I'm handling errors outside of my ImageReszier? this is good. Because let's face it. An ImageResizer that has to handle resizing of images AND error handling, that's a lot. Even for our AwesomeImageResizer, we all have a limit ;)
Ok, I hope that with writing this huuuuuuge ImageResizer I have given you a better biew into writing OO code.
Ok, it's a bit of a long read, but I'll come back soon with more text ;)
On Comments:
A good read: http://pear.php.net/manual/en/standards.sample.php Note ofcourse that this is proabably overkill. But it's good to know how far you can push it ;)
-
\$\begingroup\$ Hi Pinoniq...I appreciate the time you've taken to do all this. As I said, im new to both OOP and PHP and this seems a little over my head. I'm sure I'll come to understand it in time. For now though...u mentioned that my DB class is not abstracted, this seems a key thing. I'd like to see how that's wrong before moving on to other stuff like class interfaces which I haven't even read about yet. I'm still trying to come to a thorough understanding of the fundamentals.I'm just starting a PHP certification course everything you've done here will definately come in handy \$\endgroup\$steveBK– steveBK2014年06月04日 15:20:00 +00:00Commented Jun 4, 2014 at 15:20
-
\$\begingroup\$ aha, I'll edit my post this evening with some more basic stuff :) \$\endgroup\$Pinoniq– Pinoniq2014年06月04日 15:21:30 +00:00Commented Jun 4, 2014 at 15:21
Security
SQL Injection
Sometimes you use prepared statements, which is good, but other times you don't. This is vulnerable:
$usrFind = 'Select email,password,salt,usrID from users where email=' . $usrLogin;
XSS
print_r( $postVals ); looks like a debug statement, but even debug statements should be secure. If you don't expect arrays as input, this would be a simple solution: print_r(array_map('htmlentities', $postVals));
File Upload
$_FILES['upFile']['type'] is completely user controlled, so an attacker can upload for example PHP files which would result in code execution.
What you should do is check the file extension as well as the actual file type. The functions that are generally recommended for this are pathinfo and finfo_file respectively. Ideally, you should use whitelists, not blacklists.
It is a good idea to additionally forbid execution inside the upload directory, ideally by using server configuration as well as OS configuration, but this should not be your only line of defense.
You are currently setting these rights:
mkdir( $dir[$i], 0764, true );
chmod( $dir[$i], 0764 );
This means read for anyone, read/write for the group, and read/write/execute for the owner. The problem is that the owner is the webserver, as the webserver is the one creating these files. To make the directory actually non-executable, 664 or similar would be better.
Misc
- echoing database errors to end users is not a good idea. It simplified finding of security issues, and actually aids in some attacks such as SQL injections into
INSERT, which show retrieved data in error messages. - you should use
session_regenerate_id(true)when the state of a session changes to prevent session fixation (not much of an issue with default php.ini settings, but still).
Explore related questions
See similar questions with these tags.
createNewUserandfindUsershould be abstracted from the database class \$\endgroup\$