From ade14f6489739e26ec053b95ccc74120d81a2e4d Mon Sep 17 00:00:00 2001 From: Tigrov Date: Sun, 17 Dec 2023 09:55:30 +0700 Subject: [PATCH 1/5] Refactor `BaseActiveRecord::getDirtyAttributes()` --- src/BaseActiveRecord.php | 42 ++++++++++-------------------- tests/ActiveRecordTest.php | 52 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 28 deletions(-) diff --git a/src/BaseActiveRecord.php b/src/BaseActiveRecord.php index d6cb497e3..eaa4eedc0 100644 --- a/src/BaseActiveRecord.php +++ b/src/BaseActiveRecord.php @@ -22,8 +22,10 @@ use Yiisoft\Db\Helper\DbStringHelper; use function array_combine; +use function array_diff_key; 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_search; @@ -188,40 +190,24 @@ public function getOldAttribute(string $name): mixed public function getDirtyAttributes(array $names = null): array { if ($names === null) { - $names = $this->attributes(); + $attributes = $this->attributes; + } else { + $attributes = array_intersect_key($this->attributes, array_flip($names)); } - $names = array_flip($names); - $attributes = []; - 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; - } + return $attributes; + } + + $result = array_diff_key($attributes, $this->oldAttributes); + + 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 diff --git a/tests/ActiveRecordTest.php b/tests/ActiveRecordTest.php index 903ee53ce..a85d2bbcf 100644 --- a/tests/ActiveRecordTest.php +++ b/tests/ActiveRecordTest.php @@ -836,4 +836,56 @@ public function testToArrayForArrayable(): void ]), ); } + + 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', 'adam@example.com'); + $customer->setAttribute('address', null); + + $this->assertEquals( + ['name' => 'Adam', 'email' => 'adam@example.com', 'address' => null], + $customer->getDirtyAttributes() + ); + $this->assertEquals( + ['email' => 'adam@example.com', 'address' => null], + $customer->getDirtyAttributes(['id', 'email', 'address', 'status']), + ); + + $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', 'adam@example.com'); + $customer->setAttribute('address', null); + + $this->assertEquals( + ['name' => 'Adam', 'email' => 'adam@example.com', 'address' => null], + $customer->getDirtyAttributes(), + ); + $this->assertEquals( + ['email' => 'adam@example.com', 'address' => null], + $customer->getDirtyAttributes(['id', 'email', 'address', 'status']), + ); + } } From 78023fe72f084a5be15a0807c17b8f4976260c30 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Fri, 29 Dec 2023 16:19:08 +0700 Subject: [PATCH 2/5] Fix AR with properties --- src/BaseActiveRecord.php | 33 ++++---- tests/ActiveRecordTest.php | 46 ++++++++--- .../ActiveRecord/CustomerWithProperties.php | 79 +++++++++++++++++++ 3 files changed, 131 insertions(+), 27 deletions(-) create mode 100644 tests/Stubs/ActiveRecord/CustomerWithProperties.php diff --git a/src/BaseActiveRecord.php b/src/BaseActiveRecord.php index cc19dbf40..485bc7ccf 100644 --- a/src/BaseActiveRecord.php +++ b/src/BaseActiveRecord.php @@ -29,12 +29,15 @@ 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; /** @@ -187,11 +190,8 @@ public function getOldAttribute(string $name): mixed */ public function getDirtyAttributes(array $names = null): array { - if ($names === null) { - $attributes = $this->attributes; - } else { - $attributes = array_intersect_key($this->attributes, array_flip($names)); - } + $attributes = array_merge($this->attributes, get_object_vars($this)); + $attributes = array_intersect_key($attributes, array_flip($names ?? $this->attributes())); if ($this->oldAttributes === null) { return $attributes; @@ -570,21 +570,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 = []; } @@ -1252,4 +1242,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 index a85d2bbcf..5ccc4b8ef 100644 --- 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(UnknownPropertyException::class); + $this->expectExceptionMessage('Setting unknown 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); @@ -855,7 +851,7 @@ public function testGetDirtyAttributesOnNewRecord(): void ); $this->assertEquals( ['email' => 'adam@example.com', 'address' => null], - $customer->getDirtyAttributes(['id', 'email', 'address', 'status']), + $customer->getDirtyAttributes(['id', 'email', 'address', 'status', 'unknown']), ); $this->assertTrue($customer->save()); @@ -885,7 +881,37 @@ public function testGetDirtyAttributesAfterFind(): void ); $this->assertEquals( ['email' => 'adam@example.com', 'address' => null], - $customer->getDirtyAttributes(['id', 'email', 'address', 'status']), + $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('adam@example.com'); + $customer->setName('Adam'); + $customer->setAddress(null); + $customer->setStatus(null); + + $this->assertEquals( + ['email' => 'adam@example.com', 'name' => 'Adam', 'address' => null, 'status' => null], + $customer->getDirtyAttributes(), + ); + $this->assertEquals( + ['email' => 'adam@example.com', 'address' => null], + $customer->getDirtyAttributes(['id', 'email', 'address', 'unknown']), ); } } diff --git a/tests/Stubs/ActiveRecord/CustomerWithProperties.php b/tests/Stubs/ActiveRecord/CustomerWithProperties.php new file mode 100644 index 000000000..4aba20cc6 --- /dev/null +++ b/tests/Stubs/ActiveRecord/CustomerWithProperties.php @@ -0,0 +1,79 @@ +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); + } +} From bd6e59efccc14c400234aa432be355a2f1cee8f6 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Fri, 29 Dec 2023 09:19:29 +0000 Subject: [PATCH 3/5] Apply fixes from StyleCI --- tests/ActiveRecordTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/ActiveRecordTest.php b/tests/ActiveRecordTest.php index 5ccc4b8ef..616c27f9f 100644 --- a/tests/ActiveRecordTest.php +++ b/tests/ActiveRecordTest.php @@ -25,7 +25,6 @@ use Yiisoft\ActiveRecord\Tests\Support\Assert; use Yiisoft\Db\Exception\Exception; use Yiisoft\Db\Exception\InvalidArgumentException; -use Yiisoft\Db\Exception\InvalidCallException; use Yiisoft\Db\Exception\InvalidConfigException; use Yiisoft\Db\Exception\UnknownPropertyException; use Yiisoft\Db\Query\Query; From 4f9a24adb7202864afb5f5434e5b5d0de2cc3de5 Mon Sep 17 00:00:00 2001 From: Tigrov Date: Sat, 30 Dec 2023 14:49:26 +0700 Subject: [PATCH 4/5] Fix tests --- tests/ActiveRecordTest.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/ActiveRecordTest.php b/tests/ActiveRecordTest.php index 616c27f9f..576618406 100644 --- a/tests/ActiveRecordTest.php +++ b/tests/ActiveRecordTest.php @@ -25,6 +25,7 @@ use Yiisoft\ActiveRecord\Tests\Support\Assert; use Yiisoft\Db\Exception\Exception; use Yiisoft\Db\Exception\InvalidArgumentException; +use Yiisoft\Db\Exception\InvalidCallException; use Yiisoft\Db\Exception\InvalidConfigException; use Yiisoft\Db\Exception\UnknownPropertyException; use Yiisoft\Db\Query\Query; @@ -597,8 +598,8 @@ public function testAttributeAccess(): void $this->assertTrue($customer->canGetProperty('orderItems')); $this->assertFalse($customer->canSetProperty('orderItems')); - $this->expectException(UnknownPropertyException::class); - $this->expectExceptionMessage('Setting unknown property: ' . Customer::class . '::orderItems'); + $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 */ From 002c3563f99974d4d327cab86a9070b205d76f5a Mon Sep 17 00:00:00 2001 From: Tigrov Date: Fri, 12 Jan 2024 09:32:20 +0700 Subject: [PATCH 5/5] Improve `BaseActiveRecord::getAttributes()` --- src/BaseActiveRecord.php | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/BaseActiveRecord.php b/src/BaseActiveRecord.php index 485bc7ccf..64e3406f5 100644 --- a/src/BaseActiveRecord.php +++ b/src/BaseActiveRecord.php @@ -140,21 +140,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 @@ -190,8 +183,7 @@ public function getOldAttribute(string $name): mixed */ public function getDirtyAttributes(array $names = null): array { - $attributes = array_merge($this->attributes, get_object_vars($this)); - $attributes = array_intersect_key($attributes, array_flip($names ?? $this->attributes())); + $attributes = $this->getAttributes($names); if ($this->oldAttributes === null) { return $attributes;