Refactor delete...() methods (#284)

yiisoft · Jan 14, 2024 · d2fa237 · d2fa237
1 parent fb7f4a8
commit d2fa237
Show file tree

Hide file tree

Showing 4 changed files with 71 additions and 66 deletions.
diff --git 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;
@@ -114,7 +113,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 +123,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,41 +341,6 @@ public function update(array $attributeNames = null): 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 ($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
@@ -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
@@ -66,29 +66,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);
-
-        $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
@@ -1088,6 +1068,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->$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
@@ -2005,6 +2005,36 @@ public function testOptimisticLock(): void
         $record->save();
     }
 
+    public function testOptimisticLockOnDelete(): void
+    {
+        $this->checkFixture($this->db, 'document', true);
+
+        $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', true);
+
+        $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}
      */