Merge pull request #22 from flightphp/windows-and-escape-fixes

Windows and escape fixes
flightphp · Jan 4, 2025 · e44cfab · e44cfab
2 parents 3b52872 + 3650a3a
commit e44cfab
Show file tree

Hide file tree

Showing 7 changed files with 139 additions and 58 deletions.
diff --git a/README.md b/README.md
@@ -1,8 +1,8 @@
 # FlightPHP Active Record 
-[![Latest Stable Version](http://poser.pugx.org/flightphp/active-record/v)](https://packagist.org/packages/flightphp/active-record)
+[![Latest Stable Version](https://poser.pugx.org/flightphp/active-record/v)](https://packagist.org/packages/flightphp/active-record)
 [![License](https://poser.pugx.org/flightphp/active-record/license)](https://packagist.org/packages/flightphp/active-record)
-[![PHP Version Require](http://poser.pugx.org/flightphp/active-record/require/php)](https://packagist.org/packages/flightphp/active-record)
-[![Dependencies](http://poser.pugx.org/flightphp/active-record/dependents)](https://packagist.org/packages/flightphp/active-record)
+[![PHP Version Require](https://poser.pugx.org/flightphp/active-record/require/php)](https://packagist.org/packages/flightphp/active-record)
+[![Dependencies](https://poser.pugx.org/flightphp/active-record/dependents)](https://packagist.org/packages/flightphp/active-record)
 
 An active record is mapping a database entity to a PHP object. Spoken plainly, if you have a users table in your database, you can "translate" a row in that table to a `User` class and a `$user` object in your codebase. See [basic example](#basic-example).
 

diff --git a/composer.json b/composer.json
@@ -26,7 +26,7 @@
         "phpunit/phpunit": "^9.0",
         "squizlabs/php_codesniffer": "^3.8",
         "rregeer/phpunit-coverage-check": "^0.3.1",
-        "flightphp/runway": "^0.2"
+        "flightphp/runway": "^0.2.4 || ^1.0"
     },
 	"autoload": {
 		"psr-4": {"flight\\": "src/"}
@@ -35,6 +35,6 @@
 		"test": "phpunit",
 		"test-coverage": "XDEBUG_MODE=coverage vendor/bin/phpunit --coverage-html=coverage --coverage-clover=clover.xml && vendor/bin/coverage-check clover.xml 100",
 		"beautify": "phpcbf --standard=phpcs.xml",
-		"phpcs": "phpcs --standard=phpcs.xml"
-	 }
+		"phpcs": "phpcs -n --standard=phpcs.xml"
+	}
 }
diff --git a/src/ActiveRecord.php b/src/ActiveRecord.php
@@ -7,6 +7,8 @@
 use Exception;
 use flight\database\DatabaseInterface;
 use flight\database\DatabaseStatementInterface;
+use flight\database\mysqli\MysqliAdapter;
+use flight\database\pdo\PdoAdapter;
 use JsonSerializable;
 use mysqli;
 use PDO;
@@ -93,15 +95,20 @@ abstract class ActiveRecord extends Base implements JsonSerializable
     /**
      * Database connection
      *
-     * @var DatabaseInterface
+     * @var DatabaseInterface|null
      */
-    protected DatabaseInterface $databaseConnection;
+    protected ?DatabaseInterface $databaseConnection;
 
     /**
      * @var string  The table name in database.
      */
     protected string $table;
 
+    /**
+     * @var string The type of database engine
+     */
+    protected string $databaseEngineType;
+
     /**
      * @var string  The primary key of this ActiveRecord, only supports single primary key.
      */
@@ -162,8 +169,12 @@ public function __construct($databaseConnection = null, ?string $table = '', arr
             $this->transformAndPersistConnection($rawConnection);
         } elseif ($databaseConnection instanceof DatabaseInterface) {
             $this->databaseConnection = $databaseConnection;
+        } else {
+            $this->databaseConnection = null;
         }
 
+        $this->databaseEngineType = $this->getDatabaseEngine();
+
         if ($table) {
             $this->table = $table;
         }
@@ -193,9 +204,9 @@ public function __call($name, $args)
                 'operator' => ActiveRecordData::SQL_PARTS[$name],
                 'target' => implode(', ', $args)
             ]);
-        } else if(method_exists($this->databaseConnection, $name) === true) {
-			return call_user_func_array([ $this->databaseConnection, $name ], $args);
-		}
+        } elseif (method_exists($this->databaseConnection, $name) === true) {
+            return call_user_func_array([ $this->databaseConnection, $name ], $args);
+        }
         return $this;
     }
 
@@ -317,7 +328,7 @@ protected function resetQueryData(): self
     {
         $this->params = [];
         $this->sqlExpressions = [];
-		$this->join = null;
+        $this->join = null;
         return $this;
     }
     /**
@@ -391,6 +402,23 @@ public function setDatabaseConnection($databaseConnection): void
         }
     }
 
+    /**
+     * Returns the type of database engine. Can be one of: mysql, pgsql, sqlite, oci, sqlsrv, odbc, ibm, informix, firebird, 4D, generic.
+     *
+     * @return string
+     */
+    public function getDatabaseEngine(): string
+    {
+        if ($this->databaseConnection instanceof PdoAdapter || is_subclass_of($this->databaseConnection, PDO::class) === true) {
+            // returns value of mysql, pgsql, sqlite, oci, sqlsrv, odbc, ibm, informix, firebird, 4D, generic.
+            return $this->databaseConnection->getConnection()->getAttribute(PDO::ATTR_DRIVER_NAME);
+        } elseif ($this->databaseConnection instanceof MysqliAdapter) {
+            return 'mysql';
+        } else {
+            return 'generic';
+        }
+    }
+
     /**
      * function to find one record and assign in to current object.
      * @param int|string $id If call this function using this param, will find record by using this id. If not set, just find the first record in database.
@@ -449,9 +477,14 @@ public function insert(): ActiveRecord
         }
 
         $value = $this->filterParam($this->dirty);
+
+        // escape column names from dirty data
+        $columnNames = array_keys($this->dirty);
+        $escapedColumnNames = array_map([$this, 'escapeIdentifier'], $columnNames);
+
         $this->insert = new Expressions([
-            'operator' => 'INSERT INTO ' . $this->table,
-            'target' => new WrapExpressions(['target' => array_keys($this->dirty)])
+            'operator' => 'INSERT INTO ' . $this->escapeIdentifier($this->table),
+            'target' => new WrapExpressions(['target' => $escapedColumnNames])
         ]);
         $this->values = new Expressions(['operator' => 'VALUES', 'target' => new WrapExpressions(['target' => $value])]);
 
@@ -622,9 +655,9 @@ protected function &getRelation(string $name)
     protected function buildSqlCallback(string $sql_statement, ActiveRecord $object): string
     {
         if ('select' === $sql_statement && null == $object->$sql_statement) {
-            $sql_statement = strtoupper($sql_statement) . ' ' . $object->table . '.*';
+            $sql_statement = strtoupper($sql_statement) . ' ' . $this->escapeIdentifier($object->table) . '.*';
         } elseif (('update' === $sql_statement || 'from' === $sql_statement) && null == $object->$sql_statement) {
-            $sql_statement = strtoupper($sql_statement) . ' ' . $object->table;
+            $sql_statement = strtoupper($sql_statement) . ' ' . $this->escapeIdentifier($object->table);
         } elseif ('delete' === $sql_statement) {
             $sql_statement = strtoupper($sql_statement) . ' ';
         } else {
@@ -723,7 +756,7 @@ public function addCondition(string $field, string $operator, $value, string $de
         // skip adding the `table.` prefix if it's already there or a function is being supplied.
         $skip_table_prefix = (strpos($field, '.') !== false || strpos($field, '(') !== false);
         $expressions = new Expressions([
-            'source' => ('where' === $name && $skip_table_prefix === false ? $this->table . '.' : '') . $field,
+            'source' => ('where' === $name && $skip_table_prefix === false ? $this->escapeIdentifier($this->table) . '.' : '') . $this->escapeIdentifier($field),
             'operator' => $operator,
             'target' => (
                 is_array($value)
@@ -816,6 +849,28 @@ protected function processEvent($event, array $data_to_pass = [])
         }
     }
 
+
+    /**
+     * Escapes a database identifier (e.g., table or column name) to prevent SQL injection.
+     *
+     * @param string $name The database identifier to be escaped.
+     * @return string The escaped database identifier.
+     */
+    public function escapeIdentifier(string $name)
+    {
+        switch ($this->databaseEngineType) {
+            case 'sqlite':
+            case 'pgsql':
+                return '"' . $name . '"';
+            case 'mysql':
+                return '`' . $name . '`';
+            case 'sqlsrv':
+                return '[' . $name . ']';
+            default:
+                return $name;
+        }
+    }
+
     /**
      * @inheritDoc
      */

diff --git a/src/database/pdo/PdoAdapter.php b/src/database/pdo/PdoAdapter.php
@@ -42,4 +42,14 @@ public function lastInsertId()
     {
         return $this->pdo->lastInsertId();
     }
+
+    /**
+     * Returns a PDO connection to the database.
+     *
+     * @return PDO The PDO connection instance.
+     */
+    public function getConnection(): PDO
+    {
+        return $this->pdo;
+    }
 }
diff --git a/tests/ActiveRecordPdoIntegrationTest.php b/tests/ActiveRecordPdoIntegrationTest.php
@@ -57,6 +57,9 @@ public function testInsert()
         $user->password = md5('demo');
         $user->insert();
         $this->assertGreaterThan(0, $user->id);
+        $sql = $user->getBuiltSql();
+        $this->assertStringContainsString('INSERT INTO "user" ("name","password")', $sql);
+        $this->assertStringContainsString('VALUES (:ph1,:ph2)', $sql);
     }
 
     public function testInsertNoChanges()
@@ -81,6 +84,9 @@ public function testEdit()
         $this->assertEquals('demo1', $user->name);
         $this->assertNotEquals($original_password, $user->password);
         $this->assertEquals($original_id, $user->id);
+
+        $sql = $user->getBuiltSql();
+        $this->assertStringContainsString('UPDATE "user"  SET "name" = :ph3 , "password" = :ph4   WHERE "user"."id" = :ph5', $sql);
     }
 
     public function testUpdateNoChanges()
@@ -183,7 +189,7 @@ public function testJoin()
         $this->assertEquals($user->address, $contact->address);
     }
 
-	public function testJoinIsClearedAfterCalledTwice()
+    public function testJoinIsClearedAfterCalledTwice()
     {
         $user = new User(new PDO('sqlite:test.db'));
         $user->name = 'demo';
@@ -202,8 +208,8 @@ public function testJoinIsClearedAfterCalledTwice()
         $this->assertEquals($user->email, $contact->email);
         $this->assertEquals($user->address, $contact->address);
 
-		$user->select('*, c.email, c.address')->join('contact as c', 'c.user_id = user.id')->find();
-		// email and address will stored in user data array.
+        $user->select('*, c.email, c.address')->join('contact as c', 'c.user_id = user.id')->find();
+        // email and address will stored in user data array.
         $this->assertEquals($user->id, $contact->user_id);
         $this->assertEquals($user->email, $contact->email);
         $this->assertEquals($user->address, $contact->address);
@@ -228,6 +234,8 @@ public function getDirty()
         $contact->insert();
 
         $user->isNotNull('id')->eq('id', 1)->lt('id', 2)->gt('id', 0)->find();
+        $sql = $user->getBuiltSql();
+        $this->assertStringContainsString('SELECT "user".* FROM "user"   WHERE "user"."id" IS NOT NULL  AND "user"."id" = 1 AND "user"."id" < 2 AND "user"."id" > 0', $sql);
         $this->assertGreaterThan(0, $user->id);
         $this->assertSame([], $user->getDirty());
         $user->name = 'testname';
@@ -277,12 +285,15 @@ public function testDelete()
         $this->assertEquals($uid, $new_user->eq('id', $uid)->find()->id);
         $this->assertTrue($contact->user->delete());
         $this->assertTrue($contact->delete());
+
+        $sql = $contact->getBuiltSql();
         $new_contact = new Contact(new PDO('sqlite:test.db'));
         $new_user = new User(new PDO('sqlite:test.db'));
         $this->assertInstanceOf(Contact::class, $new_contact->eq('id', $cid)->find());
         $this->assertEmpty($new_contact->id);
         $this->assertInstanceOf(User::class, $new_user->find($uid));
         $this->assertEmpty($new_user->id);
+        $this->assertStringContainsString('DELETE  FROM "contact"  WHERE "contact"."id" = :ph4', $sql);
     }
 
     public function testDeleteWithConditions()
@@ -654,10 +665,10 @@ public function testTextBasedPrimaryKeyDuplicateKey()
         $myTextTable2->save();
     }
 
-	public function testCallMethodPassingToPdoConnection()
-	{
-		$result = $this->ActiveRecord->prepare('SELECT * FROM user');
-		$this->assertInstanceOf(PdoStatementAdapter::class, $result);
-		$this->assertNotInstanceOf(ActiveRecord::class, $result);
-	}
+    public function testCallMethodPassingToPdoConnection()
+    {
+        $result = $this->ActiveRecord->prepare('SELECT * FROM user');
+        $this->assertInstanceOf(PdoStatementAdapter::class, $result);
+        $this->assertNotInstanceOf(ActiveRecord::class, $result);
+    }
 }
diff --git a/tests/ActiveRecordTest.php b/tests/ActiveRecordTest.php
@@ -13,6 +13,7 @@ class ActiveRecordTest extends \PHPUnit\Framework\TestCase
     public function testMagicSet()
     {
         $pdo_mock = $this->createStub(PDO::class);
+        $pdo_mock->method('getAttribute')->willReturn('generic');
         $record = new class ($pdo_mock) extends ActiveRecord {
             public function getDirty()
             {
@@ -28,6 +29,7 @@ public function getDirty()
     public function testExecutePdoError()
     {
         $pdo_mock = $this->createStub(PDO::class);
+        $pdo_mock->method('getAttribute')->willReturn('generic');
         $pdo_mock->method('prepare')->willReturn(false);
         $pdo_mock->method('errorInfo')->willReturn(['HY000', 1, 'test']);
         $record = new class ($pdo_mock) extends ActiveRecord {
@@ -42,6 +44,7 @@ public function testExecuteStatementError()
         $statement_mock = $this->createStub(PDOStatement::class);
         $pdo_mock = $this->createStub(PDO::class);
         $pdo_mock->method('prepare')->willReturn($statement_mock);
+        $pdo_mock->method('getAttribute')->willReturn('generic');
         $statement_mock->method('execute')->willReturn(false);
         $statement_mock->method('errorInfo')->willReturn(['HY000', 1, 'test_statement']);
         $record = new class ($pdo_mock) extends ActiveRecord {
@@ -54,6 +57,7 @@ public function testExecuteStatementError()
     public function testUnsetSqlExpressions()
     {
         $pdo_mock = $this->createStub(PDO::class);
+        $pdo_mock->method('getAttribute')->willReturn('generic');
         $record = new class ($pdo_mock) extends ActiveRecord {
         };
         $record->where = '1';
@@ -64,6 +68,7 @@ public function testUnsetSqlExpressions()
     public function testCustomData()
     {
         $pdo_mock = $this->createStub(PDO::class);
+        $pdo_mock->method('getAttribute')->willReturn('generic');
         $record = new class ($pdo_mock) extends ActiveRecord {
         };
         $record->setCustomData('test', 'something');
@@ -73,6 +78,7 @@ public function testCustomData()
     public function testCustomDataUnset()
     {
         $pdo_mock = $this->createStub(PDO::class);
+        $pdo_mock->method('getAttribute')->willReturn('generic');
         $record = new class ($pdo_mock) extends ActiveRecord {
         };
         $record->setCustomData('test', 'something');