Skip to content

Commit

Permalink
Refactor update...() methods (#282)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Tigrov
Tigrov authored Jan 14, 2024
1 parent 2ba318b commit c5f92e5
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 76 deletions.
3 changes: 3 additions & 0 deletions psalm.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,7 @@
<directory name="vendor" />
</ignoreFiles>
</projectFiles>
<issueHandlers>
<MixedAssignment errorLevel="suppress" />
</issueHandlers>
</psalm>
6 changes: 1 addition & 5 deletions src/ActiveRecord.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down
29 changes: 20 additions & 9 deletions src/ActiveRecordInterface.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand All @@ -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();
* }
* ```
*
Expand Down
95 changes: 33 additions & 62 deletions src/BaseActiveRecord.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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();

Expand All @@ -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;
}

Expand All @@ -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;
Expand Down Expand Up @@ -660,7 +651,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
Expand Down Expand Up @@ -739,7 +732,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);
}
Expand All @@ -757,16 +750,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;
Expand All @@ -780,11 +765,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;
Expand Down Expand Up @@ -848,6 +832,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
*
Expand All @@ -857,18 +843,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
Expand Down Expand Up @@ -1180,41 +1165,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;
}

Expand Down
36 changes: 36 additions & 0 deletions tests/ActiveRecordTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -836,4 +836,40 @@ 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);
$customer->id = 2;

$this->assertSame(1, $customer->getOldPrimaryKey());
$this->assertSame(['id' => 1], $customer->getOldPrimaryKey(true));
}
}

0 comments on commit c5f92e5

Please sign in to comment.