Trenger litt kritiske øyne!

[[kami]]

Heisann,

 

Dette er det første php scriptet jeg har laget på mange år nå.. Sist jeg koda php var php 4 nytt og fett.. så jeg mistenker at det er et og annnet jeg har gått glipp av i det siste. (til vanlig koder jeg c++)

 

Jeg har tenkt å lage en media database som inneholder alt jeg har av movies/serier og linke det opp mot imdb for så å vise det i nettleser.

 

Så jeg starta så smått i går på database backend. Jeg leita litt etter klasser å bruke, men tenkte dette i grunn var en fin treningssak for å komme meg litt inn i php kode igjen.

 

noen spm i samme slengen:

- Støtter ikke mysql/php bind variables? jeg tok nesten dette for gitt (har koda mot oracle i det siste), men da jeg skulle binde fant jeg ikke noe så endte med å kjøre en search og replace på binds'ene mine.

- sql injection. med bind variabler er jo ikke dette noe stress, men nå måtte jeg litt overraskende tenke på det. har jeg driti meg ut noen sted?

- har jeg gjort noe sinnsykt uoptimalt noe sted? note, jeg er stor fan av lesbar kode, og klassen min trenger ikke skalere til tusenvis av brukere. Men har jeg gjort noe teit er jeg klar for pekere i riktig retning

- er exceptions egentlig en kul ting å bruke i php? eller gjør det script mye tregere?

- hvordan får jeg color highlights på code som er posta her?

 

For å kjøre koden trenger man denne tabellen i databasen.

<?php
/**
* Database abstraction class
*
* Abstracts the SQL used to perform common operations on databases.   Handles Insert, Update, Select and Delete.
*
* @license    LGPL 
*
* @author Anders Elton
*
* @copyright Anders Elton
*/


/**
*
* This class is an enumerator for field types in databse
*
* @author Anders Elton
*
* @copyright Anders Elton
*
*/
class FieldType
{
  const Uknown = 1;
  const Number = 2;
  const String = 3;  
}

