Refactor BaseActiveRecord::getDirtyAttributes() (#283)

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

Hide file tree

Showing 3 changed files with 193 additions and 59 deletions.
diff --git a/src/BaseActiveRecord.php b/src/BaseActiveRecord.php
@@ -22,17 +22,22 @@
 use Yiisoft\Db\Helper\DbStringHelper;
 
 use function array_combine;
+use function array_diff_key;
 use function array_diff;
 use function array_flip;
 use function array_intersect;
+use function array_intersect_key;
 use function array_key_exists;
 use function array_keys;
+use function array_merge;
 use function array_search;
 use function array_values;
 use function count;
+use function get_object_vars;
 use function in_array;
 use function is_array;
 use function is_int;
+use function property_exists;
 use function reset;
 
 /**
@@ -131,21 +136,14 @@ public function getAttribute(string $name): mixed
 
     public function getAttributes(array $names = null, array $except = []): array
     {
-        $values = [];
-
-        if ($names === null) {
-            $names = $this->attributes();
-        }
+        $names ??= $this->attributes();
+        $attributes = array_merge($this->attributes, get_object_vars($this));
 
         if ($except !== []) {
             $names = array_diff($names, $except);
         }
 
-        foreach ($names as $name) {
-            $values[$name] = $this->$name;
-        }
-
-        return $values;
+        return array_intersect_key($attributes, array_flip($names));
     }
 
     public function getIsNewRecord(): bool
@@ -181,41 +179,21 @@ public function getOldAttribute(string $name): mixed
      */
     public function getDirtyAttributes(array $names = null): array
     {
-        if ($names === null) {
-            $names = $this->attributes();
+        $attributes = $this->getAttributes($names);
+
+        if ($this->oldAttributes === null) {
+            return $attributes;
         }
 
-        $names = array_flip($names);
-        $attributes = [];
+        $result = array_diff_key($attributes, $this->oldAttributes);
 
-        if ($this->oldAttributes === null) {
-            /**
-             * @var string $name
-             * @var mixed $value
-             */
-            foreach ($this->attributes as $name => $value) {
-                if (isset($names[$name])) {
-                    /** @psalm-var mixed */
-                    $attributes[$name] = $value;
-                }
-            }
-        } else {
-            /**
-             * @var string $name
-             * @var mixed $value
-             */
-            foreach ($this->attributes as $name => $value) {
-                if (
-                    isset($names[$name])
-                    && (!array_key_exists($name, $this->oldAttributes) || $value !== $this->oldAttributes[$name])
-                ) {
-                    /** @psalm-var mixed */
-                    $attributes[$name] = $value;
-                }
+        foreach (array_diff_key($attributes, $result) as $name => $value) {
+            if ($value !== $this->oldAttributes[$name]) {
+                $result[$name] = $value;
             }
         }
 
-        return $attributes;
+        return $result;
     }
 
     public function getOldAttributes(): array
@@ -582,21 +560,11 @@ public function optimisticLock(): string|null
      */
     public function populateRecord(array|object $row): void
     {
-        $columns = array_flip($this->attributes());
-
-        /**
-         * @psalm-var string $name
-         * @psalm-var mixed $value
-         */
         foreach ($row as $name => $value) {
-            if (isset($columns[$name])) {
-                $this->attributes[$name] = $value;
-            } elseif ($this->canSetProperty($name)) {
-                $this->$name = $value;
-            }
+            $this->populateAttribute($name, $value);
+            $this->oldAttributes[$name] = $value;
         }
 
-        $this->oldAttributes = $this->attributes;
         $this->related = [];
         $this->relationsDependencies = [];
     }
@@ -1246,4 +1214,13 @@ public function getTableName(): string
 
         return $this->tableName;
     }
+
+    private function populateAttribute(string $name, mixed $value): void
+    {
+        if (property_exists($this, $name)) {
+            $this->$name = $value;
+        } else {
+            $this->attributes[$name] = $value;
+        }
+    }
 }
diff --git a/tests/ActiveRecordTest.php b/tests/ActiveRecordTest.php
@@ -13,6 +13,7 @@
 use Yiisoft\ActiveRecord\Tests\Stubs\ActiveRecord\CustomerClosureField;
 use Yiisoft\ActiveRecord\Tests\Stubs\ActiveRecord\CustomerForArrayable;
 use Yiisoft\ActiveRecord\Tests\Stubs\ActiveRecord\CustomerWithAlias;
+use Yiisoft\ActiveRecord\Tests\Stubs\ActiveRecord\CustomerWithProperties;
 use Yiisoft\ActiveRecord\Tests\Stubs\ActiveRecord\Dog;
 use Yiisoft\ActiveRecord\Tests\Stubs\ActiveRecord\Item;
 use Yiisoft\ActiveRecord\Tests\Stubs\ActiveRecord\NoExist;
@@ -597,14 +598,9 @@ public function testAttributeAccess(): void
         $this->assertTrue($customer->canGetProperty('orderItems'));
         $this->assertFalse($customer->canSetProperty('orderItems'));
 
-        try {
-            /** @var $itemClass ActiveRecordInterface */
-            $customer->orderItems = [new Item($this->db)];
-            $this->fail('setter call above MUST throw Exception');
-        } catch (Exception $e) {
-            /** catch exception "Setting read-only property" */
-            $this->assertInstanceOf(InvalidCallException::class, $e);
-        }
+        $this->expectException(InvalidCallException::class);
+        $this->expectExceptionMessage('Setting read-only property: ' . Customer::class . '::orderItems');
+        $customer->orderItems = [new Item($this->db)];
 
         /** related attribute $customer->orderItems didn't change cause it's read-only */
         $this->assertSame([], $customer->orderItems);
@@ -872,4 +868,86 @@ public function testGetOldPrimaryKey(): void
         $this->assertSame(1, $customer->getOldPrimaryKey());
         $this->assertSame(['id' => 1], $customer->getOldPrimaryKey(true));
     }
+
+    public function testGetDirtyAttributesOnNewRecord(): void
+    {
+        $this->checkFixture($this->db, 'customer');
+
+        $customer = new Customer($this->db);
+
+        $this->assertSame([], $customer->getDirtyAttributes());
+
+        $customer->setAttribute('name', 'Adam');
+        $customer->setAttribute('email', '[email protected]');
+        $customer->setAttribute('address', null);
+
+        $this->assertEquals(
+            ['name' => 'Adam', 'email' => '[email protected]', 'address' => null],
+            $customer->getDirtyAttributes()
+        );
+        $this->assertEquals(
+            ['email' => '[email protected]', 'address' => null],
+            $customer->getDirtyAttributes(['id', 'email', 'address', 'status', 'unknown']),
+        );
+
+        $this->assertTrue($customer->save());
+        $this->assertSame([], $customer->getDirtyAttributes());
+
+        $customer->setAttribute('address', '');
+
+        $this->assertSame(['address' => ''], $customer->getDirtyAttributes());
+    }
+
+    public function testGetDirtyAttributesAfterFind(): void
+    {
+        $this->checkFixture($this->db, 'customer');
+
+        $customerQuery = new ActiveQuery(Customer::class, $this->db);
+        $customer = $customerQuery->findOne(1);
+
+        $this->assertSame([], $customer->getDirtyAttributes());
+
+        $customer->setAttribute('name', 'Adam');
+        $customer->setAttribute('email', '[email protected]');
+        $customer->setAttribute('address', null);
+
+        $this->assertEquals(
+            ['name' => 'Adam', 'email' => '[email protected]', 'address' => null],
+            $customer->getDirtyAttributes(),
+        );
+        $this->assertEquals(
+            ['email' => '[email protected]', 'address' => null],
+            $customer->getDirtyAttributes(['id', 'email', 'address', 'status', 'unknown']),
+        );
+    }
+
+    public function testGetDirtyAttributesWithProperties(): void
+    {
+        $this->checkFixture($this->db, 'customer');
+
+        $customer = new CustomerWithProperties($this->db);
+        $this->assertSame([
+            'name' => null,
+            'address' => null,
+        ], $customer->getDirtyAttributes());
+
+        $customerQuery = new ActiveQuery(CustomerWithProperties::class, $this->db);
+        $customer = $customerQuery->findOne(1);
+
+        $this->assertSame([], $customer->getDirtyAttributes());
+
+        $customer->setEmail('[email protected]');
+        $customer->setName('Adam');
+        $customer->setAddress(null);
+        $customer->setStatus(null);
+
+        $this->assertEquals(
+            ['email' => '[email protected]', 'name' => 'Adam', 'address' => null, 'status' => null],
+            $customer->getDirtyAttributes(),
+        );
+        $this->assertEquals(
+            ['email' => '[email protected]', 'address' => null],
+            $customer->getDirtyAttributes(['id', 'email', 'address', 'unknown']),
+        );
+    }
 }
