From f49c2701962d0e614204906a45650c6fa8109ccb Mon Sep 17 00:00:00 2001 From: Tigrov Date: Sun, 24 Dec 2023 23:23:49 +0700 Subject: [PATCH 1/9] Refactor `update...()` methods --- psalm.xml | 3 ++ src/ActiveRecord.php | 2 +- src/ActiveRecordInterface.php | 29 ++++++++---- src/BaseActiveRecord.php | 86 ++++++++++++----------------------- tests/ActiveRecordTest.php | 11 +++++ 5 files changed, 65 insertions(+), 66 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 f636136a3..31ac97320 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); 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 d6cb497e3..11b739de2 100644 --- a/src/BaseActiveRecord.php +++ b/src/BaseActiveRecord.php @@ -233,7 +233,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(); @@ -244,16 +244,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; } @@ -270,9 +267,7 @@ public function getPrimaryKey(bool $asArray = false): mixed $values = []; - /** @psalm-var list $keys */ foreach ($keys as $name) { - /** @psalm-var string|null */ $values[$name] = $this->attributes[$name] ?? null; } @@ -655,7 +650,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 @@ -734,7 +731,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); } @@ -752,19 +749,11 @@ 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; + $this->setAttribute($name, $value); $attrs[] = $name; } } @@ -775,11 +764,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; @@ -843,6 +831,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 * @@ -852,18 +842,16 @@ 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) { + $this->attributes[$name] = (int)($this->attributes[$name] ?? 0) + $value; + $this->oldAttributes[$name] = $this->attributes[$name]; } - return false; + return true; } public function unlink(string $name, ActiveRecordInterface $arClass, bool $delete = false): void @@ -1173,41 +1161,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 = (int)$this->getAttribute($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->setAttribute($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..8a9e6f084 100644 --- a/tests/ActiveRecordTest.php +++ b/tests/ActiveRecordTest.php @@ -836,4 +836,15 @@ public function testToArrayForArrayable(): void ]), ); } + + public function testSaveWithoutChanges() + { + $this->checkFixture($this->db, 'customer'); + + $customerQuery = new ActiveQuery(Customer::class, $this->db); + + $customer = $customerQuery->findOne(1); + + $this->assertTrue($customer->save()); + } } From d4299e05c0b5cb49050e86ed09ef2d10d6677385 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Mon, 25 Dec 2023 00:12:38 +0700 Subject: [PATCH 2/9] Fix psalm issues --- src/ActiveRecord.php | 4 ---- src/BaseActiveRecord.php | 6 +----- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/src/ActiveRecord.php b/src/ActiveRecord.php index 31ac97320..a2e872c39 100644 --- a/src/ActiveRecord.php +++ b/src/ActiveRecord.php @@ -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/BaseActiveRecord.php b/src/BaseActiveRecord.php index 11b739de2..4b278dba0 100644 --- a/src/BaseActiveRecord.php +++ b/src/BaseActiveRecord.php @@ -68,10 +68,6 @@ public function delete(): false|int */ $condition = $this->getOldPrimaryKey(true); - if (is_array($condition) === false) { - return false; - } - $lock = $this->optimisticLock(); if ($lock !== null) { @@ -261,7 +257,7 @@ public function getPrimaryKey(bool $asArray = false): mixed { $keys = $this->primaryKey(); - if (!$asArray && count($keys) === 1) { + if ($asArray === false && count($keys) === 1) { return $this->attributes[$keys[0]] ?? null; } From 92e60ea1b7daad1ff3a30bfc34523b4a7c96a413 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Fri, 29 Dec 2023 07:42:41 +0700 Subject: [PATCH 3/9] Remove redundant `(int)` type casting --- src/BaseActiveRecord.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/BaseActiveRecord.php b/src/BaseActiveRecord.php index 4b278dba0..34157caf6 100644 --- a/src/BaseActiveRecord.php +++ b/src/BaseActiveRecord.php @@ -843,7 +843,7 @@ public function updateCounters(array $counters): bool } foreach ($counters as $name => $value) { - $this->attributes[$name] = (int)($this->attributes[$name] ?? 0) + $value; + $this->attributes[$name] = ($this->attributes[$name] ?? 0) + $value; $this->oldAttributes[$name] = $this->attributes[$name]; } @@ -1161,7 +1161,7 @@ protected function updateInternal(array $attributes = null): int $lock = $this->optimisticLock(); if ($lock !== null) { - $lockValue = (int)$this->getAttribute($lock); + $lockValue = $this->getAttribute($lock); $condition[$lock] = $lockValue; $values[$lock] = ++$lockValue; From 0ca354de1edfbfc842a8050bf938c1cf172a85e2 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Fri, 29 Dec 2023 00:43:10 +0000 Subject: [PATCH 4/9] Apply fixes from StyleCI --- tests/Stubs/ActiveRecord/Cat.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Stubs/ActiveRecord/Cat.php b/tests/Stubs/ActiveRecord/Cat.php index dc9a402d3..97ff4cf28 100644 --- a/tests/Stubs/ActiveRecord/Cat.php +++ b/tests/Stubs/ActiveRecord/Cat.php @@ -27,7 +27,7 @@ public function getException(): void */ public function getThrowable(): float|int { - return 5/0; + return 5 / 0; } public function setNonExistingProperty(string $value): void From 91d9c234ed8fcba2abff8b67d256149cb2dd2eb1 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Tue, 2 Jan 2024 16:16:46 +0700 Subject: [PATCH 5/9] Improve --- src/BaseActiveRecord.php | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/BaseActiveRecord.php b/src/BaseActiveRecord.php index 34157caf6..be7e95e62 100644 --- a/src/BaseActiveRecord.php +++ b/src/BaseActiveRecord.php @@ -258,13 +258,13 @@ public function getPrimaryKey(bool $asArray = false): mixed $keys = $this->primaryKey(); if ($asArray === false && count($keys) === 1) { - return $this->attributes[$keys[0]] ?? null; + return $this->{$keys[0]}; } $values = []; foreach ($keys as $name) { - $values[$name] = $this->attributes[$name] ?? null; + $values[$name] = $this->$name; } return $values; @@ -749,7 +749,7 @@ public function updateAttributes(array $attributes): int if (is_int($name)) { $attrs[] = $value; } else { - $this->setAttribute($name, $value); + $this->$name = $value; $attrs[] = $name; } } @@ -843,8 +843,9 @@ public function updateCounters(array $counters): bool } foreach ($counters as $name => $value) { - $this->attributes[$name] = ($this->attributes[$name] ?? 0) + $value; - $this->oldAttributes[$name] = $this->attributes[$name]; + $value += $this->$name ?? 0; + $this->$name = $value; + $this->oldAttributes[$name] = $value; } return true; @@ -1161,7 +1162,7 @@ protected function updateInternal(array $attributes = null): int $lock = $this->optimisticLock(); if ($lock !== null) { - $lockValue = $this->getAttribute($lock); + $lockValue = $this->$lock; $condition[$lock] = $lockValue; $values[$lock] = ++$lockValue; @@ -1172,7 +1173,7 @@ protected function updateInternal(array $attributes = null): int throw new StaleObjectException('The object being updated is outdated.'); } - $this->setAttribute($lock, $lockValue); + $this->$lock = $lockValue; } else { $rows = $this->updateAll($values, $condition); } From eeac663266a6f4fa3d75e7eb64f4cd1d9a4510a8 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Tue, 2 Jan 2024 17:02:20 +0700 Subject: [PATCH 6/9] Add test for `ActiveRecord::getPrimaryKey()` method --- tests/ActiveRecordTest.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/ActiveRecordTest.php b/tests/ActiveRecordTest.php index 8a9e6f084..c19156013 100644 --- a/tests/ActiveRecordTest.php +++ b/tests/ActiveRecordTest.php @@ -847,4 +847,16 @@ public function testSaveWithoutChanges() $this->assertTrue($customer->save()); } + + public function testGetPrimaryKey() + { + $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)); + } } From a0aa456374e04aba2fd2715f18023fbea65e7e43 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Tue, 2 Jan 2024 17:05:24 +0700 Subject: [PATCH 7/9] Add test for `ActiveRecord::getOldPrimaryKey()` method --- tests/ActiveRecordTest.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/ActiveRecordTest.php b/tests/ActiveRecordTest.php index c19156013..f1858d3f5 100644 --- a/tests/ActiveRecordTest.php +++ b/tests/ActiveRecordTest.php @@ -859,4 +859,16 @@ public function testGetPrimaryKey() $this->assertSame(1, $customer->getPrimaryKey()); $this->assertSame(['id' => 1], $customer->getPrimaryKey(true)); } + + public function testGetOldPrimaryKey() + { + $this->checkFixture($this->db, 'customer'); + + $customerQuery = new ActiveQuery(Customer::class, $this->db); + + $customer = $customerQuery->findOne(1); + + $this->assertSame(1, $customer->getOldPrimaryKey()); + $this->assertSame(['id' => 1], $customer->getOldPrimaryKey(true)); + } } From bc639357d965a804ac6c90ab37bc90593d313e38 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Tue, 2 Jan 2024 20:35:41 +0700 Subject: [PATCH 8/9] Add void type to tests [skip ci] --- tests/ActiveRecordTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/ActiveRecordTest.php b/tests/ActiveRecordTest.php index f1858d3f5..9258b4939 100644 --- a/tests/ActiveRecordTest.php +++ b/tests/ActiveRecordTest.php @@ -837,7 +837,7 @@ public function testToArrayForArrayable(): void ); } - public function testSaveWithoutChanges() + public function testSaveWithoutChanges(): void { $this->checkFixture($this->db, 'customer'); @@ -848,7 +848,7 @@ public function testSaveWithoutChanges() $this->assertTrue($customer->save()); } - public function testGetPrimaryKey() + public function testGetPrimaryKey(): void { $this->checkFixture($this->db, 'customer'); @@ -860,7 +860,7 @@ public function testGetPrimaryKey() $this->assertSame(['id' => 1], $customer->getPrimaryKey(true)); } - public function testGetOldPrimaryKey() + public function testGetOldPrimaryKey(): void { $this->checkFixture($this->db, 'customer'); From f3648dbccdde57c6ce799210b3417d4b153e5e82 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Fri, 12 Jan 2024 16:03:18 +0700 Subject: [PATCH 9/9] Change primary key before test `getOldPrimaryKey()` --- tests/ActiveRecordTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/ActiveRecordTest.php b/tests/ActiveRecordTest.php index 9258b4939..9ec06c79c 100644 --- a/tests/ActiveRecordTest.php +++ b/tests/ActiveRecordTest.php @@ -867,6 +867,7 @@ public function testGetOldPrimaryKey(): void $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));