/**
*
* This class defines a table that exist in the database.  You are expected to
* provide the fields you want access to.
* After defining how the table looks you can use this class to generate SQL
* that is able to fetcj, update or delete the content in the table.
*
* @author Anders Elton
*
* @copyright Anders Elton
*/
class SQLTable
{
  public function __construct($tableName) {
    $this->m_TableName = $tableName;
 $this->m_Columns = array();
 $this->m_KeyColumns = array();
    $this->m_SelectParametersNeeded = 0;
    $this->m_InsertParametersNeeded = 0;
    $this->m_DeleteParametersNeeded = 0;
    $this->m_UpdateParametersNeeded = 0;
  }
  public function __destruct() {
  }

/**	
*
* Defines that the Database table has a number
*
* @author Anders Elton
*
* @access public
*
* @var STRING Name of the number field
*
* @return a reference to self
*/   
  public function &AddNumber($fieldName) {
    if (array_key_exists($fieldName, $this->m_Columns)) {
   throw new Exception("Error: Column $fieldName already exist in table $m_TableName");
 }
    $this->m_Columns[$fieldName] = FieldType::Number;
 return $this;
  }

/**	
*
* Defines that the Database table has a string
*
* @author Anders Elton
*
* @access public
*
* @var STRING Name of the number field
*
* @return a reference to self
*/   
  public function &AddString($fieldName) {
    if (array_key_exists($fieldName, $this->m_Columns)) {
   throw new Exception("Error: Column $fieldName already exist in table $m_TableName");
 }
    $this->m_Columns[$fieldName] = FieldType::String;
 return $this;
  }

/**	
*
* Marks a previously added field as a key column.  This means it will be used in SELECT and UPDATE
* statements.  A key column will always be an argument for Select(), Update() and Delete() functions.
*
* @author Anders Elton
*
* @access public
*
* @var STRING Name of the number field
*
* @return a reference to self
*/     
  public function &AddKeyColumn($fieldName) {
    if (!array_key_exists($fieldName, $this->m_Columns)) {
   throw new Exception("Error: key $fieldName does not exist in table $m_TableName");
 }
 $this->m_KeyColumns[] = $fieldName;
 return $this;
  }

/**	
*
* Generates a Select SQL string based on the definitions we have on the table.
* The SQL is generated using a "bind variable" syntax.
*
* Example of a string returned is: select a,b,c where a=1, b=2
*
* @author Anders Elton
*
* @access public
*
* @return The SELECT SQL
*/     
  public function GetSelectString() {
    if (!isset($this->m_SelectStringCache) && !empty($this->m_Columns)) {
   reset($this->m_Columns);
   $this->m_SelectStringCache = "select ";
   $firstCol = true;
   while (list($key, $value) = each($this->m_Columns)) {
     if (!$firstCol) $this->m_SelectStringCache .= ",";
	 else $firstCol = false;
        $this->m_SelectStringCache .= $key;		 
   }
      $counter = 0;
   $this->m_SelectStringCache .= " from " . $this->m_TableName;
   if (!empty($this->m_KeyColumns)) {
     $this->m_SelectStringCache .= " where ";
	 foreach ($this->m_KeyColumns as $column) {
	   if ($counter != 0) $this->m_SelectStringCache .= " and ";
	   $counter++;
	   if ($this->m_Columns[$column] == FieldType::String) {
	     $this->m_SelectStringCache .= $column . "=':$counter'";
	   } else {
	     $this->m_SelectStringCache .= $column . "=:$counter";
	   }
	 }
   }
   $this->m_SelectParametersNeeded = $counter;
 }
 return $this->m_SelectStringCache;
  }

/**	
*
* Generates a Insert SQL string based on the definitions we have on the table.
* The SQL is generated using a "bind variable" syntax.
*
* Example of a string returned is: insert into A (a,b,c) values (1,2,3);
*
* @author Anders Elton
*
* @access public
*
* @return The INSERT SQL where arguments are replaced with :1, :2, :3 etc.
*/   
  public function GetInsertString() {
    if (!isset($this->m_InsertStringCache) && !empty($this->m_Columns)) {
   reset($this->m_Columns);
   $this->m_InsertStringCache = "insert into " . $this->m_TableName . "(";
   $firstCol = true;
   while (list($key, $value) = each($this->m_Columns)) {
     if (!$firstCol) $this->m_InsertStringCache .= ",";
	 else $firstCol = false;
        $this->m_InsertStringCache .= $key;		 
   }
   $this->m_InsertStringCache .= ") values (";
   $counter = 0;
   reset($this->m_Columns);
   while (list($key, $value) = each($this->m_Columns)) {
     if ($counter != 0) $this->m_InsertStringCache .= ",";
	  ++$counter;
	   if ($this->m_Columns[$key] == FieldType::String) {
	     $this->m_InsertStringCache .= "':$counter'";
	   } else {
	     $this->m_InsertStringCache .= ":$counter";
	   }
   }
   $this->m_InsertStringCache .= ")";
   $this->m_InsertParametersNeeded = $counter;
 }
 return $this->m_InsertStringCache;   
  }

/**	
*
* Generates a Delete SQL string based on the definitions we have on the table.
* The SQL is generated using a "bind variable" syntax.
*
* Example of a string returned is: delete from A where a=1 and b=2 and c=3
*
* Note that if no Key comuns are defined for this table a the SQL generated will
* erase all records in the table.
*
* @author Anders Elton
*
* @access public
*
* @return The Delete SQL
*/   

