From 155a53d6e899c0bdad250d67baec004266e3d6c7 Mon Sep 17 00:00:00 2001 From: Sergei Tigrov Date: Wed, 22 May 2024 14:20:59 +0700 Subject: [PATCH] Protect private properties (#338) --- src/AbstractActiveRecord.php | 116 ++++++++++++++++--------- src/ActiveRecordInterface.php | 4 +- src/BaseActiveRecord.php | 40 ++++----- src/Trait/MagicPropertiesTrait.php | 3 +- src/Trait/TransactionalTrait.php | 6 +- tests/ActiveRecordFactoryTest.php | 4 +- tests/Stubs/ActiveRecord/Animal.php | 2 +- tests/Stubs/ActiveRecord/OrderItem.php | 2 +- 8 files changed, 103 insertions(+), 74 deletions(-) diff --git a/src/AbstractActiveRecord.php b/src/AbstractActiveRecord.php index eb3db8185..9be5bc6eb 100644 --- a/src/AbstractActiveRecord.php +++ b/src/AbstractActiveRecord.php @@ -29,7 +29,6 @@ use function array_search; use function array_values; use function count; -use function get_object_vars; use function in_array; use function is_array; use function is_int; @@ -48,12 +47,47 @@ abstract class AbstractActiveRecord implements ActiveRecordInterface private array $relationsDependencies = []; public function __construct( - protected ConnectionInterface $db, + private ConnectionInterface $db, private ActiveRecordFactory|null $arFactory = null, private string $tableName = '' ) { } + /** + * Returns the public and protected property values of an Active Record object. + * + * This method is provided because a direct call of {@see get_object_vars()} within the {@see AbstractActiveRecord} + * class will return also private property values of {@see AbstractActiveRecord} class. + * + * @param ActiveRecordInterface $object + * + * @return array + * @link https://www.php.net/manual/en/function.get-object-vars.php + * + * @psalm-return array + */ + abstract protected function getObjectVars(ActiveRecordInterface $object): array; + + /** + * Inserts Active Record values into DB without considering transaction. + * + * @param array|null $attributes List of attributes that need to be saved. Defaults to `null`, meaning all + * attributes that are loaded from DB will be saved. + * + * @throws Exception + * @throws InvalidArgumentException + * @throws InvalidConfigException + * @throws Throwable + * + * @return bool Whether the record is inserted successfully. + */ + abstract protected function insertInternal(array $attributes = null): bool; + + /** + * Sets the value of the named attribute. + */ + abstract protected function populateAttribute(string $name, mixed $value): void; + public function delete(): int { return $this->deleteInternal(); @@ -78,19 +112,18 @@ public function equals(ActiveRecordInterface $record): bool public function getAttribute(string $name): mixed { - return get_object_vars($this)[$name] ?? null; + return $this->getObjectVars($this)[$name] ?? null; } public function getAttributes(array $names = null, array $except = []): array { $names ??= $this->attributes(); - $attributes = get_object_vars($this); if ($except !== []) { $names = array_diff($names, $except); } - return array_intersect_key($attributes, array_flip($names)); + return array_intersect_key($this->getObjectVars($this), array_flip($names)); } public function getIsNewRecord(): bool @@ -293,8 +326,6 @@ public function insert(array $attributes = null): bool return $this->insertInternal($attributes); } - abstract protected function insertInternal(array $attributes = null): bool; - /** * @psalm-param class-string $arClass */ @@ -314,11 +345,11 @@ public function instantiateQuery(string $arClass): ActiveQueryInterface */ public function isAttributeChanged(string $name, bool $identical = true): bool { - if (isset($this->oldAttributes[$name])) { - return $this->$name !== $this->oldAttributes[$name]; + if (!isset($this->oldAttributes[$name])) { + return array_key_exists($name, $this->getObjectVars($this)); } - return false; + return $this->getAttribute($name) !== $this->oldAttributes[$name]; } public function isPrimaryKey(array $keys): bool @@ -372,7 +403,7 @@ public function link(string $name, ActiveRecordInterface $arClass, array $extraC */ foreach ($viaLink as $a => $b) { /** @psalm-var mixed */ - $columns[$a] = $this->$b; + $columns[$a] = $this->getAttribute($b); } $link = $relation->getLink(); @@ -383,7 +414,7 @@ public function link(string $name, ActiveRecordInterface $arClass, array $extraC */ foreach ($link as $a => $b) { /** @psalm-var mixed */ - $columns[$b] = $arClass->$a; + $columns[$b] = $arClass->getAttribute($a); } /** @@ -401,7 +432,7 @@ public function link(string $name, ActiveRecordInterface $arClass, array $extraC * @psalm-var mixed $value */ foreach ($columns as $column => $value) { - $viaClass->$column = $value; + $viaClass->setAttribute($column, $value); } $viaClass->insert(); @@ -441,9 +472,9 @@ public function link(string $name, ActiveRecordInterface $arClass, array $extraC $indexBy = $relation->getIndexBy(); if ($indexBy !== null) { if ($indexBy instanceof Closure) { - $index = $relation->$indexBy($arClass::class); + $index = $indexBy($arClass->getAttributes()); } else { - $index = $arClass->$indexBy; + $index = $arClass->getAttribute($indexBy); } if ($index !== null) { @@ -609,12 +640,12 @@ public function setAttribute(string $name, mixed $value): void { if ( isset($this->relationsDependencies[$name]) - && (!isset(get_object_vars($this)[$name]) || $this->$name !== $value) + && ($value === null || $this->getAttribute($name) !== $value) ) { $this->resetDependentRelations($name); } - $this->$name = $value; + $this->populateAttribute($name, $value); } /** @@ -630,7 +661,7 @@ public function setAttributes(array $values): void /** @psalm-var mixed $value */ foreach ($values as $name => $value) { - $this->$name = $value; + $this->populateAttribute($name, $value); } } @@ -643,7 +674,7 @@ public function setAttributes(array $values): void */ public function setIsNewRecord(bool $value): void { - $this->oldAttributes = $value ? null : get_object_vars($this); + $this->oldAttributes = $value ? null : $this->getObjectVars($this); } /** @@ -791,8 +822,8 @@ public function updateCounters(array $counters): bool } foreach ($counters as $name => $value) { - $value += $this->$name ?? 0; - $this->$name = $value; + $value += $this->getAttribute($name) ?? 0; + $this->populateAttribute($name, $value); $this->oldAttributes[$name] = $value; } @@ -825,14 +856,14 @@ public function unlink(string $name, ActiveRecordInterface $arClass, bool $delet foreach ($viaRelation->getLink() as $a => $b) { /** @psalm-var mixed */ - $columns[$a] = $this->$b; + $columns[$a] = $this->getAttribute($b); } $link = $relation->getLink(); foreach ($link as $a => $b) { /** @psalm-var mixed */ - $columns[$b] = $arClass->$a; + $columns[$b] = $arClass->getAttribute($a); } $nulls = array_fill_keys(array_keys($columns), null); @@ -862,22 +893,22 @@ public function unlink(string $name, ActiveRecordInterface $arClass, bool $delet $arClass->delete(); } else { foreach ($relation->getLink() as $a => $b) { - $arClass->$a = null; + $arClass->setAttribute($a, null); } $arClass->save(); } } elseif ($arClass->isPrimaryKey(array_keys($relation->getLink()))) { foreach ($relation->getLink() as $a => $b) { /** @psalm-var mixed $values */ - $values = $this->$b; + $values = $this->getAttribute($b); /** relation via array valued attribute */ if (is_array($values)) { - if (($key = array_search($arClass->$a, $values, false)) !== false) { + if (($key = array_search($arClass->getAttribute($a), $values, false)) !== false) { unset($values[$key]); - $this->$b = array_values($values); + $this->setAttribute($b, array_values($values)); } } else { - $this->$b = null; + $this->setAttribute($b, null); } } $delete ? $this->delete() : $this->save(); @@ -943,7 +974,7 @@ public function unlinkAll(string $name, bool $delete = false): void foreach ($viaRelation->getLink() as $a => $b) { $nulls[$a] = null; /** @psalm-var mixed */ - $condition[$a] = $this->$b; + $condition[$a] = $this->getAttribute($b); } if (!empty($viaRelation->getWhere())) { @@ -973,9 +1004,9 @@ public function unlinkAll(string $name, bool $delete = false): void $relatedModel = $relation->getARInstance(); $link = $relation->getLink(); - if (!$delete && count($link) === 1 && is_array($this->{$b = reset($link)})) { + if (!$delete && count($link) === 1 && is_array($this->getAttribute($b = reset($link)))) { /** relation via array valued attribute */ - $this->$b = []; + $this->setAttribute($b, []); $this->save(); } else { $nulls = []; @@ -984,7 +1015,7 @@ public function unlinkAll(string $name, bool $delete = false): void foreach ($relation->getLink() as $a => $b) { $nulls[$a] = null; /** @psalm-var mixed */ - $condition[$a] = $this->$b; + $condition[$a] = $this->getAttribute($b); } if (!empty($relation->getWhere())) { @@ -1077,7 +1108,7 @@ protected function deleteInternal(): int $lock = $this->optimisticLock(); if ($lock !== null) { - $condition[$lock] = $this->$lock; + $condition[$lock] = $this->getAttribute($lock); $result = $this->deleteAll($condition); @@ -1109,7 +1140,7 @@ protected function refreshInternal(array|ActiveRecordInterface $record = null): } foreach ($this->attributes() as $name) { - $this->$name = $record->getAttribute($name); + $this->populateAttribute($name, $record->getAttribute($name)); } $this->oldAttributes = $record->getOldAttributes(); @@ -1142,7 +1173,7 @@ protected function updateInternal(array $attributes = null): int $lock = $this->optimisticLock(); if ($lock !== null) { - $lockValue = $this->$lock; + $lockValue = $this->getAttribute($lock); $condition[$lock] = $lockValue; $values[$lock] = ++$lockValue; @@ -1153,7 +1184,7 @@ protected function updateInternal(array $attributes = null): int throw new StaleObjectException('The object being updated is outdated.'); } - $this->$lock = $lockValue; + $this->populateAttribute($lock, $lockValue); } else { $rows = $this->updateAll($values, $condition); } @@ -1171,7 +1202,7 @@ private function bindModels( /** @psalm-var string[] $link */ foreach ($link as $fk => $pk) { /** @psalm-var mixed $value */ - $value = $primaryModel->$pk; + $value = $primaryModel->getAttribute($pk); if ($value === null) { throw new InvalidCallException( @@ -1181,12 +1212,11 @@ private function bindModels( /** * relation via array valued attribute - * - * @psalm-suppress MixedArrayAssignment */ - if (is_array($foreignModel->getAttribute($fk))) { + if (is_array($fkValue = $foreignModel->getAttribute($fk))) { /** @psalm-var mixed */ - $foreignModel->{$fk}[] = $value; + $fkValue[] = $value; + $foreignModel->setAttribute($fk, $fkValue); } else { $foreignModel->setAttribute($fk, $value); } @@ -1223,8 +1253,8 @@ public function getTableName(): string return $this->tableName; } - protected function populateAttribute(string $name, mixed $value): void + protected function db(): ConnectionInterface { - $this->$name = $value; + return $this->db; } } diff --git a/src/ActiveRecordInterface.php b/src/ActiveRecordInterface.php index 4dfafaddd..cdcc7d25e 100644 --- a/src/ActiveRecordInterface.php +++ b/src/ActiveRecordInterface.php @@ -43,14 +43,14 @@ public function delete(): int; * For example, to delete all customers whose status is 3: * * ```php - * $customer = new Customer($this->db); + * $customer = new Customer($db); * $customer->deleteAll('status = 3'); * ``` * * > Warning: If you don't specify any condition, this method will delete **all** rows in the table. * * ```php - * $customerQuery = new ActiveQuery(Customer::class, $this->db); + * $customerQuery = new ActiveQuery(Customer::class, $db); * $aqClasses = $customerQuery->where('status = 3')->all(); * foreach ($aqClasses as $aqClass) { * $aqClass->delete(); diff --git a/src/BaseActiveRecord.php b/src/BaseActiveRecord.php index cf5a64260..02c69ecd0 100644 --- a/src/BaseActiveRecord.php +++ b/src/BaseActiveRecord.php @@ -14,6 +14,7 @@ use function array_keys; use function array_map; use function array_values; +use function get_object_vars; use function in_array; use function is_array; use function is_string; @@ -90,7 +91,7 @@ public function filterCondition(array $condition, array $aliases = []): array $columnNames = $this->filterValidColumnNames($aliases); foreach ($condition as $key => $value) { - if (is_string($key) && !in_array($this->db->getQuoter()->quoteSql($key), $columnNames, true)) { + if (is_string($key) && !in_array($this->db()->getQuoter()->quoteSql($key), $columnNames, true)) { throw new InvalidArgumentException( 'Key "' . $key . '" is not a column name and can not be used as a filter' ); @@ -120,7 +121,7 @@ public function filterValidAliases(ActiveQuery $query): array */ public function getTableSchema(): TableSchemaInterface { - $tableSchema = $this->db->getSchema()->getTableSchema($this->getTableName()); + $tableSchema = $this->db()->getSchema()->getTableSchema($this->getTableName()); if ($tableSchema === null) { throw new InvalidConfigException('The table does not exist: ' . $this->getTableName()); @@ -211,41 +212,33 @@ protected function filterValidColumnNames(array $aliases): array { $columnNames = []; $tableName = $this->getTableName(); - $quotedTableName = $this->db->getQuoter()->quoteTableName($tableName); + $quotedTableName = $this->db()->getQuoter()->quoteTableName($tableName); foreach ($this->getTableSchema()->getColumnNames() as $columnName) { $columnNames[] = $columnName; - $columnNames[] = $this->db->getQuoter()->quoteColumnName($columnName); + $columnNames[] = $this->db()->getQuoter()->quoteColumnName($columnName); $columnNames[] = "$tableName.$columnName"; - $columnNames[] = $this->db->getQuoter()->quoteSql("$quotedTableName.[[$columnName]]"); + $columnNames[] = $this->db()->getQuoter()->quoteSql("$quotedTableName.[[$columnName]]"); foreach ($aliases as $tableAlias) { $columnNames[] = "$tableAlias.$columnName"; - $quotedTableAlias = $this->db->getQuoter()->quoteTableName($tableAlias); - $columnNames[] = $this->db->getQuoter()->quoteSql("$quotedTableAlias.[[$columnName]]"); + $quotedTableAlias = $this->db()->getQuoter()->quoteTableName($tableAlias); + $columnNames[] = $this->db()->getQuoter()->quoteSql("$quotedTableAlias.[[$columnName]]"); } } return $columnNames; } - /** - * Inserts an ActiveRecord into DB without considering transaction. - * - * @param array|null $attributes List of attributes that need to be saved. Defaults to `null`, meaning all - * attributes that are loaded from DB will be saved. - * - * @throws Exception - * @throws InvalidArgumentException - * @throws InvalidConfigException - * @throws Throwable - * - * @return bool Whether the record is inserted successfully. - */ + protected function getObjectVars(ActiveRecordInterface $object): array + { + return get_object_vars($object); + } + protected function insertInternal(array $attributes = null): bool { $values = $this->getDirtyAttributes($attributes); - $primaryKeys = $this->db->createCommand()->insertWithReturningPks($this->getTableName(), $values); + $primaryKeys = $this->db()->createCommand()->insertWithReturningPks($this->getTableName(), $values); if ($primaryKeys === false) { return false; @@ -263,4 +256,9 @@ protected function insertInternal(array $attributes = null): bool return true; } + + protected function populateAttribute(string $name, mixed $value): void + { + $this->$name = $value; + } } diff --git a/src/Trait/MagicPropertiesTrait.php b/src/Trait/MagicPropertiesTrait.php index c6cd58bda..89708eb1f 100644 --- a/src/Trait/MagicPropertiesTrait.php +++ b/src/Trait/MagicPropertiesTrait.php @@ -207,8 +207,7 @@ public function isAttributeChanged(string $name, bool $identical = true): bool } if (property_exists($this, $name)) { - return !array_key_exists($name, get_object_vars($this)) - || $this->getOldAttribute($name) !== $this->$name; + return $this->getOldAttribute($name) !== $this->$name; } return !array_key_exists($name, $this->attributes) diff --git a/src/Trait/TransactionalTrait.php b/src/Trait/TransactionalTrait.php index d10720e6e..bec8bf17d 100644 --- a/src/Trait/TransactionalTrait.php +++ b/src/Trait/TransactionalTrait.php @@ -25,7 +25,7 @@ public function delete(): int return $this->deleteInternal(); } - $transaction = $this->db->beginTransaction(); + $transaction = $this->db()->beginTransaction(); try { $result = $this->deleteInternal(); @@ -44,7 +44,7 @@ public function insert(array $attributes = null): bool return $this->insertInternal($attributes); } - $transaction = $this->db->beginTransaction(); + $transaction = $this->db()->beginTransaction(); try { $result = $this->insertInternal($attributes); @@ -81,7 +81,7 @@ public function update(array $attributeNames = null): int return $this->updateInternal($attributeNames); } - $transaction = $this->db->beginTransaction(); + $transaction = $this->db()->beginTransaction(); try { $result = $this->updateInternal($attributeNames); diff --git a/tests/ActiveRecordFactoryTest.php b/tests/ActiveRecordFactoryTest.php index 270634d09..ac4dd0abc 100644 --- a/tests/ActiveRecordFactoryTest.php +++ b/tests/ActiveRecordFactoryTest.php @@ -24,8 +24,9 @@ public function testCreateAR(): void public function testCreateARWithConnection(): void { $customerAR = $this->arFactory->createAR(arClass: Customer::class, db: $this->db); - $db = Assert::inaccessibleProperty($customerAR, 'db'); + $db = Assert::invokeMethod($customerAR, 'db'); + $this->assertSame($this->db, $db); $this->assertSame('customer', $customerAR->getTableName()); $this->assertInstanceOf(Customer::class, $customerAR); } @@ -62,6 +63,7 @@ public function testCreateQueryToWithConnection(): void ); $db = Assert::inaccessibleProperty($customerQuery, 'db'); + $this->assertSame($this->db, $db); $this->assertInstanceOf(ActiveQuery::class, $customerQuery); } diff --git a/tests/Stubs/ActiveRecord/Animal.php b/tests/Stubs/ActiveRecord/Animal.php index ca1685d43..35a68679e 100644 --- a/tests/Stubs/ActiveRecord/Animal.php +++ b/tests/Stubs/ActiveRecord/Animal.php @@ -39,7 +39,7 @@ public function instantiate($row): ActiveRecordInterface { $class = $row['type']; - return new $class($this->db); + return new $class($this->db()); } public function setDoes(string $value): void diff --git a/tests/Stubs/ActiveRecord/OrderItem.php b/tests/Stubs/ActiveRecord/OrderItem.php index ba67abdef..8af8b7cca 100644 --- a/tests/Stubs/ActiveRecord/OrderItem.php +++ b/tests/Stubs/ActiveRecord/OrderItem.php @@ -58,6 +58,6 @@ public function getOrderItemCompositeNoJoinQuery(): ActiveQuery public function getCustomQuery(): ActiveQuery { - return new ActiveQuery(Order::class, $this->db); + return new ActiveQuery(Order::class, $this->db()); } }