From 3b2de6ccb174ca56e5f821694237f96a27841e1a Mon Sep 17 00:00:00 2001 From: Tigrov Date: Mon, 25 Dec 2023 00:09:18 +0700 Subject: [PATCH 1/4] Refactor `delete...()` methods --- src/ActiveRecord.php | 47 ++------------------------ src/ActiveRecordInterface.php | 4 +-- src/BaseActiveRecord.php | 63 ++++++++++++++++++++--------------- tests/ActiveQueryTest.php | 30 +++++++++++++++++ 4 files changed, 71 insertions(+), 73 deletions(-) diff --git a/src/ActiveRecord.php b/src/ActiveRecord.php index f636136a3..c53c7e6db 100644 --- a/src/ActiveRecord.php +++ b/src/ActiveRecord.php @@ -114,7 +114,7 @@ public function attributes(): array return $this->getTableSchema()->getColumnNames(); } - public function delete(): false|int + public function delete(): int { if (!$this->isTransactional(self::OP_DELETE)) { return $this->deleteInternal(); @@ -124,11 +124,7 @@ public function delete(): false|int try { $result = $this->deleteInternal(); - if ($result === false) { - $transaction->rollBack(); - } else { - $transaction->commit(); - } + $transaction->commit(); return $result; } catch (Throwable $e) { @@ -346,45 +342,6 @@ public function update(array $attributeNames = null): false|int } } - /** - * Deletes an ActiveRecord without considering transaction. - * - * Note that it is possible the number of rows deleted is 0, even though the deletion execution is successful. - * - * @throws Exception - * @throws StaleObjectException - * @throws Throwable - * - * @return false|int The number of rows deleted, or `false` if the deletion is unsuccessful for some reason. - */ - protected function deleteInternal(): false|int - { - /** - * We do not check the return value of deleteAll() because it's possible the record is already deleted in the - * database and thus the method will return 0. - */ - $condition = $this->getOldPrimaryKey(true); - $lock = $this->optimisticLock(); - - if (is_array($condition) === false) { - return false; - } - - if ($lock !== null) { - $condition[$lock] = $this->$lock; - } - - $result = $this->deleteAll($condition); - - if ($lock !== null && !$result) { - throw new StaleObjectException('The object being deleted is outdated.'); - } - - $this->setOldAttributes(); - - return $result; - } - /** * Valid column names are table column names or column names prefixed with table name or table alias. * diff --git a/src/ActiveRecordInterface.php b/src/ActiveRecordInterface.php index f8a9952b8..c18d37918 100644 --- a/src/ActiveRecordInterface.php +++ b/src/ActiveRecordInterface.php @@ -34,11 +34,11 @@ public function attributes(): array; * is outdated. * @throws Throwable In case delete failed. * - * @return false|int The number of rows deleted, or `false` if the deletion is unsuccessful for some reason. + * @return int The number of rows deleted. * * Note that it's possible the number of rows deleted is 0, even though the deletion execution is successful. */ - public function delete(): false|int; + public function delete(): int; /** * Deletes rows in the table using the provided conditions. diff --git a/src/BaseActiveRecord.php b/src/BaseActiveRecord.php index d6cb497e3..da4a47541 100644 --- a/src/BaseActiveRecord.php +++ b/src/BaseActiveRecord.php @@ -60,33 +60,9 @@ public function __construct( ) { } - public function delete(): false|int + public function delete(): int { - /** - * We do not check the return value of deleteAll() because it's possible the record is already deleted in - * the database and thus the method will return 0 - */ - $condition = $this->getOldPrimaryKey(true); - - if (is_array($condition) === false) { - return false; - } - - $lock = $this->optimisticLock(); - - if ($lock !== null) { - $condition[$lock] = $lock; - } - - $result = $this->deleteAll($condition); - - if ($lock !== null && !$result) { - throw new StaleObjectException('The object being deleted is outdated.'); - } - - $this->oldAttributes = null; - - return $result; + return $this->deleteInternal(); } public function deleteAll(array $condition = [], array $params = []): int @@ -1128,6 +1104,41 @@ protected function createRelationQuery(string $arClass, array $link, bool $multi return $this->instantiateQuery($arClass)->primaryModel($this)->link($link)->multiple($multiple); } + /** + * {@see delete()} + * + * @throws Exception + * @throws StaleObjectException + * @throws Throwable + * + * @return int The number of rows deleted. + */ + protected function deleteInternal(): int + { + /** + * We do not check the return value of deleteAll() because it's possible the record is already deleted in + * the database and thus the method will return 0 + */ + $condition = $this->getOldPrimaryKey(true); + $lock = $this->optimisticLock(); + + if ($lock !== null) { + $condition[$lock] = $this->getAttribute($lock); + + $result = $this->deleteAll($condition); + + if ($result === 0) { + throw new StaleObjectException('The object being deleted is outdated.'); + } + } else { + $result = $this->deleteAll($condition); + } + + $this->setOldAttributes(); + + return $result; + } + /** * Repopulates this active record with the latest data from a newly fetched instance. * diff --git a/tests/ActiveQueryTest.php b/tests/ActiveQueryTest.php index 4fa25063b..03b9cf95f 100644 --- a/tests/ActiveQueryTest.php +++ b/tests/ActiveQueryTest.php @@ -2005,6 +2005,36 @@ public function testOptimisticLock(): void $record->save(); } + public function testOptimisticLockOnDelete(): void + { + $this->checkFixture($this->db, 'document'); + + $documentQuery = new ActiveQuery(Document::class, $this->db); + $document = $documentQuery->findOne(1); + + $this->assertSame(0, $document->version); + + $document->version = 1; + + $this->expectException(StaleObjectException::class); + $document->delete(); + } + + public function testOptimisticLockAfterDelete(): void + { + $this->checkFixture($this->db, 'document'); + + $documentQuery = new ActiveQuery(Document::class, $this->db); + $document = $documentQuery->findOne(1); + + $this->assertSame(0, $document->version); + $this->assertSame(1, $document->delete()); + $this->assertTrue($document->getIsNewRecord()); + + $this->expectException(StaleObjectException::class); + $document->delete(); + } + /** * {@see https://github.com/yiisoft/yii2/issues/9006} */ From 8fa133f2751197d5feaf42efb6f8a074158d6630 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Fri, 29 Dec 2023 00:48:58 +0000 Subject: [PATCH 2/4] Apply fixes from StyleCI --- src/ActiveRecord.php | 1 - tests/Stubs/ActiveRecord/Cat.php | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ActiveRecord.php b/src/ActiveRecord.php index c53c7e6db..808876b96 100644 --- a/src/ActiveRecord.php +++ b/src/ActiveRecord.php @@ -8,7 +8,6 @@ use Yiisoft\Db\Exception\Exception; use Yiisoft\Db\Exception\InvalidArgumentException; use Yiisoft\Db\Exception\InvalidConfigException; -use Yiisoft\Db\Exception\StaleObjectException; use Yiisoft\Db\Schema\TableSchemaInterface; use function array_diff; 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 18dd04a4ba4299d1f35cbac1cc40afd0053e4216 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Fri, 29 Dec 2023 08:08:21 +0700 Subject: [PATCH 3/4] Fix tests --- tests/ActiveQueryTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ActiveQueryTest.php b/tests/ActiveQueryTest.php index 03b9cf95f..d437fe459 100644 --- a/tests/ActiveQueryTest.php +++ b/tests/ActiveQueryTest.php @@ -2007,7 +2007,7 @@ public function testOptimisticLock(): void public function testOptimisticLockOnDelete(): void { - $this->checkFixture($this->db, 'document'); + $this->checkFixture($this->db, 'document', true); $documentQuery = new ActiveQuery(Document::class, $this->db); $document = $documentQuery->findOne(1); @@ -2022,7 +2022,7 @@ public function testOptimisticLockOnDelete(): void public function testOptimisticLockAfterDelete(): void { - $this->checkFixture($this->db, 'document'); + $this->checkFixture($this->db, 'document', true); $documentQuery = new ActiveQuery(Document::class, $this->db); $document = $documentQuery->findOne(1); From de36badcfba80e02d4dac14dd88403c2ec3ce10d Mon Sep 17 00:00:00 2001 From: Tigrov Date: Tue, 2 Jan 2024 16:33:21 +0700 Subject: [PATCH 4/4] Improve --- src/BaseActiveRecord.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/BaseActiveRecord.php b/src/BaseActiveRecord.php index ad6b4ed83..2f5f139f7 100644 --- a/src/BaseActiveRecord.php +++ b/src/BaseActiveRecord.php @@ -1121,7 +1121,7 @@ protected function deleteInternal(): int $lock = $this->optimisticLock(); if ($lock !== null) { - $condition[$lock] = $this->getAttribute($lock); + $condition[$lock] = $this->$lock; $result = $this->deleteAll($condition);