  public function GetDeleteString() {
    if (!isset($this->m_DeleteStringCache) ) {
   reset($this->m_Columns);
   $this->m_DeleteStringCache = "delete from " . $this->m_TableName;
   $counter = 0;
   if (!empty($this->m_KeyColumns)) {
     $this->m_DeleteStringCache .= " where ";
     foreach ($this->m_KeyColumns as $key) {
       if ($counter != 0) $this->m_DeleteStringCache .= " and ";
	   ++$counter;
	   if ($this->m_Columns[$key] == FieldType::String) {
	     $this->m_DeleteStringCache .= $key . "=':$counter'";
          } else {
	     $this->m_DeleteStringCache .= $key . "=:$counter";
	   }
	 }
   }
   $this->m_DeleteParametersNeeded = $counter;	   
 }
 return $this->m_DeleteStringCache;
  }

/**	
*
* Generates a Update SQL string based on the definitions we have on the table.
* The SQL is generated using a "bind variable" syntax.
*
* Example of a string returned is: update A set a=1, b=2 where c=3
*
* @author Anders Elton
*
* @access public
*
* @return The SELECT SQL
*/   
  public function GetUpdateString() {
    if (!isset($this->m_UpdateStringCache) && !empty($this->m_Columns) ) {
   reset($this->m_Columns);
   $this->m_UpdateStringCache = "update " . $this->m_TableName . " set ";
   $firstCol = true;
   $counter = 0;
   while (list($key, $value) = each($this->m_Columns)) {
     if ($counter != 0) $this->m_UpdateStringCache .= ",";
	 ++$counter;
	 if ($value == FieldType::String) {
          $this->m_UpdateStringCache .= $key . "=':$counter'";
	 } else {
          $this->m_UpdateStringCache .= $key . "=:$counter";
	 }
   }
   if (!empty($this->m_KeyColumns)) {
     $this->m_UpdateStringCache .= " where ";
	 $wherecounterstart = $counter;
	 foreach ($this->m_KeyColumns as $column) {
	   if ($counter != $wherecounterstart) $this->m_UpdateStringCache .= " and ";
	   ++$counter;
	   if ($this->m_Columns[$column] == FieldType::String) {
	     $this->m_UpdateStringCache .= $column . "=':$counter'";
	   } else {
	     $this->m_UpdateStringCache .= $column . "=:$counter";
	   }
	 }
   }
   $this->m_UpdateParametersNeeded = $counter;	   	   
 }
 return $this->m_UpdateStringCache;
  } 

/**	
*
* Generates a Select statement that can be executed to select records from the database
*
* @author Anders Elton
*
* @access public
*
* @return a statement that will Select records from the database
*/   
  public function Select() {
    return new SQLStatement($this, $this->GetSelectString(), $this->m_SelectParametersNeeded );
  }

/**	
*
* Generates a Insert statement that can be executed to Insert records into the database
*
* @author Anders Elton
*
* @access public
*
* @return a statement that will Insert records to the database
*/   
  public function Insert() {
    return new SQLStatement($this, $this->GetInsertString(), $this->m_InsertParametersNeeded );
  }

/**	
*
* Generates a Delete statement that can be executed to remove records from the database
*
* @author Anders Elton
*
* @access public
*
* @return a statement that will delete records from the database
*/      
  public function Delete() {
    return new SQLStatement($this, $this->GetDeleteString(), $this->m_DeleteParametersNeeded );
  }

/**	
*
* Generates a Update statement that can be executed to Update already existing records in the database
*
* @author Anders Elton
*
* @access public
*
* @return a statement that will Update records from the database
*/     
  public function Update() {
    return new SQLStatement($this, $this->GetUpdateString(), $this->m_UpdateParametersNeeded );
  }

/**	
*
* Get the table name
*
* @author Anders Elton
*
* @access public
*
* @return the table name associated with this object
*/      
  public function GetTableName() { return $this->m_TableName; }

  // vars   
  private $m_TableName;  // this is the name of the table in the database.
  private $m_Columns;    // this is an array that contains all the columns in the table we have defined.
                         // Note that you only need to define the columns you want to select or modify.
  private $m_KeyColumns; // This array contains the Keys, ie. usually the index of you table.  This will be used
                         // to create the criteria to fetch the row you want.  The Key must be defined in m_Columns first.

  // below are cached values. we only need to build the string once and calculate arguments once.
  private $m_SelectStringCache;
  private $m_SelectParametersNeeded;
  private $m_InsertStringCache;
  private $m_InsertParametersNeeded;
  private $m_DeleteStringCache;
  private $m_DeleteParametersNeeded;
  private $m_UpdateStringCache;
  private $m_UpdateParametersNeeded;  
}