diff --git a/tests/Stubs/ActiveRecord/CustomerWithProperties.php b/tests/Stubs/ActiveRecord/CustomerWithProperties.php
@@ -0,0 +1,79 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Yiisoft\ActiveRecord\Tests\Stubs\ActiveRecord;
+
+use Yiisoft\ActiveRecord\ActiveQuery;
+use Yiisoft\ActiveRecord\ActiveRecord;
+
+/**
+ * Class Customer with defined properties.
+ */
+class CustomerWithProperties extends ActiveRecord
+{
+    protected int $id;
+    protected string $email;
+    protected string|null $name = null;
+    public string|null $address = null;
+
+    public function getTableName(): string
+    {
+        return 'customer';
+    }
+
+    public function getId(): int
+    {
+        return $this->id;
+    }
+
+    public function getEmail(): string
+    {
+        return $this->email;
+    }
+
+    public function getName(): string|null
+    {
+        return $this->name;
+    }
+
+    public function getAddress(): string|null
+    {
+        return $this->address;
+    }
+
+    public function getStatus(): int|null
+    {
+        return $this->getAttribute('status');
+    }
+
+    public function getProfile(): ActiveQuery
+    {
+        return $this->hasOne(Profile::class, ['id' => 'profile_id']);
+    }
+
+    public function getOrders(): ActiveQuery
+    {
+        return $this->hasMany(Order::class, ['customer_id' => 'id'])->orderBy('[[id]]');
+    }
+
+    public function setEmail(string $email): void
+    {
+        $this->email = $email;
+    }
+
+    public function setName(string|null $name): void
+    {
+        $this->name = $name;
+    }
+
+    public function setAddress(string|null $address): void
+    {
+        $this->address = $address;
+    }
+
+    public function setStatus(int|null $status): void
+    {
+        $this->setAttribute('status', $status);
+    }
+}