From c5f92e5b121ae4705c5a95c051c6673407f8ab8a Mon Sep 17 00:00:00 2001 From: Sergei Tigrov Date: Sun, 14 Jan 2024 08:57:57 +0700 Subject: [PATCH] Refactor `update...()` methods (#282) --- psalm.xml | 3 ++ src/ActiveRecord.php | 6 +-- src/ActiveRecordInterface.php | 29 +++++++---- src/BaseActiveRecord.php | 95 ++++++++++++----------------------- tests/ActiveRecordTest.php | 36 +++++++++++++ 5 files changed, 93 insertions(+), 76 deletions(-) diff --git a/psalm.xml b/psalm.xml index e66ef57a1..19d34d11d 100644 --- a/psalm.xml +++ b/psalm.xml @@ -14,4 +14,7 @@ + + + diff --git a/src/ActiveRecord.php b/src/ActiveRecord.php index 7b408a43d..90318b562 100644 --- a/src/ActiveRecord.php +++ b/src/ActiveRecord.php @@ -323,7 +323,7 @@ public function transactions(): array return []; } - public function update(array $attributeNames = null): false|int + public function update(array $attributeNames = null): int { if (!$this->isTransactional(self::OP_UPDATE)) { return $this->updateInternal($attributeNames); @@ -366,10 +366,6 @@ protected function deleteInternal(): false|int $condition = $this->getOldPrimaryKey(true); $lock = $this->optimisticLock(); - if (is_array($condition) === false) { - return false; - } - if ($lock !== null) { $condition[$lock] = $this->$lock; } diff --git a/src/ActiveRecordInterface.php b/src/ActiveRecordInterface.php index f8a9952b8..6af7b3cd7 100644 --- a/src/ActiveRecordInterface.php +++ b/src/ActiveRecordInterface.php @@ -159,6 +159,12 @@ public function getIsNewRecord(): bool; * @return mixed The old primary key value. An array (column name => column value) is returned if the primary key * is composite or `$asArray` is true. A string is returned otherwise (`null` will be returned if the key value is * `null`). + * + * @psalm-return ( + * $asArray is true + * ? array + * : mixed|null + * ) */ public function getOldPrimaryKey(bool $asArray = false): mixed; @@ -172,6 +178,12 @@ public function getOldPrimaryKey(bool $asArray = false): mixed; * @return mixed The primary key value. An array (attribute name => attribute value) is returned if the primary key * is composite or `$asArray` is true. A string is returned otherwise (`null` will be returned if the key value is * `null`). + * + * @psalm-return ( + * $asArray is true + * ? array + * : mixed|null + * ) */ public function getPrimaryKey(bool $asArray = false): mixed; @@ -322,7 +334,7 @@ public function populateRelation(string $name, array|self|null $records): void; * @throws Exception * @throws InvalidConfigException * - * @psalm-return string[] The primary keys of the associated database table. + * @return string[] The primary keys of the associated database table. */ public function primaryKey(): array; @@ -377,7 +389,7 @@ public function setAttribute(string $name, mixed $value): void; * For this reason, you should use the following code to check if update() is successful or not: * * ```php - * if ($customer->update() !== false) { + * if ($customer->update() !== 0) { * // update successful * } else { * // update failed @@ -391,10 +403,9 @@ public function setAttribute(string $name, mixed $value): void; * outdated. * @throws Throwable In case update failed. * - * @return false|int The number of rows affected, or false if validation fails or {@seebeforeSave()} stops the - * updating process. + * @return int The number of rows affected. */ - public function update(array $attributeNames = null): false|int; + public function update(array $attributeNames = null): int; /** * Updates the whole table using the provided attribute values and conditions. @@ -410,10 +421,10 @@ public function update(array $attributeNames = null): false|int; * * ```php * $customerQuery = new ActiveQuery(Customer::class, $db); - * $aqClasses = $customerQuery->where('status = 2')->all(); - * foreach ($aqClasses as $aqClass) { - * $aqClass->status = 1; - * $aqClass->update(); + * $customers = $customerQuery->where('status = 2')->all(); + * foreach ($customers as $customer) { + * $customer->status = 1; + * $customer->update(); * } * ``` * diff --git a/src/BaseActiveRecord.php b/src/BaseActiveRecord.php index 9e34d15f5..e40125933 100644 --- a/src/BaseActiveRecord.php +++ b/src/BaseActiveRecord.php @@ -69,10 +69,6 @@ public function delete(): false|int */ $condition = $this->getOldPrimaryKey(true); - if (is_array($condition) === false) { - return false; - } - $lock = $this->optimisticLock(); if ($lock !== null) { @@ -231,7 +227,7 @@ public function getOldAttributes(): array * @throws InvalidConfigException * @throws Exception */ - public function getOldPrimaryKey(bool $asArray = false): array|string|null + public function getOldPrimaryKey(bool $asArray = false): mixed { $keys = $this->primaryKey(); @@ -242,16 +238,13 @@ public function getOldPrimaryKey(bool $asArray = false): array|string|null ); } - if (!$asArray && count($keys) === 1) { - /** @psalm-var string|null */ + if ($asArray === false && count($keys) === 1) { return $this->oldAttributes[$keys[0]] ?? null; } $values = []; - /** @psalm-var list $keys */ foreach ($keys as $name) { - /** @psalm-var string|null */ $values[$name] = $this->oldAttributes[$name] ?? null; } @@ -262,16 +255,14 @@ public function getPrimaryKey(bool $asArray = false): mixed { $keys = $this->primaryKey(); - if (!$asArray && count($keys) === 1) { - return $this->attributes[$keys[0]] ?? null; + if ($asArray === false && count($keys) === 1) { + return $this->{$keys[0]}; } $values = []; - /** @psalm-var list $keys */ foreach ($keys as $name) { - /** @psalm-var string|null */ - $values[$name] = $this->attributes[$name] ?? null; + $values[$name] = $this->$name; } return $values; @@ -660,7 +651,9 @@ public function save(array $attributeNames = null): bool return $this->insert($attributeNames); } - return $this->update($attributeNames) !== false; + $this->update($attributeNames); + + return true; } public function setAttribute(string $name, mixed $value): void @@ -739,7 +732,7 @@ public function setOldAttributes(array $values = null): void $this->oldAttributes = $values; } - public function update(array $attributeNames = null): false|int + public function update(array $attributeNames = null): int { return $this->updateInternal($attributeNames); } @@ -757,16 +750,8 @@ public function updateAttributes(array $attributes): int { $attrs = []; - $condition = $this->getOldPrimaryKey(true); - - if ($condition === null || $condition === []) { - return 0; - } - - /** @psalm-var mixed $value */ foreach ($attributes as $name => $value) { if (is_int($name)) { - /** @psalm-var mixed */ $attrs[] = $value; } else { $this->$name = $value; @@ -780,11 +765,10 @@ public function updateAttributes(array $attributes): int return 0; } - $rows = $this->updateAll($values, $this->getOldPrimaryKey(true) ?? []); + $rows = $this->updateAll($values, $this->getOldPrimaryKey(true)); - /** @psalm-var array $value */ foreach ($values as $name => $value) { - $this->oldAttributes[$name] = $this->attributes[$name]; + $this->oldAttributes[$name] = $value; } return $rows; @@ -848,6 +832,8 @@ public function updateAllCounters(array $counters, array|string $condition = '', * @param array $counters The counters to be updated (attribute name => increment value), use negative values if you * want to decrement the counters. * + * @psalm-param array $counters + * * @throws Exception * @throws NotSupportedException * @@ -857,18 +843,17 @@ public function updateAllCounters(array $counters, array|string $condition = '', */ public function updateCounters(array $counters): bool { - if ($this->updateAllCounters($counters, $this->getOldPrimaryKey(true) ?? '')) { - /** @psalm-var array $counters */ - foreach ($counters as $name => $value) { - $this->attributes[$name] = isset($this->attributes[$name]) - ? (int) $this->attributes[$name] + $value : $value; - $this->oldAttributes[$name] = $this->attributes[$name]; - } + if ($this->updateAllCounters($counters, $this->getOldPrimaryKey(true)) === 0) { + return false; + } - return true; + foreach ($counters as $name => $value) { + $value += $this->$name ?? 0; + $this->$name = $value; + $this->oldAttributes[$name] = $value; } - return false; + return true; } public function unlink(string $name, ActiveRecordInterface $arClass, bool $delete = false): void @@ -1180,41 +1165,27 @@ protected function updateInternal(array $attributes = null): int return 0; } - /** @psalm-var mixed $condition */ $condition = $this->getOldPrimaryKey(true); $lock = $this->optimisticLock(); - if ($lock !== null && is_array($condition)) { - $values[$lock] = (int) $this->$lock + 1; - /** @psalm-var array|string */ - $condition[$lock] = $this->$lock; - } + if ($lock !== null) { + $lockValue = $this->$lock; - /** - * We do not check the return value of updateAll() because it's possible that the UPDATE statement doesn't - * change anything and thus returns 0. - * - * @psalm-var array|string $condition - */ - $rows = $this->updateAll($values, $condition); + $condition[$lock] = $lockValue; + $values[$lock] = ++$lockValue; - if ($lock !== null && !$rows) { - throw new StaleObjectException('The object being updated is outdated.'); - } + $rows = $this->updateAll($values, $condition); - if (isset($values[$lock])) { - $this->$lock = $values[$lock]; - } + if ($rows === 0) { + throw new StaleObjectException('The object being updated is outdated.'); + } - $changedAttributes = []; + $this->$lock = $lockValue; + } else { + $rows = $this->updateAll($values, $condition); + } - /** - * @psalm-var string $name - * @psalm-var mixed $value - */ foreach ($values as $name => $value) { - /** @psalm-var mixed */ - $changedAttributes[$name] = $this->oldAttributes[$name] ?? null; $this->oldAttributes[$name] = $value; } diff --git a/tests/ActiveRecordTest.php b/tests/ActiveRecordTest.php index 903ee53ce..9ec06c79c 100644 --- a/tests/ActiveRecordTest.php +++ b/tests/ActiveRecordTest.php @@ -836,4 +836,40 @@ public function testToArrayForArrayable(): void ]), ); } + + public function testSaveWithoutChanges(): void + { + $this->checkFixture($this->db, 'customer'); + + $customerQuery = new ActiveQuery(Customer::class, $this->db); + + $customer = $customerQuery->findOne(1); + + $this->assertTrue($customer->save()); + } + + public function testGetPrimaryKey(): void + { + $this->checkFixture($this->db, 'customer'); + + $customerQuery = new ActiveQuery(Customer::class, $this->db); + + $customer = $customerQuery->findOne(1); + + $this->assertSame(1, $customer->getPrimaryKey()); + $this->assertSame(['id' => 1], $customer->getPrimaryKey(true)); + } + + public function testGetOldPrimaryKey(): void + { + $this->checkFixture($this->db, 'customer'); + + $customerQuery = new ActiveQuery(Customer::class, $this->db); + + $customer = $customerQuery->findOne(1); + $customer->id = 2; + + $this->assertSame(1, $customer->getOldPrimaryKey()); + $this->assertSame(['id' => 1], $customer->getOldPrimaryKey(true)); + } }