/**
*
* This class defines a SQL statement.
* A SQL statement is a object that can manipulte database records
* The Statement have different behaviour based on what we are trying to do and depending on
* the SQLTable definition we are using.  the SQLTable defines what arguments you must supply,
* or else this class will throw exceptions.
*
* An SQLStatement object can only be Executed once.  After Execute you may call Next() to get the next aviable
* result for Select() Statements.
*
* Insert(), Delete() and Update() Statements do not return any results
*
* @author Anders Elton
*
* @copyright Anders Elton
*/
class SQLStatement
{
  public function __construct(&$sqlTable, $sqlString, $parametersNeeded) {
    $this->m_SQLTable = $sqlTable;
 $this->m_SQLString = $sqlString;
 $this->m_Values = array();
 $this->m_Executed = false;
 $this->m_ParametersNeeded = $parametersNeeded;
 $this->m_FetchResult = 0;
 $this->m_QueryResult = 0;
  }
  public function __destruct() {
    if ($this->m_QueryResult !== true && $this->m_QueryResult !== false) {
      mysql_free_result($this->m_QueryResult);
 }
  }
  public function SetNumber($key, $value) {
    $this->m_Values[$key] = $value;
  }
  public function SetString($key, $value) {
    $this->m_Values[$key] = $value;
  }

/**	
*
* Execute this statement on the database.
* Before this method is called all expected parameters must be defined through SetNumber() or SetString() function.
*
* @author Anders Elton
*
* @access public
*
* @return false if the statment failed to execute (database error)
*/    
  public function Execute() {
    if ($this->m_ParametersNeeded != count($this->m_Values)) {
    throw new Exception("Error: Missing parameters for SQL in table " . $this->m_SQLTable->GetTableName() . ", expected: " . $this->m_ParametersNeeded ." got " . count($this->m_Values));
	return false;
 }
    if ($this->m_Executed) throw new Exception("Error: Impossible to execute twice.");
    $this->m_Executed = true;
 if (!empty($this->m_Values)) { 
      $this->m_SQLString = strtr($this->m_SQLString, $this->m_Values);
 }
 $this->m_QueryResult = mysql_query($this->m_SQLString);
 return $this->m_QueryResult;
  }

  public function Next()
  {
    $this->m_FetchResult = mysql_fetch_array($this->m_QueryResult);
 return $this->m_FetchResult;
  }

  public function GetNumber($key) {
    if (!$this->m_FetchResult) throw new Exception("Fetch result not aviable");
 return $this->m_FetchResult[$key];
  }

  public function GetString($key) {
    if (!$this->m_FetchResult) throw new Exception("Fetch result not aviable");
 return $this->m_FetchResult[$key];
  }


  private $m_SQLTable;
  private $m_Counter;
  private $m_Values;
  private $m_Executed;
  private $m_SQLString;
  private $m_ParametersNeeded;
  private $m_QueryResult;
  private $m_FetchResult;

}
?>

 

 

liten unittest som verifiserer funksjonaliteten

<?php
require_once("database.class.php");

// minitest
// create table testuser (username varchar(255), password varchar(255), info varchar(255));
$link = mysql_connect('localhost', 'popcorn', '');
mysql_select_db('test');

$oneUserTbl = new SQLTable("testuser");
$oneUserTbl->AddString("username")->
      AddString("password")->
   AddString("info")->
   AddKeyColumn("username")->
   AddKeyColumn("password");
$allUsers = new SQLTable("testuser");
$allUsers->AddString("username")->
      AddString("password")->
   AddString("info");

// First clean up any old users that exist in db.	   
$deleteStm = $allUsers->Delete();
$deleteStm->Execute();

// insert some test dummies	   
for ($i = 0; $i < 10; ++$i) {
 $insertStm = $oneUserTbl->Insert();
 $insertStm->SetString(':1',"user$i");
 $insertStm->SetString(':2',"somepassword");
 $insertStm->SetString(':3',"testuser");
 $insertStm->Execute();
}

$selectStm = $allUsers->Select();
$expectedCount = 0;
if ($selectStm->Execute()) {
 while ($selectStm->Next()) {
if ($selectStm->GetString('username') != "user$expectedCount") die("Expected username$expectedCount got " . $selectStm->GetString('username'));
echo $selectStm->GetString('password'). ",info=";
echo $selectStm->GetString('info'). "\n";
   ++$expectedCount;
 }
}
if ($expectedCount != 10) die ("The expected amount of users was not saved to the database.");

$updateStm = $oneUserTbl->Update();
$updateStm->SetString(':1', "changeduser");
$updateStm->SetString(':2', "changedpassword");
$updateStm->SetString(':3', "some changed info");
$updateStm->SetString(':4', "user0");
$updateStm->SetString(':5', "somepassword");
if ($updateStm->Execute()) {
} 

