Refactor update...() methods

psalm.xml

@@ -14,4 +14,7 @@
         <directory name="vendor" />
     </ignoreFiles>
   </projectFiles>
+  <issueHandlers>
+    <MixedAssignment errorLevel="suppress" />
+  </issueHandlers>
 </psalm>

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;
         }

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<string, mixed>
+     *     : 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<string, mixed>
+     *     : 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();
      * }
      * ```
      *

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<string> $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<string> $keys */
         foreach ($keys as $name) {
-            /** @psalm-var string|null */
-            $values[$name] = $this->attributes[$name] ?? null;
+            $values[$name] = $this->$name;
         }
 
         return $values;
@@ -653,7 +644,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
@@ -732,7 +725,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);
     }
@@ -750,16 +743,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;
@@ -773,11 +758,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<string, mixed> $value */
         foreach ($values as $name => $value) {
-            $this->oldAttributes[$name] = $this->attributes[$name];
+            $this->oldAttributes[$name] = $value;
         }
 
         return $rows;
@@ -841,6 +825,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<string, int> $counters
+     *
      * @throws Exception
      * @throws NotSupportedException
      *
@@ -850,18 +836,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<string, int> $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
@@ -1171,41 +1156,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<array-key, mixed>|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<array-key, mixed>|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;
         }
 

tests/ActiveRecordTest.php

@@ -836,4 +836,39 @@ 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);
+
+        $this->assertSame(1, $customer->getOldPrimaryKey());
+        $this->assertSame(['id' => 1], $customer->getOldPrimaryKey(true));
+    }
 }