$expectedCount = 0;
$selectAUserStm = $oneUserTbl->Select();
$selectAUserStm->SetString(':1', "changeduser");
$selectAUserStm->SetString(':2', "changedpassword");
if ($selectAUserStm->Execute()) {
 while ($selectAUserStm->Next()) {
   ++$expectedCount;
 }
}
if ($expectedCount != 1) die("didnt manage to update user in db..");

$deleteStm = $allUsers->Delete();
$deleteStm->Execute();

$expectedCount = 0;
$selectStm = $allUsers->Select();
if ($selectStm->Execute()) {
 while ($selectStm->Next()) {
 ++$expectedCount;
 }
}

if ($expectedCount != 0) die("Expected no users to be in database.");

echo "test finished successfully\n";
?>

[Ernie]

noen spm i samme slengen:

- Støtter ikke mysql/php bind variables? jeg tok nesten dette for gitt (har koda mot oracle i det siste), men da jeg skulle binde fant jeg ikke noe så endte med å kjøre en search og replace på binds'ene mine.

Både ja og nei. MySQL-funksjonene har ingen støtte for slikt, men det gjør PDO (og det finnes en PDO driver til blant annet MySQL) og MySQLi (den objektorienterte delen).

 

- sql injection. med bind variabler er jo ikke dette noe stress, men nå måtte jeg litt overraskende tenke på det. har jeg driti meg ut noen sted?

Jepp, du er vid åpen for SQL injection. Grunnen til dette er at du ikke «escape»-er inndataene i det heltatt. Skal du bruke mysql-funksjonene må all inndata innom mysql_real_escape_string eller tilsvarende før man dytter variablene inn i en spørring, noe jeg ikke kan se at du gjør. PDO og MySQLi har støtte for «prepared statements» og har dermed ikke det samme problemet.

 

- er exceptions egentlig en kul ting å bruke i php? eller gjør det script mye tregere?

Tja, kult og kult. Det er vel sikkert noen som synes det, men det er vel ganske uviktig. Det er vel mer et spørsmål om man har nytte av det. Hastighet skal iallfall ikke være noe stort problem. Dvs. iallfall ikke med mindre du spytter ut et par tusen av de hver gang scriptet kjører.

 

- hvordan får jeg color highlights på code som er posta her?

Tror ikke det er mulig.

[[kami]]

tusen takk for at du tok deg tid til å lese gjennom! Dette beviser nok en gang at flere øyne og hjerner funker bedre enn èn

 

Skal fikse sql injection med escape, takk for tipset!

 

men enda bedre var dette med PDO! akuratt hva jeg håpet og trodde php hadde et eller annet sted

[pan100]

tusen takk for at du tok deg tid til å lese gjennom! Dette beviser nok en gang at flere øyne og hjerner funker bedre enn èn

 

Skal fikse sql injection med escape, takk for tipset!

 

men enda bedre var dette med PDO! akuratt hva jeg håpet og trodde php hadde et eller annet sted

 

Du fikser escapen med mysql_real_escape_string(); :-)

 

EDIT: så ikke at det stod der allerede

Endret 7. april 2009 av pan100

[Black Star]

Hei!

 

Emnetittelen i denne tråden er lite beskrivende for trådens innhold og det er derfor ingen god emnetittel. Jo bedre og mer beskrivende emnetittelen er, jo lettere er det for andre å skjønne trådens innhold og det vil være lettere å treffe den riktige forumbrukeren med det rette svaret. Ber deg derfor om å endre emnetittel. Vennligst forsøk å ha dette i tankene neste gang du starter en tråd, og orienter deg om hva vår nettikette sier om dårlig bruk av emnetitler.

 

Husk at en god emnetittel skal beskrive eller oppsummere hvilket problem du har - ikke at du har et problem. En god emnetittel skal heller ikke kun bestå av et produktnavn.

 

Bruk -knappen i første post for å endre emnetittelen.

 

(Dette innlegget vil bli fjernet ved endring av emnetittel. Ikke kommenter dette innlegget, men gjerne dette innlegget når tittelen er endret, så vil det bli fjernet..)